Message-ID: <ff0a4ed4-9fde-7a9f-da39-d799dfb946f1@gmail.com>
Date:   Tue, 14 Mar 2023 19:37:00 +0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH] FS, NET: Fix KMSAN uninit-value in vfs_write
Content-Language: en-US
To:     Oliver Hartkopp <socketcan@hartkopp.net>, mkl@pengutronix.de,
        davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        pabeni@redhat.com
Cc:     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>
From:   Ivan Orlov <ivan.orlov0322@gmail.com>
In-Reply-To: <0e7090c4-ca9b-156f-5922-fd7ddb55fee4@hartkopp.net>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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?