[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <b4abefa2-16d0-a18c-4614-1786eb94ffab@hartkopp.net>
Date: Tue, 14 Mar 2023 20:23:47 +0100
From: Oliver Hartkopp <socketcan@...tkopp.net>
To: Ivan Orlov <ivan.orlov0322@...il.com>, mkl@...gutronix.de,
davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
pabeni@...hat.com
Cc: linux-can@...r.kernel.org, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, himadrispandya@...il.com,
skhan@...uxfoundation.org,
syzbot+c9bfd85eca611ebf5db1@...kaller.appspotmail.com
Subject: Re: [PATCH] FS, NET: Fix KMSAN uninit-value in vfs_write
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@...kaller.appspotmail.com
>>> Link:
>>> https://syzkaller.appspot.com/bug?id=47f897f8ad958bbde5790ebf389b5e7e0a345089
>>> Signed-off-by: Ivan Orlov <ivan.orlov0322@...il.com>
>>
>> Acked-by: Oliver Hartkopp <socketcan@...tkopp.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
Powered by blists - more mailing lists