[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] fdc: force the fifo access to be in bounds of t
From: |
John Snow |
Subject: |
Re: [Qemu-devel] [PATCH] fdc: force the fifo access to be in bounds of the allocated buffer |
Date: |
Wed, 13 May 2015 16:54:33 -0400 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 |
On 05/13/2015 02:51 PM, Stefan Weil wrote:
> Hi,
>
> I just noticed this patch because my provider told me that my KVM based
> server
> needs a reboot because of a CVE (see this German news:
> http://www.heise.de/newsticker/meldung/Venom-Schwachstelle-Aus-Hypervisor-ausbrechen-und-VMs-ausspionieren-2649614.html)
>
>
> Am 13.05.2015 um 16:33 schrieb John Snow:
>> From: Petr Matousek <address@hidden>
>>
>> During processing of certain commands such as FD_CMD_READ_ID and
>> FD_CMD_DRIVE_SPECIFICATION_COMMAND the fifo memory access could
>> get out of bounds leading to memory corruption with values coming
>> from the guest.
>>
>> Fix this by making sure that the index is always bounded by the
>> allocated memory.
>>
>> This is CVE-2015-3456.
>>
>> Signed-off-by: Petr Matousek <address@hidden>
>> Reviewed-by: John Snow <address@hidden>
>> Signed-off-by: John Snow <address@hidden>
>> ---
>> hw/block/fdc.c | 17 +++++++++++------
>> 1 file changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>> index f72a392..d8a8edd 100644
>> --- a/hw/block/fdc.c
>> +++ b/hw/block/fdc.c
>> @@ -1497,7 +1497,7 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
>> {
>> FDrive *cur_drv;
>> uint32_t retval = 0;
>> - int pos;
>> + uint32_t pos;
>> cur_drv = get_cur_drv(fdctrl);
>> fdctrl->dsr &= ~FD_DSR_PWRDOWN;
>> @@ -1506,8 +1506,8 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
>> return 0;
>> }
>> pos = fdctrl->data_pos;
>> + pos %= FD_SECTOR_LEN;
>
> I'd combine both statements and perhaps use fdctrl->fifo_size (even if
> the resulting code will be slightly larger):
>
Sure. Send me a patch and I'll ACK it.
> pos = fdctrl->data_pos % fdctrl->fifo_size;
>
>
>> if (fdctrl->msr & FD_MSR_NONDMA) {
>> - pos %= FD_SECTOR_LEN;
>> if (pos == 0) {
>> if (fdctrl->data_pos != 0)
>> if (!fdctrl_seek_to_next_sect(fdctrl, cur_drv)) {
>> @@ -1852,10 +1852,13 @@ static void fdctrl_handle_option(FDCtrl
>> *fdctrl, int direction)
>> static void fdctrl_handle_drive_specification_command(FDCtrl
>> *fdctrl, int direction)
>> {
>> FDrive *cur_drv = get_cur_drv(fdctrl);
>> + uint32_t pos;
>> - if (fdctrl->fifo[fdctrl->data_pos - 1] & 0x80) {
>> + pos = fdctrl->data_pos - 1;
>> + pos %= FD_SECTOR_LEN;
>
> Shorter (and more clear):
>
> uint32_t pos = (fdctrl->data_pos - 1) % fdctrl->fifo_size;
>
Good here, too.
>> + if (fdctrl->fifo[pos] & 0x80) {
>> /* Command parameters done */
>> - if (fdctrl->fifo[fdctrl->data_pos - 1] & 0x40) {
>> + if (fdctrl->fifo[pos] & 0x40) {
>> fdctrl->fifo[0] = fdctrl->fifo[1];
>> fdctrl->fifo[2] = 0;
>> fdctrl->fifo[3] = 0;
>> @@ -1955,7 +1958,7 @@ static uint8_t command_to_handler[256];
>> static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
>> {
>> FDrive *cur_drv;
>> - int pos;
>> + uint32_t pos;
>> /* Reset mode */
>> if (!(fdctrl->dor & FD_DOR_nRESET)) {
>> @@ -2004,7 +2007,9 @@ static void fdctrl_write_data(FDCtrl *fdctrl,
>> uint32_t value)
>> }
>> FLOPPY_DPRINTF("%s: %02x\n", __func__, value);
>> - fdctrl->fifo[fdctrl->data_pos++] = value;
>> + pos = fdctrl->data_pos++;
>> + pos %= FD_SECTOR_LEN;
>> + fdctrl->fifo[pos] = value;
>> if (fdctrl->data_pos == fdctrl->data_len) {
>> /* We now have all parameters
>> * and will be able to treat the command
>
> Not strictly related to this patch: The code which sets fifo_size could
> also be improved.
>
> fdctrl->fifo = qemu_memalign(512, FD_SECTOR_LEN);
> fdctrl->fifo_size = 512;
>
> The 2nd line should be
>
> fdctrl->fifo_size = FD_SECTOR_LEN;
>
Agreed, and it came up during the review for this, but we kept it out to
keep this a one patch targeted fix.
Also arising from the review: I want to move tmpbuf off of the stack,
though that particular buffer appears to be properly bounded at all times.
>
> As far as I see the original code can read or write illegal memory
> locations in the address space of the QEMU process. It cannot (as it was
> claimed) modify the code of the VM host because those memory is usually
> write protected - at least if QEMU is running without KVM. If the code
> which is generated for KVM is writable from anywhere in QEMU, we should
> perhaps consider changing that.
>
I don't think we are aware of any particular weaknesses, the security
report only said the "possibility" of arbitrary code execution due to
the buffer overflow. I haven't heard any more detailed explanation than
this.
> Regards
> Stefan
>
Thanks,
--js
- Re: [Qemu-devel] [Qemu-stable] [PATCH] fdc: force the fifo access to be in bounds of the allocated buffer, (continued)
- Re: [Qemu-devel] [Qemu-stable] [PATCH] fdc: force the fifo access to be in bounds of the allocated buffer, Stefan Priebe, 2015/05/13
- Re: [Qemu-devel] [Qemu-stable] [PATCH] fdc: force the fifo access to be in bounds of the allocated buffer, Stefan Weil, 2015/05/13
- Re: [Qemu-devel] [Qemu-stable] [PATCH] fdc: force the fifo access to be in bounds of the allocated buffer, Stefan Priebe, 2015/05/13
- Re: [Qemu-devel] [Qemu-stable] [PATCH] fdc: force the fifo access to be in bounds of the allocated buffer, Peter Lieven, 2015/05/13
- Re: [Qemu-devel] [Qemu-stable] [PATCH] fdc: force the fifo access to be in bounds of the allocated buffer, Markus Armbruster, 2015/05/13
- Re: [Qemu-devel] [Qemu-stable] [PATCH] fdc: force the fifo access to be in bounds of the allocated buffer, Peter Lieven, 2015/05/13
- Re: [Qemu-devel] [Qemu-stable] [PATCH] fdc: force the fifo access to be in bounds of the allocated buffer, John Snow, 2015/05/13
- Re: [Qemu-devel] [Qemu-stable] [PATCH] fdc: force the fifo access to be in bounds of the allocated buffer, Peter Lieven, 2015/05/13
Re: [Qemu-devel] [PATCH] fdc: force the fifo access to be in bounds of the allocated buffer,
John Snow <=