Messages in this thread | | | Date | Tue, 14 Mar 2023 20:23:47 +0100 | Subject | Re: [PATCH] FS, NET: Fix KMSAN uninit-value in vfs_write | From | Oliver Hartkopp <> |
| |
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@syzkaller.appspotmail.com >>> Link: >>> https://syzkaller.appspot.com/bug?id=47f897f8ad958bbde5790ebf389b5e7e0a345089 >>> Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com> >> >> Acked-by: Oliver Hartkopp <socketcan@hartkopp.net> >> >> >>> --- >>> 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.
Best regards, Oliver
| |