[PATCH] staging: most: video: improve arguments to copy_to_user()

[PATCH] staging: most: video: improve arguments to copy_to_user()

[Akhilesh Patil]

Define cnt constant as unsigned long as expected by copy_to_user()
to avoid implicit type conversion. Define rem constant as unsigned long
to compare it with the same type size_t of count variable.
Use standard helper min() to carry out careful comparison to achive
same functionality.

Signed-off-by: Akhilesh Patil <akhilesh@ee.iitb.ac.in>
---
This patch is motivated from coccinelle report which suggested to use
kernel standard helper min(). During build check, I found that
comparison max()  showing error while comparing variables of
different types. Hence this patch also fixes that to make comparison of
save types.

Compile tested only.
---
 drivers/staging/most/video/video.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/most/video/video.c b/drivers/staging/most/video/video.c
index 2b3cdb1ce140..4b15c390c32d 100644
--- a/drivers/staging/most/video/video.c
+++ b/drivers/staging/most/video/video.c
@@ -172,8 +172,8 @@ static ssize_t comp_vdev_read(struct file *filp, char __user *buf,
 
 	while (count > 0 && data_ready(mdev)) {
 		struct mbo *const mbo = get_top_mbo(mdev);
-		int const rem = mbo->processed_length - fh->offs;
-		int const cnt = rem < count ? rem : count;
+		unsigned long const rem = mbo->processed_length - fh->offs;
+		unsigned long const cnt = min(rem, count);
 
 		if (copy_to_user(buf, mbo->virt_address + fh->offs, cnt)) {
 			v4l2_err(&mdev->v4l2_dev, "read: copy_to_user failed\n");
-- 
2.34.1

Re: [PATCH] staging: most: video: improve arguments to copy_to_user()

[Greg KH]

On Wed, Aug 13, 2025 at 05:12:48PM +0530, Akhilesh Patil wrote:
> Define cnt constant as unsigned long as expected by copy_to_user()
> to avoid implicit type conversion. Define rem constant as unsigned long
> to compare it with the same type size_t of count variable.
> Use standard helper min() to carry out careful comparison to achive
> same functionality.
> 
> Signed-off-by: Akhilesh Patil <akhilesh@ee.iitb.ac.in>
> ---
> This patch is motivated from coccinelle report which suggested to use
> kernel standard helper min(). During build check, I found that
> comparison max()  showing error while comparing variables of
> different types. Hence this patch also fixes that to make comparison of
> save types.
> 
> Compile tested only.
> ---
>  drivers/staging/most/video/video.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/most/video/video.c b/drivers/staging/most/video/video.c
> index 2b3cdb1ce140..4b15c390c32d 100644
> --- a/drivers/staging/most/video/video.c
> +++ b/drivers/staging/most/video/video.c
> @@ -172,8 +172,8 @@ static ssize_t comp_vdev_read(struct file *filp, char __user *buf,
>  
>  	while (count > 0 && data_ready(mdev)) {
>  		struct mbo *const mbo = get_top_mbo(mdev);
> -		int const rem = mbo->processed_length - fh->offs;
> -		int const cnt = rem < count ? rem : count;
> +		unsigned long const rem = mbo->processed_length - fh->offs;

Why is this unsigned long now?  processed_length is a u16 and offs is a
u32.  So really this could have gone negative :(


> +		unsigned long const cnt = min(rem, count);

But count is size_t, so again, why unsigned long?

I understand the goal of using min(), but it feels like you just might
have broken something with the loss of the overflow check :(

thanks,

greg k-h

Re: [PATCH] staging: most: video: improve arguments to copy_to_user()

[Dan Carpenter]

On Wed, Aug 13, 2025 at 05:12:48PM +0530, Akhilesh Patil wrote:
> Define cnt constant as unsigned long as expected by copy_to_user()
> to avoid implicit type conversion. Define rem constant as unsigned long
> to compare it with the same type size_t of count variable.
> Use standard helper min() to carry out careful comparison to achive
> same functionality.
> 
> Signed-off-by: Akhilesh Patil <akhilesh@ee.iitb.ac.in>
> ---
> This patch is motivated from coccinelle report which suggested to use
> kernel standard helper min(). During build check, I found that
> comparison max()  showing error while comparing variables of
> different types. Hence this patch also fixes that to make comparison of
> save types.
> 
> Compile tested only.
> ---
>  drivers/staging/most/video/video.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/most/video/video.c b/drivers/staging/most/video/video.c
> index 2b3cdb1ce140..4b15c390c32d 100644
> --- a/drivers/staging/most/video/video.c
> +++ b/drivers/staging/most/video/video.c
> @@ -172,8 +172,8 @@ static ssize_t comp_vdev_read(struct file *filp, char __user *buf,
>  
>  	while (count > 0 && data_ready(mdev)) {
>  		struct mbo *const mbo = get_top_mbo(mdev);
> -		int const rem = mbo->processed_length - fh->offs;
> -		int const cnt = rem < count ? rem : count;
> +		unsigned long const rem = mbo->processed_length - fh->offs;
> +		unsigned long const cnt = min(rem, count);

The easiest (and most pointless and trivial) comment to make is the
"const" adds no value here.  It doesn't help the compiler and it doesn't
help human readers.

So you wanted to use min(), but it generated warnings that rem is signed
and count is unsigned.  A different way to silence the warning is to
say umin(rem, count).  The umin() macro casts both sides to unsigned.

diff --git a/drivers/staging/most/video/video.c b/drivers/staging/most/video/video.c
index 2b3cdb1ce140..de07ed831815 100644
--- a/drivers/staging/most/video/video.c
+++ b/drivers/staging/most/video/video.c
@@ -173,7 +173,7 @@ static ssize_t comp_vdev_read(struct file *filp, char __user *buf,
 	while (count > 0 && data_ready(mdev)) {
 		struct mbo *const mbo = get_top_mbo(mdev);
 		int const rem = mbo->processed_length - fh->offs;
-		int const cnt = rem < count ? rem : count;
+		int const cnt = umin(rem, count);
 
 		if (copy_to_user(buf, mbo->virt_address + fh->offs, cnt)) {
 			v4l2_err(&mdev->v4l2_dev, "read: copy_to_user failed\n");

So can the rem variable actually be negative?  It can't unless there is
an issue with locking but I feel like the locking in this driver is
pretty suspect so there could easily be an issue.

So probably we could investigate to see if multiple thread can call
comp_vdev_read() at the same time and maybe add a sanity check to say:

	int rem = mbo->processed_length - fh->offs;

	if (rem < 0) {
		WARN_ON_ONCE(true);
		return -EINVAL;
	}

regards,
dan carpenter