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
