linux-kernel - Re: [PATCH] staging: most: video: improve arguments to copy_to

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] [day] [month] [year] [list]

Message-ID: <aJyx7o9KeVS0HEhh@stanley.mountain>
Date: Wed, 13 Aug 2025 18:40:30 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Akhilesh Patil <akhilesh@...iitb.ac.in>
Cc: parthiban.veerasooran@...rochip.com, christian.gromm@...rochip.com,
	gregkh@...uxfoundation.org, linux-staging@...ts.linux.dev,
	linux-kernel@...r.kernel.org, akhileshpatilvnit@...il.com,
	skhan@...uxfoundation.org
Subject: Re: [PATCH] staging: most: video: improve arguments to copy_to_user()

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@...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