linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] FS, NET: Fix KMSAN uninit-value in vfs_write
@ 2023-03-14 12:04 Ivan Orlov
  2023-03-14 14:38 ` Oliver Hartkopp
  2023-03-24 17:33 ` Marc Kleine-Budde
  0 siblings, 2 replies; 7+ messages in thread
From: Ivan Orlov @ 2023-03-14 12:04 UTC (permalink / raw)
  To: socketcan, mkl, davem, edumazet, kuba, pabeni
  Cc: Ivan Orlov, linux-can, netdev, linux-kernel, himadrispandya,
	skhan, syzbot+c9bfd85eca611ebf5db1

Syzkaller reported the following issue:

=====================================================
BUG: KMSAN: uninit-value in aio_rw_done fs/aio.c:1520 [inline]
BUG: KMSAN: uninit-value in aio_write+0x899/0x950 fs/aio.c:1600
 aio_rw_done fs/aio.c:1520 [inline]
 aio_write+0x899/0x950 fs/aio.c:1600
 io_submit_one+0x1d1c/0x3bf0 fs/aio.c:2019
 __do_sys_io_submit fs/aio.c:2078 [inline]
 __se_sys_io_submit+0x293/0x770 fs/aio.c:2048
 __x64_sys_io_submit+0x92/0xd0 fs/aio.c:2048
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x3d/0xb0 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x63/0xcd

Uninit was created at:
 slab_post_alloc_hook mm/slab.h:766 [inline]
 slab_alloc_node mm/slub.c:3452 [inline]
 __kmem_cache_alloc_node+0x71f/0xce0 mm/slub.c:3491
 __do_kmalloc_node mm/slab_common.c:967 [inline]
 __kmalloc+0x11d/0x3b0 mm/slab_common.c:981
 kmalloc_array include/linux/slab.h:636 [inline]
 bcm_tx_setup+0x80e/0x29d0 net/can/bcm.c:930
 bcm_sendmsg+0x3a2/0xce0 net/can/bcm.c:1351
 sock_sendmsg_nosec net/socket.c:714 [inline]
 sock_sendmsg net/socket.c:734 [inline]
 sock_write_iter+0x495/0x5e0 net/socket.c:1108
 call_write_iter include/linux/fs.h:2189 [inline]
 aio_write+0x63a/0x950 fs/aio.c:1600
 io_submit_one+0x1d1c/0x3bf0 fs/aio.c:2019
 __do_sys_io_submit fs/aio.c:2078 [inline]
 __se_sys_io_submit+0x293/0x770 fs/aio.c:2048
 __x64_sys_io_submit+0x92/0xd0 fs/aio.c:2048
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x3d/0xb0 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x63/0xcd

CPU: 1 PID: 5034 Comm: syz-executor350 Not tainted 6.2.0-rc6-syzkaller-80422-geda666ff2276 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/12/2023
=====================================================

We can follow the call chain and find that 'bcm_tx_setup' function calls 'memcpy_from_msg'
to copy some content to the newly allocated frame of 'op->frames'. After that the 'len'
field of copied structure being compared with some constant value (64 or 8). However, if
'memcpy_from_msg' returns an error, we will compare some uninitialized memory. This triggers
'uninit-value' issue.

This patch will add 'memcpy_from_msg' possible errors processing to avoid uninit-value
issue.

Tested via syzkaller

Reported-by: syzbot+c9bfd85eca611ebf5db1@syzkaller.appspotmail.com
Link: https://syzkaller.appspot.com/bug?id=47f897f8ad958bbde5790ebf389b5e7e0a345089
Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
---
 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;
 }
 
 /*
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] FS, NET: Fix KMSAN uninit-value in vfs_write
  2023-03-14 12:04 [PATCH] FS, NET: Fix KMSAN uninit-value in vfs_write Ivan Orlov
@ 2023-03-14 14:38 ` Oliver Hartkopp
  2023-03-14 15:37   ` Ivan Orlov
  2023-03-24 17:33 ` Marc Kleine-Budde
  1 sibling, 1 reply; 7+ messages in thread
From: Oliver Hartkopp @ 2023-03-14 14:38 UTC (permalink / raw)
  To: Ivan Orlov, mkl, davem, edumazet, kuba, pabeni
  Cc: linux-can, netdev, linux-kernel, himadrispandya, skhan,
	syzbot+c9bfd85eca611ebf5db1

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;
>   }
>   
>   /*

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] FS, NET: Fix KMSAN uninit-value in vfs_write
  2023-03-14 14:38 ` Oliver Hartkopp
@ 2023-03-14 15:37   ` Ivan Orlov
  2023-03-14 19:23     ` Oliver Hartkopp
  0 siblings, 1 reply; 7+ messages in thread
From: Ivan Orlov @ 2023-03-14 15:37 UTC (permalink / raw)
  To: Oliver Hartkopp, mkl, davem, edumazet, kuba, pabeni
  Cc: linux-can, netdev, linux-kernel, himadrispandya, skhan,
	syzbot+c9bfd85eca611ebf5db1

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?

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] FS, NET: Fix KMSAN uninit-value in vfs_write
  2023-03-14 15:37   ` Ivan Orlov
@ 2023-03-14 19:23     ` Oliver Hartkopp
  2023-03-20 15:40       ` Marc Kleine-Budde
  0 siblings, 1 reply; 7+ messages in thread
From: Oliver Hartkopp @ 2023-03-14 19:23 UTC (permalink / raw)
  To: Ivan Orlov, mkl, davem, edumazet, kuba, pabeni
  Cc: linux-can, netdev, linux-kernel, himadrispandya, skhan,
	syzbot+c9bfd85eca611ebf5db1



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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] FS, NET: Fix KMSAN uninit-value in vfs_write
  2023-03-14 19:23     ` Oliver Hartkopp
@ 2023-03-20 15:40       ` Marc Kleine-Budde
  2023-03-20 20:22         ` Oliver Hartkopp
  0 siblings, 1 reply; 7+ messages in thread
From: Marc Kleine-Budde @ 2023-03-20 15:40 UTC (permalink / raw)
  To: Oliver Hartkopp
  Cc: Ivan Orlov, davem, edumazet, kuba, pabeni, linux-can, netdev,
	linux-kernel, himadrispandya, skhan, syzbot+c9bfd85eca611ebf5db1

[-- 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 --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] FS, NET: Fix KMSAN uninit-value in vfs_write
  2023-03-20 15:40       ` Marc Kleine-Budde
@ 2023-03-20 20:22         ` Oliver Hartkopp
  0 siblings, 0 replies; 7+ messages in thread
From: Oliver Hartkopp @ 2023-03-20 20:22 UTC (permalink / raw)
  To: Marc Kleine-Budde
  Cc: Ivan Orlov, davem, edumazet, kuba, pabeni, linux-can, netdev,
	linux-kernel, himadrispandya, skhan, syzbot+c9bfd85eca611ebf5db1



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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] FS, NET: Fix KMSAN uninit-value in vfs_write
  2023-03-14 12:04 [PATCH] FS, NET: Fix KMSAN uninit-value in vfs_write Ivan Orlov
  2023-03-14 14:38 ` Oliver Hartkopp
@ 2023-03-24 17:33 ` Marc Kleine-Budde
  1 sibling, 0 replies; 7+ messages in thread
From: Marc Kleine-Budde @ 2023-03-24 17:33 UTC (permalink / raw)
  To: Ivan Orlov
  Cc: socketcan, davem, edumazet, kuba, pabeni, linux-can, netdev,
	linux-kernel, himadrispandya, skhan, syzbot+c9bfd85eca611ebf5db1

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

On 14.03.2023 16:04:45, Ivan Orlov wrote:
> Syzkaller reported the following issue:
> 
> =====================================================
> BUG: KMSAN: uninit-value in aio_rw_done fs/aio.c:1520 [inline]
> BUG: KMSAN: uninit-value in aio_write+0x899/0x950 fs/aio.c:1600
>  aio_rw_done fs/aio.c:1520 [inline]
>  aio_write+0x899/0x950 fs/aio.c:1600
>  io_submit_one+0x1d1c/0x3bf0 fs/aio.c:2019
>  __do_sys_io_submit fs/aio.c:2078 [inline]
>  __se_sys_io_submit+0x293/0x770 fs/aio.c:2048
>  __x64_sys_io_submit+0x92/0xd0 fs/aio.c:2048
>  do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>  do_syscall_64+0x3d/0xb0 arch/x86/entry/common.c:80
>  entry_SYSCALL_64_after_hwframe+0x63/0xcd
> 
> Uninit was created at:
>  slab_post_alloc_hook mm/slab.h:766 [inline]
>  slab_alloc_node mm/slub.c:3452 [inline]
>  __kmem_cache_alloc_node+0x71f/0xce0 mm/slub.c:3491
>  __do_kmalloc_node mm/slab_common.c:967 [inline]
>  __kmalloc+0x11d/0x3b0 mm/slab_common.c:981
>  kmalloc_array include/linux/slab.h:636 [inline]
>  bcm_tx_setup+0x80e/0x29d0 net/can/bcm.c:930
>  bcm_sendmsg+0x3a2/0xce0 net/can/bcm.c:1351
>  sock_sendmsg_nosec net/socket.c:714 [inline]
>  sock_sendmsg net/socket.c:734 [inline]
>  sock_write_iter+0x495/0x5e0 net/socket.c:1108
>  call_write_iter include/linux/fs.h:2189 [inline]
>  aio_write+0x63a/0x950 fs/aio.c:1600
>  io_submit_one+0x1d1c/0x3bf0 fs/aio.c:2019
>  __do_sys_io_submit fs/aio.c:2078 [inline]
>  __se_sys_io_submit+0x293/0x770 fs/aio.c:2048
>  __x64_sys_io_submit+0x92/0xd0 fs/aio.c:2048
>  do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>  do_syscall_64+0x3d/0xb0 arch/x86/entry/common.c:80
>  entry_SYSCALL_64_after_hwframe+0x63/0xcd
> 
> CPU: 1 PID: 5034 Comm: syz-executor350 Not tainted 6.2.0-rc6-syzkaller-80422-geda666ff2276 #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/12/2023
> =====================================================
> 
> We can follow the call chain and find that 'bcm_tx_setup' function calls 'memcpy_from_msg'
> to copy some content to the newly allocated frame of 'op->frames'. After that the 'len'
> field of copied structure being compared with some constant value (64 or 8). However, if
> 'memcpy_from_msg' returns an error, we will compare some uninitialized memory. This triggers
> 'uninit-value' issue.
> 
> This patch will add 'memcpy_from_msg' possible errors processing to avoid uninit-value
> issue.
> 
> Tested via syzkaller
> 
> Reported-by: syzbot+c9bfd85eca611ebf5db1@syzkaller.appspotmail.com
> Link: https://syzkaller.appspot.com/bug?id=47f897f8ad958bbde5790ebf389b5e7e0a345089
> Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>

Applied to linux-can.

Thanks,
Marc

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

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2023-03-24 17:34 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-14 12:04 [PATCH] FS, NET: Fix KMSAN uninit-value in vfs_write Ivan Orlov
2023-03-14 14:38 ` Oliver Hartkopp
2023-03-14 15:37   ` Ivan Orlov
2023-03-14 19:23     ` Oliver Hartkopp
2023-03-20 15:40       ` Marc Kleine-Budde
2023-03-20 20:22         ` Oliver Hartkopp
2023-03-24 17:33 ` Marc Kleine-Budde

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).