| 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
| ||
|
Message-ID: <20210624101443.GK3@valkosipuli.retiisi.eu>
Date: Thu, 24 Jun 2021 13:14:43 +0300
From: Sakari Ailus <sakari.ailus@....fi>
To: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
Cc: linuxarm@...wei.com, mauro.chehab@...wei.com,
Mauro Carvalho Chehab <mchehab@...nel.org>,
linux-kernel@...r.kernel.org, linux-media@...r.kernel.org
Subject: Re: [PATCH 3/5] media: v4l2-flash-led-class: drop an useless check
Hi Mauro,
On Thu, Jun 24, 2021 at 11:59:25AM +0200, Mauro Carvalho Chehab wrote:
> Em Thu, 24 Jun 2021 12:31:53 +0300
> Sakari Ailus <sakari.ailus@....fi> escreveu:
>
> > Hi Mauro,
> >
> > Could you check if your mail client could be configured not to add junk to
> > To: field? It often leads anything in the Cc: field being dropped.
>
> I have no idea why it is doing that. I'm just using git send-email
> here. Perhaps a git bug?
>
> $ git --version
> git version 2.31.1
>
> The setup is like this one:
>
> [sendemail]
> confirm = always
> multiedit = true
> chainreplyto = false
> aliasesfile = /home/mchehab/.addressbook
> aliasfiletype = pine
> assume8bitencoding = UTF-8
I tried sending a message to myself using git send-email with an empty To:
field and it came through just fine, with To: field remaining empty. I
wonder if it could be the list server?
It could be difficult to fix, but what I'm saying leaving the To: field
empty now has this effect. :-I
>
>
> >
> > On Mon, Jun 21, 2021 at 01:56:47PM +0200, Mauro Carvalho Chehab wrote:
> > > As pointed by smatch:
> > > drivers/media/v4l2-core/v4l2-flash-led-class.c:264 v4l2_flash_s_ctrl() error: we previously assumed 'fled_cdev' could be null (see line 197)
> > >
> > > It is too late to check if fled_cdev is NULL there. If such check is
> > > needed, it should be, instead, inside v4l2_flash_init().
> > >
> > > On other words, if v4l2_flash->fled_cdev() is NULL at
> > > v4l2_flash_s_ctrl(), all led_*() function calls inside the function
> > > would try to de-reference a NULL pointer, as the logic won't prevent
> > > it.
> > >
> > > So, remove the useless check.
> > >
> > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
> > > ---
> > > drivers/media/v4l2-core/v4l2-flash-led-class.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/v4l2-core/v4l2-flash-led-class.c b/drivers/media/v4l2-core/v4l2-flash-led-class.c
> > > index 10ddcc48aa17..a1653c635d82 100644
> > > --- a/drivers/media/v4l2-core/v4l2-flash-led-class.c
> > > +++ b/drivers/media/v4l2-core/v4l2-flash-led-class.c
> > > @@ -194,7 +194,7 @@ static int v4l2_flash_s_ctrl(struct v4l2_ctrl *c)
> > > {
> > > struct v4l2_flash *v4l2_flash = v4l2_ctrl_to_v4l2_flash(c);
> > > struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
> > > - struct led_classdev *led_cdev = fled_cdev ? &fled_cdev->led_cdev : NULL;
> > > + struct led_classdev *led_cdev = &fled_cdev->led_cdev;
> >
> > fled_cdev may be NULL here. The reason is that some controls are for flash
> > LEDs only but the same sub-device may also control an indicator. This is
> > covered when the controls are created, so that the NULL pointer isn't
> > dereferenced.
>
> I double-checked the code: if a a NULL pointer is passed, the calls
> to the leds framework will try to de-reference it or will return an
> error.
>
> For instance, those will return an error:
>
> static inline int led_set_flash_strobe(struct led_classdev_flash *fled_cdev,
> bool state)
> {
> if (!fled_cdev)
> return -EINVAL;
> return fled_cdev->ops->strobe_set(fled_cdev, state);
> }
>
> #define call_flash_op(fled_cdev, op, args...) \
> ((has_flash_op(fled_cdev, op)) ? \
> (fled_cdev->ops->op(fled_cdev, args)) : \
> -EINVAL)
>
> No big issue here (except that the function will do nothing but
> return an error).
>
> However, there are places that it will cause it to de-reference
> a NULL pointer:
>
> int led_set_brightness_sync(struct led_classdev *led_cdev, unsigned int value)
> {
> if (led_cdev->blink_delay_on || led_cdev->blink_delay_off)
> return -EBUSY;
>
> led_cdev->brightness = min(value, led_cdev->max_brightness);
>
> if (led_cdev->flags & LED_SUSPENDED)
> return 0;
>
> return __led_set_brightness_blocking(led_cdev, led_cdev->brightness);
> }
>
> So, this is not a false-positive, but, instead, a real issue.
>
> So, if led_cdev/fled_cdev can indeed be NULL, IMO, the right solution would be
> to explicitly check it, and return an error, e. g.:
>
> static int v4l2_flash_s_ctrl(struct v4l2_ctrl *c)
> {
> struct v4l2_flash *v4l2_flash = v4l2_ctrl_to_v4l2_flash(c);
> struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
> struct led_classdev *led_cdev;
> struct v4l2_ctrl **ctrls = v4l2_flash->ctrls;
> bool external_strobe;
> int ret = 0;
>
> if (!fled_cdev)
> return -EINVAL;
The approach is correct, but as noted, the check needs to be done later.
Could you drop this patch, please?
>
> led_cdev = &fled_cdev->led_cdev;
>
> ...
>
> >
> > If you wish the false positive to be addressed while also improving the
> > implementation, that could be done by e.g. splitting the switch into two,
> > the part that needs fled_cdev and another that doesn't.
> >
> > I can send a patch for that.
> >
> > Please also cc me to V4L2 flash class patches. I noticed this one by
> > accident only.
>
> Better to add you as a reviewer at the MAINTAINERS file, to
> ensure that you'll always be c/c on such code.
There's no separate entry for flash class, just like the rest of the V4L2
core. I think it could be worth addressing this for all the bits in V4L2
core, but that's separate from this issue in any case.
--
Kind regards,
Sakari Ailus
Powered by blists - more mailing lists