linux-kernel - Re: [PATCH] media: dvb_ringbuffer: Return -EFAULT if copy fails

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aab14cc4-0400-c114-f180-6db943fd7e32@nfschina.com>
Date:   Wed, 24 May 2023 13:20:27 +0800
From:   Su Hui <suhui@...china.com>
To:     Dan Carpenter <dan.carpenter@...aro.org>
Cc:     Mauro Carvalho Chehab <mchehab@...nel.org>,
        YongSu Yoo <yongsuyoo0215@...il.com>,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
        kernel-janitors@...r.kernel.org
Subject: Re: [PATCH] media: dvb_ringbuffer: Return -EFAULT if copy fails


On 2023/5/24 12:47, Dan Carpenter wrote:
> On Wed, May 24, 2023 at 07:23:45AM +0300, Dan Carpenter wrote:
>> On Wed, May 24, 2023 at 09:27:33AM +0800, Su Hui wrote:
>>> The copy_to/from_user() functions return the number of bytes remaining
>>> to be copied, but we want to return -EFAULT to the user.
>>>
> So basically these bugs are caused because people are used to functions
> returning negative error codes and they write some form of:
>
> 	ret = copy_from_user();
> 	if (ret)
> 		return ret;
>
> If you look at the code and you think, "They author thinks 'ret' is
> negative" then probably it is a bug.  The common false positives are
> in the core kernel where it does:
>
> 	return copy_from_user();
>
> and the caller checks:
>
> 	if (function_one() || function_two() || function_three())
> 		return -EFAULT;
>
> Those are done because it's a fast path and adding a lot of if
> statements would slow things down.  Driver code tends not to do this
> because normally drivers are not so performance sensitive and it's more
> important to be readable.
>
> So you have to look at the context a bit.

Thanks very much for your suggestion.

It's confusing about the comment on function declaration.

     /**
      * dvb_ringbuffer_write_user - Writes a buffer received via a user 
pointer

     ..........

      * Return: number of bytes transferred or -EFAULT

But the function Only returns  the number of bytes transferred.

Maybe the comment should be modified because it never returns -EFAULT.

I will look at the context more carefully next time , thanks again!

Su Hui

> regards,
> dan carpenter
>