Messages in this thread | | | Date | Mon, 20 Mar 2023 21:22:29 +0100 | Subject | Re: [PATCH] FS, NET: Fix KMSAN uninit-value in vfs_write | From | Oliver Hartkopp <> |
| |
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@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. > > 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
| |