linux-kernel - Re: [PATCH 3/5] media: v4l2-flash-led-class: drop an useless check

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: <20210624093153.GJ3@valkosipuli.retiisi.eu>
Date:   Thu, 24 Jun 2021 12:31:53 +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,

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.

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.

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.

>  	struct v4l2_ctrl **ctrls = v4l2_flash->ctrls;
>  	bool external_strobe;
>  	int ret = 0;

-- 
Kind regards,

Sakari Ailus