Message-ID: <b5c30cb9-f35f-01b1-21b8-4658d46a5c1d@hartkopp.net>
Date:   Mon, 20 Mar 2023 21:22:29 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.0
Subject: Re: [PATCH] FS, NET: Fix KMSAN uninit-value in vfs_write
Content-Language: en-US
To:     Marc Kleine-Budde <mkl@pengutronix.de>
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
References: <20230314120445.12407-1-ivan.orlov0322@gmail.com>
 <0e7090c4-ca9b-156f-5922-fd7ddb55fee4@hartkopp.net>
 <ff0a4ed4-9fde-7a9f-da39-d799dfb946f1@gmail.com>
 <b4abefa2-16d0-a18c-4614-1786eb94ffab@hartkopp.net>
 <20230320154053.x3h54b2s3r7iclby@pengutronix.de>
From:   Oliver Hartkopp <socketcan@hartkopp.net>
In-Reply-To: <20230320154053.x3h54b2s3r7iclby@pengutronix.de>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


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