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: <Y2kiixRDxjxSwFp+@kadam>
Date:   Mon, 7 Nov 2022 18:21:47 +0300
From:   Dan Carpenter <error27@...il.com>
To:     Deepak R Varma <drv@...lo.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 08:40:27PM +0530, Deepak R Varma wrote:
> 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.

Let's ignore that other layers prevent "count" from being greater than
INT_MAX.  mbo->processed_length is a u16.  Also if "fh->offs" is more
than mbo->processed_length that's a separate bug and we are already
screwed.

So that means rem is a relatively small number.  A small number can
easily fit in "int cnt".  So we are eating a big pie ("count") but we
are taking small bites ("cnt").  Everything works fine.

But if we chop the pie in half or treat it as negative pie then the
math breaks.

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

Is "-ve" the TikTok way of abbreviating negative?  Am I old?

The small bites are always positive.  But if we are eating negative
pie then we take negative size bites.  min_t() should almost always use
unsigned types.  Everything else is a headache.  I have often wondered
why people do it but I think it's because of the 80 character rule and
the word "int" is shorter than "unsigned long".

regards,
dan carpenter