fdc: check for illegal dma length calculation

[PATCH] fdc: check for illegal dma length calculation

Posted by Jon Maloy 2 years, 3 months ago

The function fdctrl_start_transfer() calculates the dma data length
wrongly when certain boundary conditions are fulfilled. We have
noticed that the if ((fdctrl->fifo[5] - fdctrl->fifo[6]) > 1) we get
a dma length that will be interpreted as negative by the next function
in the chain, fdctrl_transfer_handler(). This leads to a crash.

Rather than trying to fix this obscure calculation, we just check if
the harmful condition is fulfilled, and return without action if that
is the case. Since this is a condition that can only be created by a
malicious user we deem this solution safe.

This fix is intended to address CVE-2021-3507.

Signed-off-by: Jon Maloy <jmaloy@redhat.com>
---
 hw/block/fdc.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 21d18ac2e3..80a1f1750a 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -1532,6 +1532,11 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl, int direction)
         if (fdctrl->fifo[0] & 0x80)
             tmp += fdctrl->fifo[6];
         fdctrl->data_len *= tmp;
+        if (tmp < 0) {
+            FLOPPY_DPRINTF("calculated illegal data_len=%u, tmp=%i\n",
+                           fdctrl->data_len, tmp);
+            return;
+        }
     }
     fdctrl->eot = fdctrl->fifo[6];
     if (fdctrl->dor & FD_DOR_DMAEN) {
-- 
2.31.1

Re: [PATCH] fdc: check for illegal dma length calculation

Posted by Jon Maloy 2 years, 3 months ago

On 1/13/22 20:33, Jon Maloy wrote:
> The function fdctrl_start_transfer() calculates the dma data length
> wrongly when certain boundary conditions are fulfilled. We have
> noticed that the if ((fdctrl->fifo[5] - fdctrl->fifo[6]) > 1) we get
> a dma length that will be interpreted as negative by the next function
> in the chain, fdctrl_transfer_handler(). This leads to a crash.
>
> Rather than trying to fix this obscure calculation, we just check if
> the harmful condition is fulfilled, and return without action if that
> is the case. Since this is a condition that can only be created by a
> malicious user we deem this solution safe.
>
> This fix is intended to address CVE-2021-3507.
>
> Signed-off-by: Jon Maloy <jmaloy@redhat.com>
> ---
>   hw/block/fdc.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index 21d18ac2e3..80a1f1750a 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -1532,6 +1532,11 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl, int direction)
>           if (fdctrl->fifo[0] & 0x80)
>               tmp += fdctrl->fifo[6];
>           fdctrl->data_len *= tmp;
> +        if (tmp < 0) {
> +            FLOPPY_DPRINTF("calculated illegal data_len=%u, tmp=%i\n",
> +                           fdctrl->data_len, tmp);
> +            return;
> +        }
>       }
>       fdctrl->eot = fdctrl->fifo[6];
>       if (fdctrl->dor & FD_DOR_DMAEN) {
I never received any feedback on this one.
Is there any?

///jon

Re: [PATCH] fdc: check for illegal dma length calculation

Posted by Philippe Mathieu-Daudé via 2 years, 3 months ago

Hi Jon,

On 26/1/22 16:44, Jon Maloy wrote:
> On 1/13/22 20:33, Jon Maloy wrote:
>> The function fdctrl_start_transfer() calculates the dma data length
>> wrongly when certain boundary conditions are fulfilled. We have
>> noticed that the if ((fdctrl->fifo[5] - fdctrl->fifo[6]) > 1) we get
>> a dma length that will be interpreted as negative by the next function
>> in the chain, fdctrl_transfer_handler(). This leads to a crash.

If a security issue is reproducible (like the ones found by fuzzers),
please include the reproducer along.

>> Rather than trying to fix this obscure calculation, we just check if
>> the harmful condition is fulfilled, and return without action if that
>> is the case. Since this is a condition that can only be created by a
>> malicious user we deem this solution safe.
>>
>> This fix is intended to address CVE-2021-3507.

Ah, this is similar to:
https://lore.kernel.org/qemu-devel/20211118115733.4038610-1-philmd@redhat.com/
which already contains the reproducer...
You might want to review it ;)

>> Signed-off-by: Jon Maloy <jmaloy@redhat.com>
>> ---
>>   hw/block/fdc.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>> index 21d18ac2e3..80a1f1750a 100644
>> --- a/hw/block/fdc.c
>> +++ b/hw/block/fdc.c
>> @@ -1532,6 +1532,11 @@ static void fdctrl_start_transfer(FDCtrl 
>> *fdctrl, int direction)
>>           if (fdctrl->fifo[0] & 0x80)
>>               tmp += fdctrl->fifo[6];
>>           fdctrl->data_len *= tmp;
>> +        if (tmp < 0) {
>> +            FLOPPY_DPRINTF("calculated illegal data_len=%u, tmp=%i\n",
>> +                           fdctrl->data_len, tmp);
>> +            return;
>> +        }
>>       }
>>       fdctrl->eot = fdctrl->fifo[6];
>>       if (fdctrl->dor & FD_DOR_DMAEN) {
> I never received any feedback on this one.
> Is there any?

Probably none so far because you forgot to Cc the maintainers:

$ ./scripts/get_maintainer.pl -f hw/block/fdc.c
John Snow <jsnow@redhat.com> (supporter:Floppy)
Kevin Wolf <kwolf@redhat.com> (supporter:Block layer core)
Hanna Reitz <hreitz@redhat.com> (supporter:Block layer core)
qemu-block@nongnu.org (open list:Floppy)
qemu-devel@nongnu.org (open list:All patches CC here)

(now Cc'ed).