linux-kernel - Re: [PATCH 10/13] media: adv7604 prevent underflow condition when reporting colorspace

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: <c04c22f5-cafa-4618-ad7c-319a8afc6214@xs4all.nl>
Date: Wed, 16 Oct 2024 13:58:48 +0200
From: Hans Verkuil <hverkuil@...all.nl>
To: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
 stable@...r.kernel.org
Subject: Re: [PATCH 10/13] media: adv7604 prevent underflow condition when
 reporting colorspace

On 16/10/2024 13:24, Mauro Carvalho Chehab wrote:
> Em Wed, 16 Oct 2024 12:57:53 +0200
> Hans Verkuil <hverkuil@...all.nl> escreveu:
> 
>> On 16/10/2024 12:22, Mauro Carvalho Chehab wrote:
>>> Currently, adv76xx_log_status() reads some date using
>>> io_read() which may return negative values. The current logi
>>> doesn't check such errors, causing colorspace to be reported
>>> on a wrong way at adv76xx_log_status().
>>>
>>> If I/O error happens there, print a different message, instead
>>> of reporting bogus messages to userspace.
>>>
>>> Fixes: 54450f591c99 ("[media] adv7604: driver for the Analog Devices ADV7604 video decoder")
>>> Cc: stable@...r.kernel.org  
>>
>> Not really a fix since this would just affect logging for debugging
>> purposes. I would personally just drop the Fixes and Cc tag.
> 
> The issue is on a VIDIOC_ ioctl, so part of media API. Ok, this is
> used only for debugging purposes and should, instead be implemented
> via debugfs, etc, but, in summary: it is what it is: part of the V4L2
> uAPI.

The ioctl, yes, but what it logs to the kernel log isn't part of the ABI.
That can change.

I think it is overkill to send this to stable for an old chip that almost
nobody uses, and that requires an i2c read to go wrong for it to produce
a wrong debug message. It seems an unnecessary waste of time.

> 
> -
> 
> Now, the question about what should have Fixes: tag and what
> shouldn't is a different matter. I've saw long discussions about
> that at the kernel mailing lists. In the particular case of y2038,
> I'm pretty sure I saw some of them with Fixes tag on it.

But patch 13/13 doesn't affect the operation either, again it is just
an incorrect log message that can only go wrong if Pulse-Eight still
sells that device in 2038 with a firmware build date >= 2038. And v6.12
is guaranteed to be EOL in 2038 :-)

Regards,

	Hans

> 
>>
>>> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>  
>>
>> Reviewed-by: Hans Verkuil <hverkuil@...all.nl>
>>
>> Regards,
>>
>> 	Hans
>>
>>> ---
>>>  drivers/media/i2c/adv7604.c | 26 +++++++++++++++++---------
>>>  1 file changed, 17 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
>>> index 48230d5109f0..272945a878b3 100644
>>> --- a/drivers/media/i2c/adv7604.c
>>> +++ b/drivers/media/i2c/adv7604.c
>>> @@ -2519,10 +2519,10 @@ static int adv76xx_log_status(struct v4l2_subdev *sd)
>>>  	const struct adv76xx_chip_info *info = state->info;
>>>  	struct v4l2_dv_timings timings;
>>>  	struct stdi_readback stdi;
>>> -	u8 reg_io_0x02 = io_read(sd, 0x02);
>>> +	int ret;
>>> +	u8 reg_io_0x02;
>>>  	u8 edid_enabled;
>>>  	u8 cable_det;
>>> -
>>>  	static const char * const csc_coeff_sel_rb[16] = {
>>>  		"bypassed", "YPbPr601 -> RGB", "reserved", "YPbPr709 -> RGB",
>>>  		"reserved", "RGB -> YPbPr601", "reserved", "RGB -> YPbPr709",
>>> @@ -2621,13 +2621,21 @@ static int adv76xx_log_status(struct v4l2_subdev *sd)
>>>  	v4l2_info(sd, "-----Color space-----\n");
>>>  	v4l2_info(sd, "RGB quantization range ctrl: %s\n",
>>>  			rgb_quantization_range_txt[state->rgb_quantization_range]);
>>> -	v4l2_info(sd, "Input color space: %s\n",
>>> -			input_color_space_txt[reg_io_0x02 >> 4]);
>>> -	v4l2_info(sd, "Output color space: %s %s, alt-gamma %s\n",
>>> -			(reg_io_0x02 & 0x02) ? "RGB" : "YCbCr",
>>> -			(((reg_io_0x02 >> 2) & 0x01) ^ (reg_io_0x02 & 0x01)) ?
>>> -				"(16-235)" : "(0-255)",
>>> -			(reg_io_0x02 & 0x08) ? "enabled" : "disabled");
>>> +
>>> +	ret = io_read(sd, 0x02);
>>> +	if (ret < 0) {
>>> +		v4l2_info(sd, "Can't read Input/Output color space\n");
>>> +	} else {
>>> +		reg_io_0x02 = ret;
>>> +
>>> +		v4l2_info(sd, "Input color space: %s\n",
>>> +				input_color_space_txt[reg_io_0x02 >> 4]);
>>> +		v4l2_info(sd, "Output color space: %s %s, alt-gamma %s\n",
>>> +				(reg_io_0x02 & 0x02) ? "RGB" : "YCbCr",
>>> +				(((reg_io_0x02 >> 2) & 0x01) ^ (reg_io_0x02 & 0x01)) ?
>>> +					"(16-235)" : "(0-255)",
>>> +				(reg_io_0x02 & 0x08) ? "enabled" : "disabled");
>>> +	}
>>>  	v4l2_info(sd, "Color space conversion: %s\n",
>>>  			csc_coeff_sel_rb[cp_read(sd, info->cp_csc) >> 4]);
>>>    
>>
> 
> 
> 
> Thanks,
> Mauro