| Message ID | 20230524012733.414441-1-suhui@nfschina.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: dvb_ringbuffer: Return -EFAULT if copy fails | expand |
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
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
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 >
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
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 >
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
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 >
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 >
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);
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(-)