+ The lba is set to -1 to avoid some code paths, to make PoC simpler.
void ide_atapi_cmd_reply_end(IDEState *s)
{
int byte_count_limit, size, ret;
while (s->packet_transfer_size > 0) {
.....
if (s->lba != -1 && s->io_buffer_index >= s->cd_sector_size) {
<----- set lba to -1 to avoid this part
.....
}
if (s->elementary_transfer_size > 0) {
......
} else {
.......
if (s->lba != -1) { <-----
if (size > (s->cd_sector_size - s->io_buffer_index))
size = (s->cd_sector_size - s->io_buffer_index); <-----
}
}
Wenxiang Qian <leonwxq...@gmail.com> 于2020年12月11日周五 下午4:23写道：
> Hello,
>
> I may not have made the detail clear in my previous email. The details of
> the AHCI device, after running the reproducer I attached in my report are
> as follows. If there is any information I can provide, please let me know.
> Thank you.
>
> ###root cause###
> (1) The s->packet_transfer_size is bigger than the actual data.
> (2) packet_transfer_size is passed into ide_atapi_cmd_reply_end, as the
> total number of iterations. Each iterate round, s->io_buffer_index is
> increased by 2048, but without boundary check.
> (3) The call to ide_transfer_start_norecurse use s->io_buffer +
> s->io_buffer_index - size as the index, cause an OOB access.
>
> ###details###
> 1. The reproducer sends a command of [WIN_PACKETCMD] + [CMD_READ] and
> value of IDE device's registers from guest to qemu.
>
> Callback ahci_port_write is called, then check_cmd is called.
>
> 2. The packet will set all the registers of the device via: handle_cmd -->
> handle_reg_h2d_fis.
>
> Registers will be set here:
>
> handle_reg_h2d_fis(..){
> ...
> ide_state->feature = cmd_fis[3]; //######[1]###### , cmd_fis is the
> data sent by the reproducer.
> ide_state->sector = cmd_fis[4]; /* LBA 7:0 */
> ide_state->lcyl = cmd_fis[5]; /* LBA 15:8 */
> ide_state->hcyl = cmd_fis[6]; /* LBA 23:16 */
> ide_state->select = cmd_fis[7]; /* LBA 27:24 (LBA28) */
> ide_state->hob_sector = cmd_fis[8]; /* LBA 31:24 */
> ide_state->hob_lcyl = cmd_fis[9]; /* LBA 39:32 */
> ide_state->hob_hcyl = cmd_fis[10]; /* LBA 47:40 */
> ide_state->hob_feature = cmd_fis[11];
> ide_state->nsector = (int64_t)((cmd_fis[13] << 8) | cmd_fis[12]);
>
> and ide_exec_cmd will be called to process [WIN_PACKETCMD] command.
> ide_exec_cmd(&s->dev[port].port, cmd_fis[2]);
>
> 3. ide_exec_cmd (core.c) handles the command,
>
> [WIN_PACKETCMD] = { cmd_packet, CD_OK },
>
> and make a call to cmd_packet,
>
> cmd_packet(...) {
> ...
>
> s->atapi_dma = s->feature & 1; //######[2]######
> if (s->atapi_dma) {
> s->dma_cmd = IDE_DMA_ATAPI;
> }
> s->nsector = 1;
> ide_transfer_start(s, s->io_buffer, ATAPI_PACKET_SIZE,
> ide_atapi_cmd);
> ...
> }
>
> and set the device to use PIO mode according to s->feature (set in Step
> 2->##[1]##).
>
> Then, ide_transfer_start is called.
> It will pass the [CMD_READ] part after [WIN_PACKETCMD] to ide_atapi_cmd.
>
> 4. ide_atapi_cmd parses [CMD_READ], and then calls the corresponding
> handler: cmd_read.
>
> [ 0x28 ] = { cmd_read, /* (10) */ CHECK_READY },
>
> In cmd_read, the values of nb_sectors and lba are determined according to
> the packets passed by the reproducer.
>
> In my PoC I set lba to -1 (0xfffffff) and nb_sectors to a large value,
> such as 0x800.
>
>
> static void cmd_read(IDEState *s, uint8_t* buf)
> {
> int nb_sectors, lba;
>
> if (buf[0] == GPCMD_READ_10) {
> nb_sectors = lduw_be_p(buf + 7);
> } else {
> nb_sectors = ldl_be_p(buf + 6); //#####3#####
> }
>
> lba = ldl_be_p(buf + 2); //######4######
>
> ....
>
> ide_atapi_cmd_read(s, lba, nb_sectors, 2048);
> }
>
> 5. The code enters the ide_atapi_cmd_read -> ide_atapi_cmd_read_pio.
>
> static void ide_atapi_cmd_read(.....)
> {...
> if (s->atapi_dma) {
> ide_atapi_cmd_read_dma(s, lba, nb_sectors, sector_size);
> } else {
> ide_atapi_cmd_read_pio(s, lba, nb_sectors, sector_size);
> //######5#######
> }
> }
>
> It will set the attributes according to the value passed in the previous
> steps.
> The number of s->packet_transfer_size, which is the packet to read or
> write, nb_sectors * 2048 can be larger than the buffer pre-allocated by
> qemu (about 256KB).
>
>
> static void ide_atapi_cmd_read_pio(IDEState *s, int lba, int nb_sectors,
> int sector_size)
> {
> s->lba = lba;
> s->packet_transfer_size = nb_sectors * sector_size;
> //########6#########
> s->elementary_transfer_size = 0;
> s->io_buffer_index = sector_size;
> s->cd_sector_size = sector_size;
>
> ide_atapi_cmd_reply_end(s);
> }
>
>
> 6. In ide_atapi_cmd_reply_end, the data is processed according to
> packet_transfer_size.
>
> void ide_atapi_cmd_reply_end(IDEState *s)
> {
> ...
> while (s->packet_transfer_size > 0) { //########7#######
> ....
> s->packet_transfer_size -= size;
> s->elementary_transfer_size -= size;
> s->io_buffer_index += size; //#######8#######
>
> if (!ide_transfer_start_norecurse(s,
> s->io_buffer +
> s->io_buffer_index - size,
> size, ide_atapi_cmd_reply_end)) {
> return;
> }
>
>
> The "size" is usually 2048, which means the io_buffer_index increases by
> 2048 per round.
>
> Qemu does not test if the result of this operation exceeds the length of
> the io_buffer itself, resulting in out-of-bounds access.
>
> In ide_transfer_start_norecurse,
>
> bool ide_transfer_start_norecurse(IDEState *s, uint8_t *buf, int size,
> EndTransferFunc *end_transfer_func)
> {
> s->data_ptr = buf; //s->io_buffer + s->io_buffer_index - size
> s->data_end = buf + size; //data_ptr + 2048
> ....
> s->bus->dma->ops->pio_transfer(s->bus->dma); //#######9########
> return true;
> }
>
> //####9####:
> static const IDEDMAOps ahci_dma_ops = {
> ...
> .pio_transfer = ahci_pio_transfer,
> ...
> };
>
> In the final processing function ahci_pio_transfer:
>
> static void ahci_pio_transfer(const IDEDMA *dma)
> {
> ....
>
> uint32_t size = (uint32_t)(s->data_end - s->data_ptr); // 2048,
> usually
>
> uint16_t opts = le16_to_cpu(ad->cur_cmd->opts); //####user controlled
> value#####
> int is_write = opts & AHCI_CMD_WRITE; // read or write is
> decided by user.
>
> .....
>
>
> if (has_sglist && size) {
> if (is_write) {
> dma_buf_write(s->data_ptr, size, &s->sg); //##10##### both
> can be reached ####
> } else {
> dma_buf_read(s->data_ptr, size, &s->sg); //##11##### both
> can be reached ####
> }
> }
> }
>
>
> s->data_ptr can be a value out of range, so base on ad->cur_cmd->opts,
> ##10## ##11## can be OOB read or OOB write.
>
> OOB read: obtain the leaked information, which can be used to bypass ASLR
> or obtain information about the host.
> OOB write: which may overwrite some structs of the virtual device after
> it, overwrite the function pointers in the struct.
>
> Best regards,
> Wenxiang Qian
>
> P J P <ppan...@redhat.com> 于2020年12月2日周三 下午9:17写道：
>
>> Hi,
>>
>> [doing a combined reply]
>>
>> +-- On Tue, 1 Dec 2020, Philippe Mathieu-Daudé wrote --+
>> | Is it possible to release the reproducer to the community, so we can
>> work on
>> | a fix and test it?
>>
>> * No, we can not release/share reproducers on a public list.
>>
>> * We can request reporters to do so by their volition.
>>
>>
>> | What happens to reproducers when a CVE is assigned, but the bug is
>> marked as
>> | "out of the QEMU security boundary"?
>> |
>> +-- On Tue, 1 Dec 2020, Peter Maydell wrote --+
>> | Also, why are we assigning CVEs for bugs we don't consider security
>> bugs?
>>
>> * We need to mark these componets 'out of security scope' at the source
>> level,
>> rather than on each bug.
>>
>> * Easiest could be to just have a list of such components in the git text
>> file. At least till the time we device --security build and run time
>> option
>> discussed earlier.
>> ->
>> https://lists.nongnu.org/archive/html/qemu-devel/2020-07/msg04680.html
>>
>> +-- On Tue, 1 Dec 2020, Paolo Bonzini wrote --+
>> | qtests are not just helpful. Adding regression tests for bugs is a
>> *basic*
>> | software engineering principle. If you don't have time to write tests,
>> you
>> | (or your organization) should find it.
>>
>> * I've been doing the patch work out of my own interest.
>>
>> * We generally rely on upstream/engineering for fix patches, because of
>> our
>> narrower understanding of the code base.
>>
>> +-- On Wed, 2 Dec 2020, Markus Armbruster wrote --+
>> | Paolo Bonzini <pbonz...@redhat.com> writes:
>> | > you need at least to enclose the reproducer, otherwise you're posting
>> a
>> | > puzzle not a patch. :)
>> |
>> | Indeed. Posting puzzles is a negative-sum game.]
>>
>> * Agreed. I think the upcoming 'qemu-security' list may help in this
>> regard.
>> As issue reports+reproducer details shall go there.
>>
>> * Even then, we'll need to ask reporter's permission before sharing their
>> reproducers on a public list OR with non-members.
>>
>> * Best is if reporters share/release reproducers themselves. Maybe we can
>> have
>> a public git repository and they can send a PR to include their
>> reproducers
>> in the repository.
>>
>> * That way multiple reproducers for the same issue can be held together.
>>
>>
>> Thank you.
>> --
>> Prasad J Pandit / Red Hat Product Security Team
>> 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
>
>
