[PATCH] hw/net/can: sja1000 fix buff2frame_bas for dlc out of std CAN 8 bytes

[PATCH] hw/net/can: sja1000 fix buff2frame_bas for dlc out of std CAN 8 bytes

[Pavel Pisa]

Problem reported by openEuler fuzz-sig group.

The buff2frame_bas function (hw\net\can\can_sja1000.c)
infoleak(qemu5.x~qemu6.x) or stack-overflow(qemu 4.x).

Reported-by: Qiang Ning <ningqiang1@huawei.com>
Signed-off-by: Pavel Pisa <pisa@cmp.felk.cvut.cz>
---
 hw/net/can/can_sja1000.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/net/can/can_sja1000.c b/hw/net/can/can_sja1000.c
index 42d2f99dfb..64e81bff58 100644
--- a/hw/net/can/can_sja1000.c
+++ b/hw/net/can/can_sja1000.c
@@ -311,6 +311,10 @@ static void buff2frame_bas(const uint8_t *buff, qemu_can_frame *frame)
     }
     frame->can_dlc = buff[1] & 0x0f;
 
+    if (frame->can_dlc > 8) {
+        frame->can_dlc = 8;
+    }
+
     for (i = 0; i < frame->can_dlc; i++) {
         frame->data[i] = buff[2 + i];
     }
-- 
2.20.1

Re: [PATCH] hw/net/can: sja1000 fix buff2frame_bas for dlc out of std CAN 8 bytes

[Pavel Pisa]

[-- Attachment #1: Type: text/plain, Size: 1090 bytes --]

Hello everybody,

please, can somebody accept the fix for master?
It should be ideally applied even to stable
branches.

Or should I send request through some other form
then on the list?

Thanks,

Pavel

On Monday 26 of July 2021 18:24:58 Pavel Pisa wrote:
> Problem reported by openEuler fuzz-sig group.
>
> The buff2frame_bas function (hw\net\can\can_sja1000.c)
> infoleak(qemu5.x~qemu6.x) or stack-overflow(qemu 4.x).
>
> Reported-by: Qiang Ning <ningqiang1@huawei.com>
> Signed-off-by: Pavel Pisa <pisa@cmp.felk.cvut.cz>
> ---
>  hw/net/can/can_sja1000.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/hw/net/can/can_sja1000.c b/hw/net/can/can_sja1000.c
> index 42d2f99dfb..64e81bff58 100644
> --- a/hw/net/can/can_sja1000.c
> +++ b/hw/net/can/can_sja1000.c
> @@ -311,6 +311,10 @@ static void buff2frame_bas(const uint8_t *buff,
> qemu_can_frame *frame) }
>      frame->can_dlc = buff[1] & 0x0f;
>
> +    if (frame->can_dlc > 8) {
> +        frame->can_dlc = 8;
> +    }
> +
>      for (i = 0; i < frame->can_dlc; i++) {
>          frame->data[i] = buff[2 + i];
>      }

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH-for-6.1] hw/net/can: sja1000 fix buff2frame_bas for dlc out of std CAN 8 bytes

[Philippe Mathieu-Daudé]

Cc'ing QEMU fuzz team.

On 7/29/21 11:19 AM, Pavel Pisa wrote:
> Hello everybody,
> 
> please, can somebody accept the fix for master?
> It should be ideally applied even to stable
> branches.
> 
> Or should I send request through some other form
> then on the list?

I suppose the patch fell through the cracks.

Apparently Paolo doesn't like to queue fuzzer fixes without
reproducer. For examples see tests/qtest/fuzz-*.c in the tree.

> 
> Thanks,
> 
> Pavel
> 
> On Monday 26 of July 2021 18:24:58 Pavel Pisa wrote:
>> Problem reported by openEuler fuzz-sig group.
>>
>> The buff2frame_bas function (hw\net\can\can_sja1000.c)
>> infoleak(qemu5.x~qemu6.x) or stack-overflow(qemu 4.x).
>>

If you want the patch backported in stable releases, please
include:

Cc: qemu-stable@nongnu.org

>> Reported-by: Qiang Ning <ningqiang1@huawei.com>
>> Signed-off-by: Pavel Pisa <pisa@cmp.felk.cvut.cz>
>> ---
>>  hw/net/can/can_sja1000.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/hw/net/can/can_sja1000.c b/hw/net/can/can_sja1000.c
>> index 42d2f99dfb..64e81bff58 100644
>> --- a/hw/net/can/can_sja1000.c
>> +++ b/hw/net/can/can_sja1000.c
>> @@ -311,6 +311,10 @@ static void buff2frame_bas(const uint8_t *buff,
>> qemu_can_frame *frame) }
>>      frame->can_dlc = buff[1] & 0x0f;
>>
>> +    if (frame->can_dlc > 8) {
>> +        frame->can_dlc = 8;
>> +    }
>> +

This doesn't seem a complete fix (see buff2frame_pel).

Here can_dlc shouldn't be more than 8.

What you can do here (and in buff2frame_pel) is:

        assert(frame->can_dlc <= 8);

and find where the field is abused, probably discarding
invalid frames earlier?

>>      for (i = 0; i < frame->can_dlc; i++) {
>>          frame->data[i] = buff[2 + i];
>>      }

Re: [PATCH-for-6.1] hw/net/can: sja1000 fix buff2frame_bas for dlc out of std CAN 8 bytes

[Pavel Pisa]

Hello Philippe,

thanks for the reply.

On Thursday 29 of July 2021 12:03:00 Philippe Mathieu-Daudé wrote:
> I suppose the patch fell through the cracks.
>
> Apparently Paolo doesn't like to queue fuzzer fixes without
> reproducer. For examples see tests/qtest/fuzz-*.c in the tree.

I can try to find how to build required fuzz test or fuzz
team has some code availabe, may it be in the required form.
But the fix is from SJA1000 CAN frame/chip definition.

> > On Monday 26 of July 2021 18:24:58 Pavel Pisa wrote:
> >> Problem reported by openEuler fuzz-sig group.
> >>
> >> The buff2frame_bas function (hw\net\can\can_sja1000.c)
> >> infoleak(qemu5.x~qemu6.x) or stack-overflow(qemu 4.x).
>
> If you want the patch backported in stable releases, please
> include:
>
> Cc: qemu-stable@nongnu.org

OK, I will send updated version there.

> >> Reported-by: Qiang Ning <ningqiang1@huawei.com>
> >> Signed-off-by: Pavel Pisa <pisa@cmp.felk.cvut.cz>
> >> ---
> >>  hw/net/can/can_sja1000.c | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/hw/net/can/can_sja1000.c b/hw/net/can/can_sja1000.c
> >> index 42d2f99dfb..64e81bff58 100644
> >> --- a/hw/net/can/can_sja1000.c
> >> +++ b/hw/net/can/can_sja1000.c
> >> @@ -311,6 +311,10 @@ static void buff2frame_bas(const uint8_t *buff,
> >> qemu_can_frame *frame) }
> >>      frame->can_dlc = buff[1] & 0x0f;
> >>
> >> +    if (frame->can_dlc > 8) {
> >> +        frame->can_dlc = 8;
> >> +    }
> >> +
>
> This doesn't seem a complete fix (see buff2frame_pel).

Thanks I have overlooked that. I will send updated
version.

> Here can_dlc shouldn't be more than 8.
>
> What you can do here (and in buff2frame_pel) is:
>
>         assert(frame->can_dlc <= 8);
>
> and find where the field is abused, probably discarding
> invalid frames earlier?

I do not think that it is right to put assert there
and kill whole virtual machine.

The value source is write to the register by guest OS
kernel, driver. It can be intentional or unintentional
case, but problem local to the guest. I can add report/
logging of the problem.

I have not checked what happens on the real SJA1000
chip if DLC is written out of range. I can try to test
that. But generally that falls under undefined behavior
of the chip. But even in this case the chip should prevent
disruption of whole CAN bus/link so I expect that it
silently limits length to 8 bytes.

Best wishes,

                Pavel
--
                Pavel Pisa
    e-mail:     pisa@cmp.felk.cvut.cz
    Department of Control Engineering FEE CVUT
    Karlovo namesti 13, 121 35, Prague 2
    university: http://dce.fel.cvut.cz/
    personal:   http://cmp.felk.cvut.cz/~pisa
    projects:   https://www.openhub.net/accounts/ppisa
    CAN related:http://canbus.pages.fel.cvut.cz/