| Message ID | 20210726162458.26197-1-pisa@cmp.felk.cvut.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/net/can: sja1000 fix buff2frame_bas for dlc out of std CAN 8 bytes | expand |
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]; > }
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]; >> }
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/
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]; }
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(+)