| Message ID | 20201201120926.56559-1-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] ide: atapi: assert that the buffer pointer is in range | expand |
Am 01.12.2020 um 13:09 hat Paolo Bonzini geschrieben: > A case was reported where s->io_buffer_index can be out of range. > The report skimped on the details but it seems to be triggered > by s->lba == -1 on the READ/READ CD paths (e.g. by sending an > ATAPI command with LBA = 0xFFFFFFFF). For now paper over it > with assertions. The first one ensures that there is no overflow > when incrementing s->io_buffer_index, the second checks for the > buffer overrun. > > Note that the buffer overrun is only a read, so I am not sure > if the assertion failure is actually less harmful than the overrun. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> I don't think reading LBA 0xFFFFFFFF from a CD image would ever be valid (or at least I have never seen an 8 TB CD...), so it's probably a malicious guest. Assertion failure seems okay to me, guests have already enough ways to kill themselves, so it feels slightly preferable to an information leak. Reviewed-by: Kevin Wolf <kwolf@redhat.com>
On Tue, 1 Dec 2020 at 15:17, Kevin Wolf <kwolf@redhat.com> wrote: > > Am 01.12.2020 um 13:09 hat Paolo Bonzini geschrieben: > > A case was reported where s->io_buffer_index can be out of range. > > The report skimped on the details but it seems to be triggered > > by s->lba == -1 on the READ/READ CD paths (e.g. by sending an > > ATAPI command with LBA = 0xFFFFFFFF). For now paper over it > > with assertions. The first one ensures that there is no overflow > > when incrementing s->io_buffer_index, the second checks for the > > buffer overrun. > > > > Note that the buffer overrun is only a read, so I am not sure > > if the assertion failure is actually less harmful than the overrun. > > > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > I don't think reading LBA 0xFFFFFFFF from a CD image would ever be > valid (or at least I have never seen an 8 TB CD...), so it's probably a > malicious guest. Assertion failure seems okay to me, guests have already > enough ways to kill themselves, so it feels slightly preferable to an > information leak. > > Reviewed-by: Kevin Wolf <kwolf@redhat.com> Thanks; applied to master for 5.2. -- PMM
diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c index 14a2b0bb2f..e79157863f 100644 --- a/hw/ide/atapi.c +++ b/hw/ide/atapi.c @@ -276,6 +276,8 @@ void ide_atapi_cmd_reply_end(IDEState *s) s->packet_transfer_size -= size; s->elementary_transfer_size -= size; s->io_buffer_index += size; + assert(size <= s->io_buffer_total_len); + assert(s->io_buffer_index <= s->io_buffer_total_len); /* Some adapters process PIO data right away. In that case, we need * to avoid mutual recursion between ide_transfer_start
A case was reported where s->io_buffer_index can be out of range. The report skimped on the details but it seems to be triggered by s->lba == -1 on the READ/READ CD paths (e.g. by sending an ATAPI command with LBA = 0xFFFFFFFF). For now paper over it with assertions. The first one ensures that there is no overflow when incrementing s->io_buffer_index, the second checks for the buffer overrun. Note that the buffer overrun is only a read, so I am not sure if the assertion failure is actually less harmful than the overrun. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- hw/ide/atapi.c | 2 ++ 1 file changed, 2 insertions(+)