All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] hw/net/can: sja1000 fix buff2frame_bas for dlc out of std CAN 8 bytes
@ 2021-07-26 16:24 Pavel Pisa
  2021-07-29  9:19 ` Pavel Pisa
  0 siblings, 1 reply; 4+ messages in thread
From: Pavel Pisa @ 2021-07-26 16:24 UTC (permalink / raw)
  To: qemu-devel, Paolo Bonzini, Jason Wang
  Cc: Vikram Garhwal, Qiang Ning, Jiri Novak, Oliver Hartkopp,
	Jin-Yang, Ondrej Ille, Jan Charvat, Jan Kiszka, 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



^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] hw/net/can: sja1000 fix buff2frame_bas for dlc out of std CAN 8 bytes
  2021-07-26 16:24 [PATCH] hw/net/can: sja1000 fix buff2frame_bas for dlc out of std CAN 8 bytes Pavel Pisa
@ 2021-07-29  9:19 ` Pavel Pisa
  2021-07-29 10:03   ` [PATCH-for-6.1] " Philippe Mathieu-Daudé
  0 siblings, 1 reply; 4+ messages in thread
From: Pavel Pisa @ 2021-07-29  9:19 UTC (permalink / raw)
  To: Paolo Bonzini, Jason Wang, Qiang Ning; +Cc: qemu-devel

[-- 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 --]

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH-for-6.1] hw/net/can: sja1000 fix buff2frame_bas for dlc out of std CAN 8 bytes
  2021-07-29  9:19 ` Pavel Pisa
@ 2021-07-29 10:03   ` Philippe Mathieu-Daudé
  2021-07-29 11:52     ` Pavel Pisa
  0 siblings, 1 reply; 4+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-07-29 10:03 UTC (permalink / raw)
  To: Pavel Pisa, Paolo Bonzini, Jason Wang, Qiang Ning
  Cc: Vikram Garhwal, Darren Kenny, Alexander Bulekov, qemu-devel,
	Francisco Iglesias, Bandan Das

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];
>>      }



^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH-for-6.1] hw/net/can: sja1000 fix buff2frame_bas for dlc out of std CAN 8 bytes
  2021-07-29 10:03   ` [PATCH-for-6.1] " Philippe Mathieu-Daudé
@ 2021-07-29 11:52     ` Pavel Pisa
  0 siblings, 0 replies; 4+ messages in thread
From: Pavel Pisa @ 2021-07-29 11:52 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Francisco Iglesias, Vikram Garhwal, Qiang Ning, Darren Kenny,
	Jason Wang, qemu-devel, Alexander Bulekov, Bandan Das,
	Paolo Bonzini

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/



^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-07-29 12:05 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-26 16:24 [PATCH] hw/net/can: sja1000 fix buff2frame_bas for dlc out of std CAN 8 bytes Pavel Pisa
2021-07-29  9:19 ` Pavel Pisa
2021-07-29 10:03   ` [PATCH-for-6.1] " Philippe Mathieu-Daudé
2021-07-29 11:52     ` Pavel Pisa

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.