On 20.03.23 16:40, Marc Kleine-Budde wrote:
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?
Fixes: 6f3b911d5f29b ("can: bcm: add support for CAN FD frames")
Btw. do you really think this is a candidate for stable?
IMHO it is just a KMSAN hit that should be fixed for future releases ...
Best regards,
Oliver
