linux-kernel - Re: [PATCH] media: em28xx: Fix missing check in em28xx_capture

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] [day] [month] [year] [list]

Message-ID: <74451231-31b9-4489-2241-febb05cf5bec@xs4all.nl>
Date:   Tue, 16 Mar 2021 08:55:48 +0100
From:   Hans Verkuil <hverkuil@...all.nl>
To:     Dinghao Liu <dinghao.liu@....edu.cn>, kjlu@....edu
Cc:     Mauro Carvalho Chehab <mchehab@...nel.org>,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] media: em28xx: Fix missing check in em28xx_capture_start

Hi Dinghao Liu,

Thank you for the patch, but I've decided not to take it. While the
patch looks fine, it is not very useful since the return code of the
em28xx_capture_start() function is never checked. And I am hesitant
to change the behavior here in case it might break something subtle.

Ideally the error code of em28xx_capture_start() should also be
checked, and cascaded all the way up to the higher levels of the code,
but the reality is that if these register writes fail, then you likely
have much bigger problems so I don't think it is worth doing that.

Regards,

	Hans

On 10/03/2021 10:00, Dinghao Liu wrote:
> There are several em28xx_write_reg() and em28xx_write_reg_bits()
> calls that we have caught their return values but lack further
> handling. Check and return error on failure just like other calls
> in em28xx_capture_start().
> 
> Signed-off-by: Dinghao Liu <dinghao.liu@....edu.cn>
> ---
>  drivers/media/usb/em28xx/em28xx-core.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
> index 584fa400cd7d..2563275fec8e 100644
> --- a/drivers/media/usb/em28xx/em28xx-core.c
> +++ b/drivers/media/usb/em28xx/em28xx-core.c
> @@ -661,6 +661,8 @@ int em28xx_capture_start(struct em28xx *dev, int start)
>  						   EM2874_R5F_TS_ENABLE,
>  						   start ? EM2874_TS2_CAPTURE_ENABLE : 0x00,
>  						   EM2874_TS2_CAPTURE_ENABLE | EM2874_TS2_FILTER_ENABLE | EM2874_TS2_NULL_DISCARD);
> +		if (rc < 0)
> +			return rc;
>  	} else {
>  		/* FIXME: which is the best order? */
>  		/* video registers are sampled by VREF */
> @@ -670,8 +672,11 @@ int em28xx_capture_start(struct em28xx *dev, int start)
>  			return rc;
>  
>  		if (start) {
> -			if (dev->is_webcam)
> +			if (dev->is_webcam) {
>  				rc = em28xx_write_reg(dev, 0x13, 0x0c);
> +				if (rc < 0)
> +					return rc;
> +			}
>  
>  			/* Enable video capture */
>  			rc = em28xx_write_reg(dev, 0x48, 0x00);
> @@ -693,6 +698,8 @@ int em28xx_capture_start(struct em28xx *dev, int start)
>  		} else {
>  			/* disable video capture */
>  			rc = em28xx_write_reg(dev, EM28XX_R12_VINENABLE, 0x27);
> +			if (rc < 0)
> +				return rc;
>  		}
>  	}
>  
>