Re: [PATCH 08/30] media: v4l2-ioctl: fix some "too small" warnings

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Mauro,

On Monday, 26 March 2018 23:28:55 EEST Mauro Carvalho Chehab wrote:
> Em Mon, 26 Mar 2018 21:47:33 +0300 Laurent Pinchart escreveu:
> > On Monday, 26 March 2018 13:08:16 EEST Mauro Carvalho Chehab wrote:
> > > Em Fri, 23 Mar 2018 23:53:56 +0200 Sakari Ailus escreveu:
> > > > On Fri, Mar 23, 2018 at 07:56:54AM -0400, Mauro Carvalho Chehab wrote:
> > > >> While the code there is right, it produces three false positives:
> > > >> 	drivers/media/v4l2-core/v4l2-ioctl.c:2868 video_usercopy() error:
> > > >> 	copy_from_user() 'parg' too small (128 vs 16383)
> > > >> 	drivers/media/v4l2-core/v4l2-ioctl.c:2868 video_usercopy() error:
> > > >> 	copy_from_user() 'parg' too small (128 vs 16383)
> > > >> 	drivers/media/v4l2-core/v4l2-ioctl.c:2876 video_usercopy() error:
> > > >> 	memset() 'parg' too small (128 vs 16383)> >
> > > >> 
> > > >> Store the ioctl size on a cache var, in order to suppress those.
> > > > 
> > > > I have to say I'm not a big fan of changing perfectly fine code in
> > > > order
> > > > to please static analysers.
> > > 
> > > Well, there's a side effect of this patch: it allows gcc to better
> > > 
> > > optimize the text size, with is good:
> > >    text	   data	    bss	    dec	    hex	filename
> > >   
> > >   34538	   2320	      0	  36858
> > > 
> > > 8ffa	old/drivers/media/v4l2-core/v4l2-ioctl.o 34490	   2320	      0
> > > 36810	   8fca	new/drivers/media/v4l2-core/v4l2-ioctl.o
> > > 
> > > > What's this, smatch? I wonder if it could be fixed
> > > > instead of changing the code. That'd be presumably a lot more work
> > > > though.
> > > 
> > > Yes, the warnings came from smatch. No idea how easy/hard would be to
> > > change it.
> > > 
> > > > On naming --- "size" is rather more generic, but it's not a long
> > > > function
> > > > either. I'd be a bit more specific, e.g. ioc_size or arg_size.
> > > 
> > > Agreed.
> > > 
> > > I'll add the enclosed patch changing it to ioc_size.
> > > 
> > > 
> > > Thanks,
> > > Mauro
> > > 
> > > [PATCH] media: v4l2-ioctl: rename a temp var that stores _IOC_SIZE(cmd)
> > > 
> > > Instead of just calling it as "size", let's name it as "ioc_size",
> > > as it reflects better its contents.
> > > 
> > > As this is constant along the function, also mark it as const.
> > > 
> > > Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> > 
> > I would have expected a v2 of the original patch, but it seems you pushed
> > it to the public master branch before giving anyone a change to review it
> > (Sakari's review came 10h after the past was posted).
> > 
> > Patches need to be reviewed before being applied, and that applies to all
> > patches, regarding of the author. Please refrain from merging future
> > patches before they get reviewed.
> 
> It shouldn't have pushed. It happened due to some script errors,
> when I was handling a request from Hans to push upstream some fixes
> for -rc7, as mentioned on IRC:
> 	https://linuxtv.org/irc/irclogger_log/v4l?date=2018-03-23,Fri

Thank you for the explanation. Bugs happen, luckily it should have less impact 
than spectre or meltdown :-)

> As I said there:
> 
> "If someone finds an issue on the warning fixes, I can revert or apply a
> fixup I really hate when things like that happens :-("

-- 
Regards,

Laurent Pinchart