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

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

[Su Hui]

The copy_to/from_user() functions return the number of bytes remaining
to be copied, but we want to return -EFAULT to the user.

Fixes: 04da2daee383 ("[media] ngene: properly handle __user ptr")
Signed-off-by: Su Hui <suhui@nfschina.com>
---
 drivers/media/dvb-core/dvb_ringbuffer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/dvb-core/dvb_ringbuffer.c b/drivers/media/dvb-core/dvb_ringbuffer.c
index 7d4558de8e83..f132578840ad 100644
--- a/drivers/media/dvb-core/dvb_ringbuffer.c
+++ b/drivers/media/dvb-core/dvb_ringbuffer.c
@@ -216,7 +216,7 @@ ssize_t dvb_ringbuffer_write_user(struct dvb_ringbuffer *rbuf,
 	if (split > 0) {
 		status = copy_from_user(rbuf->data+rbuf->pwrite, buf, split);
 		if (status)
-			return len - todo;
+			return -EFAULT;
 		buf += split;
 		todo -= split;
 		/* smp_store_release() for write pointer update to ensure that
@@ -228,7 +228,7 @@ ssize_t dvb_ringbuffer_write_user(struct dvb_ringbuffer *rbuf,
 	}
 	status = copy_from_user(rbuf->data+rbuf->pwrite, buf, todo);
 	if (status)
-		return len - todo;
+		return -EFAULT;
 	/* smp_store_release() for write pointer update, see above */
 	smp_store_release(&rbuf->pwrite, (rbuf->pwrite + todo) % rbuf->size);
 
-- 
2.30.2

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

[Dan Carpenter]

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.
> 
> Fixes: 04da2daee383 ("[media] ngene: properly handle __user ptr")
> Signed-off-by: Su Hui <suhui@nfschina.com>
> ---
>  drivers/media/dvb-core/dvb_ringbuffer.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/dvb-core/dvb_ringbuffer.c b/drivers/media/dvb-core/dvb_ringbuffer.c
> index 7d4558de8e83..f132578840ad 100644
> --- a/drivers/media/dvb-core/dvb_ringbuffer.c
> +++ b/drivers/media/dvb-core/dvb_ringbuffer.c
> @@ -216,7 +216,7 @@ ssize_t dvb_ringbuffer_write_user(struct dvb_ringbuffer *rbuf,
>  	if (split > 0) {
>  		status = copy_from_user(rbuf->data+rbuf->pwrite, buf, split);
>  		if (status)
> -			return len - todo;
> +			return -EFAULT;

No no no.  This is obviously deliberate.  It's returning the number of
bytes that were successfully copied.

(I'm not sure I like this API.  If buf is a bad address then shouldn't
we report that to the user?  It's not like we ran out of space or hit
some limit in the kernel, it's just a bug in the user space program.
However, changing the API is dangerous and could break user space).

Same for the other.

regards,
dan carpenter

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

[Dan Carpenter]

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.

regards,
dan carpenter

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

[Su Hui]

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
>

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

[Dan Carpenter]

On Wed, May 24, 2023 at 01:20:27PM +0800, Su Hui wrote:
> 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.

To be honest, I think that -EFAULT is probably a better return.  But
there is no way we could apply the patch with that commit message.  The
commit message doesn't explain the problem for the user or why returning
the number of bytes copied is not correct in this case.

I think that maybe it's not too late to change this to return -EFAULT,
but it would have been easier to make the change in 2014 before there
were many users.  Also it would be easier if you were testing this on
real hardware.

Possibly other people think the current behavior is correct or that it
is too late to change it.  That's also fine.  I'm not a domain expert
here.

regards,
dan carpenter

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

[Longsuhui]

On 2023/5/24 15:20, Dan Carpenter wrote:
> On Wed, May 24, 2023 at 01:20:27PM +0800, Su Hui wrote:
>> 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.
> To be honest, I think that -EFAULT is probably a better return.  But
> there is no way we could apply the patch with that commit message.  The
> commit message doesn't explain the problem for the user or why returning
> the number of bytes copied is not correct in this case.
>
> I think that maybe it's not too late to change this to return -EFAULT,
> but it would have been easier to make the change in 2014 before there
> were many users.  Also it would be easier if you were testing this on
> real hardware.
>
> Possibly other people think the current behavior is correct or that it
> is too late to change it.  That's also fine.  I'm not a domain expert
> here.
I understand it.
Thanks a lot!

Su Hui

>
> regards,
> dan carpenter
>

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

[Mauro Carvalho Chehab]

Em Wed, 24 May 2023 10:20:38 +0300
Dan Carpenter <dan.carpenter@linaro.org> escreveu:

> On Wed, May 24, 2023 at 01:20:27PM +0800, Su Hui wrote:
> > 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.  
> 
> To be honest, I think that -EFAULT is probably a better return.  But
> there is no way we could apply the patch with that commit message.  The
> commit message doesn't explain the problem for the user or why returning
> the number of bytes copied is not correct in this case.
> 
> I think that maybe it's not too late to change this to return -EFAULT,
> but it would have been easier to make the change in 2014 before there
> were many users.  Also it would be easier if you were testing this on
> real hardware.

It is too late to change the API here, as this could break userspace.

Basically, DVB subsystem normally works with a Kernel-implemented ringbuffer
that transfers MPEG TS data between kernelspace/userspace. The size is
set via an ioctl (DMX_SET_BUFFER_SIZE). By the way, such uAPI is older 
than 2014. It was added upstream on Kernel 2.6.

The buffer size is usually big. For instance, dvbv5-zap uses:

	#define DVB_BUF_SIZE      (4096 * 8 * 188)

The normal operation is that data will be received from a MPEG-TS
stream, although it is also possible to send data on cable TV, when
using dvb net interface.

While on several boards, the hardware<->kernel transfer happens on
188-bytes packages, there are some hardware out there where the
data passed from/to kernel is not 188-bytes aligned.

The normal operation (receiving a TV broadcast) means that the Kernel 
will be filling a ringbuffer containing the data passed from the
hardware. The size of the such buffer is adjusted via DMX_SET_BUFFER_SIZE
and contains MPEG TS packets of 188-bytes. Userspace will be in an
endless loop that will be waiting for data to arrive at the ringbuffer,
copying received data its own userspace buffer. If the buffer is not set
to a multiple of 188, it should be up to userspace to handle incomplete
frames. The same occurs if the data is 204-bytes aligned. Btw, userspace
can detect the packet size, based on the frame content.

On such example, if a ringbuffer transfer would be passing 1554 bytes,
it means that 8 MPEG-TS frames are complete, and that 50 bytes of the
next frame was also transfered from/to userspace.

It should be up to userspace to ensure that those extra 50 bytes will
be probably taken into account by the application and ensure that the
remaining 138 bytes will be handled at the next from/to userspace
data transfer.

Not the best API, but any change there will break userspace.

In particular, this patch will completely break transfers if the
buffer size is not 188-bytes aligned.

so,

NACK.

Su,

Did you find any real problem with this? On what hardware/application?

Regards,
Mauro

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

[Longsuhui]

On 2023/5/26 18:45, Mauro Carvalho Chehab wrote:
> Em Wed, 24 May 2023 10:20:38 +0300
> Dan Carpenter <dan.carpenter@linaro.org> escreveu:
>
>> On Wed, May 24, 2023 at 01:20:27PM +0800, Su Hui wrote:
>>> 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.
>> To be honest, I think that -EFAULT is probably a better return.  But
>> there is no way we could apply the patch with that commit message.  The
>> commit message doesn't explain the problem for the user or why returning
>> the number of bytes copied is not correct in this case.
>>
>> I think that maybe it's not too late to change this to return -EFAULT,
>> but it would have been easier to make the change in 2014 before there
>> were many users.  Also it would be easier if you were testing this on
>> real hardware.
> It is too late to change the API here, as this could break userspace.
>
> Basically, DVB subsystem normally works with a Kernel-implemented ringbuffer
> that transfers MPEG TS data between kernelspace/userspace. The size is
> set via an ioctl (DMX_SET_BUFFER_SIZE). By the way, such uAPI is older
> than 2014. It was added upstream on Kernel 2.6.
>
> The buffer size is usually big. For instance, dvbv5-zap uses:
>
> 	#define DVB_BUF_SIZE      (4096 * 8 * 188)
>
> The normal operation is that data will be received from a MPEG-TS
> stream, although it is also possible to send data on cable TV, when
> using dvb net interface.
>
> While on several boards, the hardware<->kernel transfer happens on
> 188-bytes packages, there are some hardware out there where the
> data passed from/to kernel is not 188-bytes aligned.
>
> The normal operation (receiving a TV broadcast) means that the Kernel
> will be filling a ringbuffer containing the data passed from the
> hardware. The size of the such buffer is adjusted via DMX_SET_BUFFER_SIZE
> and contains MPEG TS packets of 188-bytes. Userspace will be in an
> endless loop that will be waiting for data to arrive at the ringbuffer,
> copying received data its own userspace buffer. If the buffer is not set
> to a multiple of 188, it should be up to userspace to handle incomplete
> frames. The same occurs if the data is 204-bytes aligned. Btw, userspace
> can detect the packet size, based on the frame content.
>
> On such example, if a ringbuffer transfer would be passing 1554 bytes,
> it means that 8 MPEG-TS frames are complete, and that 50 bytes of the
> next frame was also transfered from/to userspace.
>
> It should be up to userspace to ensure that those extra 50 bytes will
> be probably taken into account by the application and ensure that the
> remaining 138 bytes will be handled at the next from/to userspace
> data transfer.
>
> Not the best API, but any change there will break userspace.
>
> In particular, this patch will completely break transfers if the
> buffer size is not 188-bytes aligned.
>
> so,
>
> NACK.
>
> Su,
>
> Did you find any real problem with this? On what hardware/application?
There is no real problem with this.
I understand, and this patch is wrong.
Sorry to bother you.

Su Hui

>
> Regards,
> Mauro
>

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

[Su Hui]

On 2023/5/26 18:45, Mauro Carvalho Chehab wrote:
> Em Wed, 24 May 2023 10:20:38 +0300
> Dan Carpenter <dan.carpenter@linaro.org> escreveu:
>
>> On Wed, May 24, 2023 at 01:20:27PM +0800, Su Hui wrote:
>>> 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.
>> To be honest, I think that -EFAULT is probably a better return.  But
>> there is no way we could apply the patch with that commit message.  The
>> commit message doesn't explain the problem for the user or why returning
>> the number of bytes copied is not correct in this case.
>>
>> I think that maybe it's not too late to change this to return -EFAULT,
>> but it would have been easier to make the change in 2014 before there
>> were many users.  Also it would be easier if you were testing this on
>> real hardware.
> It is too late to change the API here, as this could break userspace.
>
> Basically, DVB subsystem normally works with a Kernel-implemented ringbuffer
> that transfers MPEG TS data between kernelspace/userspace. The size is
> set via an ioctl (DMX_SET_BUFFER_SIZE). By the way, such uAPI is older
> than 2014. It was added upstream on Kernel 2.6.
>
> The buffer size is usually big. For instance, dvbv5-zap uses:
>
> 	#define DVB_BUF_SIZE      (4096 * 8 * 188)
>
> The normal operation is that data will be received from a MPEG-TS
> stream, although it is also possible to send data on cable TV, when
> using dvb net interface.
>
> While on several boards, the hardware<->kernel transfer happens on
> 188-bytes packages, there are some hardware out there where the
> data passed from/to kernel is not 188-bytes aligned.
>
> The normal operation (receiving a TV broadcast) means that the Kernel
> will be filling a ringbuffer containing the data passed from the
> hardware. The size of the such buffer is adjusted via DMX_SET_BUFFER_SIZE
> and contains MPEG TS packets of 188-bytes. Userspace will be in an
> endless loop that will be waiting for data to arrive at the ringbuffer,
> copying received data its own userspace buffer. If the buffer is not set
> to a multiple of 188, it should be up to userspace to handle incomplete
> frames. The same occurs if the data is 204-bytes aligned. Btw, userspace
> can detect the packet size, based on the frame content.
>
> On such example, if a ringbuffer transfer would be passing 1554 bytes,
> it means that 8 MPEG-TS frames are complete, and that 50 bytes of the
> next frame was also transfered from/to userspace.
>
> It should be up to userspace to ensure that those extra 50 bytes will
> be probably taken into account by the application and ensure that the
> remaining 138 bytes will be handled at the next from/to userspace
> data transfer.
>
> Not the best API, but any change there will break userspace.
>
> In particular, this patch will completely break transfers if the
> buffer size is not 188-bytes aligned.
>
> so,
>
> NACK.
>
> Su,
>
> Did you find any real problem with this? On what hardware/application?
There is no real problem.
I understand, and this patch is wrong.
Sorry to bother you.

Su Hui

>
> Regards,
> Mauro
>