Re: [PATCH] fdc: check drive block device before usage (CVE-2021-20196)

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 5/14/21 9:26 PM, John Snow wrote:
> On 5/14/21 3:23 PM, Thomas Huth wrote:
>> On 23/01/2021 11.03, P J P wrote:
>>> From: Prasad J Pandit <pjp@fedoraproject.org>
>>>
>>> While processing ioport command in 'fdctrl_write_dor', device
>>> controller may select a drive which is not initialised with a
>>> block device. This may result in a NULL pointer dereference.
>>> Add checks to avoid it.
>>>
>>> Fixes: CVE-2021-20196
>>> Reported-by: Gaoning Pan <pgn@zju.edu.cn>
>>> Buglink: https://bugs.launchpad.net/qemu/+bug/1912780
>>> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
>>> ---
>>>   hw/block/fdc.c | 11 +++++++++--
>>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>>> index 3636874432..13a9470d19 100644
>>> --- a/hw/block/fdc.c
>>> +++ b/hw/block/fdc.c
>>> @@ -1429,7 +1429,9 @@ static void fdctrl_write_dor(FDCtrl *fdctrl,
>>> uint32_t value)
>>>           }
>>>       }
>>>       /* Selected drive */
>>> -    fdctrl->cur_drv = value & FD_DOR_SELMASK;
>>> +    if (fdctrl->drives[value & FD_DOR_SELMASK].blk) {
>>> +        fdctrl->cur_drv = value & FD_DOR_SELMASK;
>>> +    }
>>>       fdctrl->dor = value;
>>>   }
>>> @@ -1894,6 +1896,10 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
>>>       uint32_t pos;
>>>       cur_drv = get_cur_drv(fdctrl);
>>> +    if (!cur_drv->blk) {
>>> +        FLOPPY_DPRINTF("No drive connected\n");
>>> +        return 0;
>>> +    }
>>>       fdctrl->dsr &= ~FD_DSR_PWRDOWN;
>>>       if (!(fdctrl->msr & FD_MSR_RQM) || !(fdctrl->msr & FD_MSR_DIO)) {
>>>           FLOPPY_DPRINTF("error: controller not ready for reading\n");
>>> @@ -2420,7 +2426,8 @@ static void fdctrl_write_data(FDCtrl *fdctrl,
>>> uint32_t value)
>>>           if (pos == FD_SECTOR_LEN - 1 ||
>>>               fdctrl->data_pos == fdctrl->data_len) {
>>>               cur_drv = get_cur_drv(fdctrl);
>>> -            if (blk_pwrite(cur_drv->blk, fd_offset(cur_drv),
>>> fdctrl->fifo,
>>> +            if (cur_drv->blk == NULL
>>> +                || blk_pwrite(cur_drv->blk, fd_offset(cur_drv),
>>> fdctrl->fifo,
>>>                              BDRV_SECTOR_SIZE, 0) < 0) {
>>>                   FLOPPY_DPRINTF("error writing sector %d\n",
>>>                                  fd_sector(cur_drv));
>>>
>>
>> Ping again!
>>
>> Could anybody review / pick this up?

This patch misses the qtest companion with the reproducer
provided by Alexander.

> Yep. Not forgotten, despite appearances. Clearing my Python review
> backlog, then onto FDC/IDE.

Yeah \o/

> 
> In the meantime, anything anyone else happens to feel comfortable
> staging won't upset me any. I don't insist they go through my tree right
> now.