On 14.03.2023 20:23:47, Oliver Hartkopp wrote: > > > On 14.03.23 16:37, Ivan Orlov wrote: > > On 3/14/23 18:38, Oliver Hartkopp wrote: > > > Hello Ivan, > > > > > > besides the fact that we would read some uninitialized value the > > > outcome of the original implementation would have been an error and > > > a termination of the copy process too. Maybe throwing a different > > > error number. > > > > > > But it is really interesting to see what KMSAN is able to detect > > > these days! Many thanks for the finding and your effort to > > > contribute this fix! > > > > > > Best regards, > > > Oliver > > > > > > > > > On 14.03.23 13:04, Ivan Orlov wrote: > > > > Syzkaller reported the following issue: > > > > > > (..) > > > > > > > > > > > Reported-by: syzbot+c9bfd85eca611ebf5db1@xxxxxxxxxxxxxxxxxxxxxxxxx > > > > Link: https://syzkaller.appspot.com/bug?id=47f897f8ad958bbde5790ebf389b5e7e0a345089 > > > > Signed-off-by: Ivan Orlov <ivan.orlov0322@xxxxxxxxx> > > > > > > Acked-by: Oliver Hartkopp <socketcan@xxxxxxxxxxxx> > > > > > > > > > > --- > > > > net/can/bcm.c | 16 ++++++++++------ > > > > 1 file changed, 10 insertions(+), 6 deletions(-) > > > > > > > > diff --git a/net/can/bcm.c b/net/can/bcm.c > > > > index 27706f6ace34..a962ec2b8ba5 100644 > > > > --- a/net/can/bcm.c > > > > +++ b/net/can/bcm.c > > > > @@ -941,6 +941,8 @@ static int bcm_tx_setup(struct bcm_msg_head > > > > *msg_head, struct msghdr *msg, > > > > cf = op->frames + op->cfsiz * i; > > > > err = memcpy_from_msg((u8 *)cf, msg, op->cfsiz); > > > > + if (err < 0) > > > > + goto free_op; > > > > if (op->flags & CAN_FD_FRAME) { > > > > if (cf->len > 64) > > > > @@ -950,12 +952,8 @@ static int bcm_tx_setup(struct bcm_msg_head > > > > *msg_head, struct msghdr *msg, > > > > err = -EINVAL; > > > > } > > > > - if (err < 0) { > > > > - if (op->frames != &op->sframe) > > > > - kfree(op->frames); > > > > - kfree(op); > > > > - return err; > > > > - } > > > > + if (err < 0) > > > > + goto free_op; > > > > if (msg_head->flags & TX_CP_CAN_ID) { > > > > /* copy can_id into frame */ > > > > @@ -1026,6 +1024,12 @@ static int bcm_tx_setup(struct > > > > bcm_msg_head *msg_head, struct msghdr *msg, > > > > bcm_tx_start_timer(op); > > > > return msg_head->nframes * op->cfsiz + MHSIZ; > > > > + > > > > +free_op: > > > > + if (op->frames != &op->sframe) > > > > + kfree(op->frames); > > > > + kfree(op); > > > > + return err; > > > > } > > > > /* > > > > Thank you for the quick answer! I totally agree that this patch will not > > change the behavior a lot. However, I think a little bit more error > > processing will not be bad (considering this will not bring any > > performance overhead). If someone in the future tries to use the "cf" > > object right after "memcpy_from_msg" call without proper error > > processing it will lead to a bug (which will be hard to trigger). Maybe > > fixing it now to avoid possible future mistakes in the future makes > > sense? > > Yes! Definitely! > > Therefore I added my Acked-by: tag. Marc will likely pick this patch for > upstream. Can you create a proper Fixes tag? Marc -- Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Attachment:
signature.asc
Description: PGP signature
