Re: [PATCH] FS, NET: Fix KMSAN uninit-value in vfs_write - Marc Kleine-Budde

From: Marc Kleine-Budde <mkl@pengutronix.de>
To: Oliver Hartkopp <socketcan@hartkopp.net>
Cc: Ivan Orlov <ivan.orlov0322@gmail.com>,
	davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
	pabeni@redhat.com, linux-can@vger.kernel.org,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	himadrispandya@gmail.com, skhan@linuxfoundation.org,
	syzbot+c9bfd85eca611ebf5db1@syzkaller.appspotmail.com
Subject: Re: [PATCH] FS, NET: Fix KMSAN uninit-value in vfs_write
Date: Mon, 20 Mar 2023 16:40:53 +0100	[thread overview]
Message-ID: <20230320154053.x3h54b2s3r7iclby@pengutronix.de> (raw)
In-Reply-To: <b4abefa2-16d0-a18c-4614-1786eb94ffab@hartkopp.net>

[-- Attachment #1: Type: text/plain, Size: 4012 bytes --]

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?

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]