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 <[email protected]>
> >
> > 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
