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

Message-ID: <2025081323-paralyses-monetary-89f0@gregkh>
Date: Wed, 13 Aug 2025 16:24:22 +0200
From: Greg KH <gregkh@...uxfoundation.org>
To: Akhilesh Patil <akhilesh@...iitb.ac.in>
Cc: parthiban.veerasooran@...rochip.com, christian.gromm@...rochip.com,
	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;

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