linux-kernel - Re: [PATCH v2] staging: most: video: use min

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: <Y2kf40kSbFWkWkLl@qemulion>
Date:   Mon, 7 Nov 2022 20:40:27 +0530
From:   Deepak R Varma <drv@...lo.com>
To:     Dan Carpenter <error27@...il.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] staging: most: video: use min_t() for comparison and
 assignment

On Mon, Nov 07, 2022 at 04:20:27PM +0300, Dan Carpenter wrote:
> On Mon, Nov 07, 2022 at 09:50:39AM +0530, Deepak R Varma wrote:
> > Simplify code by using min_t helper macro for logical evaluation
> > and value assignment. Use the _t variant of min macro since the
> > variable types are not same.
> > This issue is identified by coccicheck using the minmax.cocci file.
> >
> > Signed-off-by: Deepak R Varma <drv@...lo.com>
> > ---
> >
> > Changes in v2:
> >    1. Revise patch description. No functional change.
> >
> >  drivers/staging/most/video/video.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/most/video/video.c b/drivers/staging/most/video/video.c
> > index ffa97ef21ea5..d5cc7eea3b52 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 = min_t(int, rem, count);
>
> TL;DR use size_t instead of int.

Hi Dan,
Thank you for reviewing the patch. Please see my queries inline.

>
> Using "int" here is wrong.  size_t is unsigned long meaning that it has
> 64 bits to use to represent positive values.  (Let's ignore 32 bit
> arches).  You have chopped it down to say that it now has 31 bits for
> positives and if BIT(31) is set then treat it as negative.  Everything
> which is larger than INT_MAX will be broken.

I did worry about the truncation int might cause to the size_t variable,
however, as the result is being assigned to an int, I decided to go for int to
be the typecast for min_t.

Also, won't size_t will force the int rem to be treated as unsigned value which
will impact the comparison when rem indeed is negative. If rem will never be
-ve, my worry will be void.

Can you please correct if my understanding is wrong?

Thanks,
./drv

>
> Fortunately, in this code the value of count will never go higher than
> "INT_MAX - PAGE_SIZE" because Linus understands that it's easy to
> introduce bugs like this.
>
> regards,
> dan carpenter
>
>