[PATCH 01/13] media: v4l2-ctrls-api: fix error handling for v4l2_g_ctrl()

[PATCH 01/13] media: v4l2-ctrls-api: fix error handling for v4l2_g_ctrl()

[Mauro Carvalho Chehab]

The error check logic at get_ctrl() is broken: if ptr_to_user()
fails to fill a control due to an error, no errors are returned
and v4l2_g_ctrl() returns success on a failed operation, which
may cause applications to fail.

Add an error check at get_ctrl() and ensure that it will
be returned to userspace without filling the control value if
get_ctrl() fails.

Fixes: 71c689dc2e73 ("media: v4l2-ctrls: split up into four source files")
Cc: stable@vger.kernel.org
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/v4l2-core/v4l2-ctrls-api.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c
index e5a364efd5e6..be32dccf9830 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls-api.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls-api.c
@@ -753,9 +753,10 @@ static int get_ctrl(struct v4l2_ctrl *ctrl, struct v4l2_ext_control *c)
 		for (i = 0; i < master->ncontrols; i++)
 			cur_to_new(master->cluster[i]);
 		ret = call_op(master, g_volatile_ctrl);
-		new_to_user(c, ctrl);
+		if (!ret)
+			ret = new_to_user(c, ctrl);
 	} else {
-		cur_to_user(c, ctrl);
+		ret = cur_to_user(c, ctrl);
 	}
 	v4l2_ctrl_unlock(master);
 	return ret;
@@ -770,7 +771,10 @@ int v4l2_g_ctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_control *control)
 	if (!ctrl || !ctrl->is_int)
 		return -EINVAL;
 	ret = get_ctrl(ctrl, &c);
-	control->value = c.value;
+
+	if (!ret)
+		control->value = c.value;
+
 	return ret;
 }
 EXPORT_SYMBOL(v4l2_g_ctrl);
-- 
2.47.0

Re: [PATCH 01/13] media: v4l2-ctrls-api: fix error handling for v4l2_g_ctrl()

[Hans Verkuil]

On 16/10/2024 12:22, Mauro Carvalho Chehab wrote:
> The error check logic at get_ctrl() is broken: if ptr_to_user()
> fails to fill a control due to an error, no errors are returned
> and v4l2_g_ctrl() returns success on a failed operation, which
> may cause applications to fail.
> 
> Add an error check at get_ctrl() and ensure that it will
> be returned to userspace without filling the control value if
> get_ctrl() fails.
> 
> Fixes: 71c689dc2e73 ("media: v4l2-ctrls: split up into four source files")
> Cc: stable@vger.kernel.org
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
>  drivers/media/v4l2-core/v4l2-ctrls-api.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c
> index e5a364efd5e6..be32dccf9830 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-api.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-api.c
> @@ -753,9 +753,10 @@ static int get_ctrl(struct v4l2_ctrl *ctrl, struct v4l2_ext_control *c)
>  		for (i = 0; i < master->ncontrols; i++)
>  			cur_to_new(master->cluster[i]);
>  		ret = call_op(master, g_volatile_ctrl);
> -		new_to_user(c, ctrl);
> +		if (!ret)
> +			ret = new_to_user(c, ctrl);
>  	} else {
> -		cur_to_user(c, ctrl);
> +		ret = cur_to_user(c, ctrl);
>  	}
>  	v4l2_ctrl_unlock(master);
>  	return ret;
> @@ -770,7 +771,10 @@ int v4l2_g_ctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_control *control)
>  	if (!ctrl || !ctrl->is_int)
>  		return -EINVAL;
>  	ret = get_ctrl(ctrl, &c);
> -	control->value = c.value;
> +
> +	if (!ret)
> +		control->value = c.value;
> +
>  	return ret;
>  }
>  EXPORT_SYMBOL(v4l2_g_ctrl);

Yeah, that's better.

There are also unchecked calls to cur_to_user() and user_to_new() in set_ctrl_lock().

Can you fix that as well?

Regards,

	Hans

[PATCH 02/13] media: v4l2-tpg: prevent the risk of a division by zero

[Mauro Carvalho Chehab]

The logic at tpg_precalculate_line() blindly rescales the
buffer even when scaled_witdh is equal to zero. If this ever
happens, this will cause a division by zero.

Instead, add a WARN_ON() to trigger such cases and return
without doing any precalculation.

Fixes: 63881df94d3e ("[media] vivid: add the Test Pattern Generator")
Cc: stable@vger.kernel.org
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/common/v4l2-tpg/v4l2-tpg-core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c b/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c
index c86343a4d0bf..a22f31515d7e 100644
--- a/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c
+++ b/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c
@@ -1795,6 +1795,9 @@ static void tpg_precalculate_line(struct tpg_data *tpg)
 	unsigned p;
 	unsigned x;
 
+	if (WARN_ON(tpg->src_width == 0))
+		return;
+
 	switch (tpg->pattern) {
 	case TPG_PAT_GREEN:
 		contrast = TPG_COLOR_100_RED;
-- 
2.47.0

Re: [PATCH 02/13] media: v4l2-tpg: prevent the risk of a division by zero

[Hans Verkuil]

On 16/10/2024 12:22, Mauro Carvalho Chehab wrote:
> The logic at tpg_precalculate_line() blindly rescales the
> buffer even when scaled_witdh is equal to zero. If this ever

scaled_witdh -> scaled_width

> happens, this will cause a division by zero.
> 
> Instead, add a WARN_ON() to trigger such cases and return
> without doing any precalculation.
> 
> Fixes: 63881df94d3e ("[media] vivid: add the Test Pattern Generator")
> Cc: stable@vger.kernel.org
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
>  drivers/media/common/v4l2-tpg/v4l2-tpg-core.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c b/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c
> index c86343a4d0bf..a22f31515d7e 100644
> --- a/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c
> +++ b/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c
> @@ -1795,6 +1795,9 @@ static void tpg_precalculate_line(struct tpg_data *tpg)
>  	unsigned p;
>  	unsigned x;
>  
> +	if (WARN_ON(tpg->src_width == 0))

You need to check for both src_width and scaled_width.

Also replace this by WARN_ON_ONCE.

Regards,

	Hans

> +		return;
> +
>  	switch (tpg->pattern) {
>  	case TPG_PAT_GREEN:
>  		contrast = TPG_COLOR_100_RED;

[PATCH 03/13] media: dvbdev: prevent the risk of out of memory access

[Mauro Carvalho Chehab]

The dvbdev contains a static variable used to store dvb minors.

The behavior of it depends if CONFIG_DVB_DYNAMIC_MINORS is set
or not. When not set, dvb_register_device() won't check for
boundaries, as it will rely that a previous call to
dvb_register_adapter() would already be enforcing it.

On a similar way, dvb_device_open() uses the assumption
that the register functions already did the needed checks.

This can be fragile if some device ends using different
calls. This also generate warnings on static check analysers.

So, add explicit guards to prevent potential risk of OOM issues.

Fixes: 5dd3f3071070 ("V4L/DVB (9361): Dynamic DVB minor allocation")
Cc: stable@vger.kernel.org
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/dvb-core/dvbdev.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c
index b43695bc51e7..14f323fbada7 100644
--- a/drivers/media/dvb-core/dvbdev.c
+++ b/drivers/media/dvb-core/dvbdev.c
@@ -86,10 +86,15 @@ static DECLARE_RWSEM(minor_rwsem);
 static int dvb_device_open(struct inode *inode, struct file *file)
 {
 	struct dvb_device *dvbdev;
+	unsigned int minor = iminor(inode);
+
+	if (minor >= MAX_DVB_MINORS)
+		return -ENODEV;
 
 	mutex_lock(&dvbdev_mutex);
 	down_read(&minor_rwsem);
-	dvbdev = dvb_minors[iminor(inode)];
+
+	dvbdev = dvb_minors[minor];
 
 	if (dvbdev && dvbdev->fops) {
 		int err = 0;
@@ -525,7 +530,7 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev,
 	for (minor = 0; minor < MAX_DVB_MINORS; minor++)
 		if (!dvb_minors[minor])
 			break;
-	if (minor == MAX_DVB_MINORS) {
+	if (minor >= MAX_DVB_MINORS) {
 		if (new_node) {
 			list_del(&new_node->list_head);
 			kfree(dvbdevfops);
@@ -540,6 +545,14 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev,
 	}
 #else
 	minor = nums2minor(adap->num, type, id);
+	if (minor >= MAX_DVB_MINORS) {
+		dvb_media_device_free(dvbdev);
+		list_del(&dvbdev->list_head);
+		kfree(dvbdev);
+		*pdvbdev = NULL;
+		mutex_unlock(&dvbdev_register_lock);
+		return ret;
+	}
 #endif
 	dvbdev->minor = minor;
 	dvb_minors[minor] = dvb_device_get(dvbdev);
-- 
2.47.0

[PATCH 04/13] media: dvb_frontend: don't play tricks with underflow values

[Mauro Carvalho Chehab]

fepriv->auto_sub_step is unsigned. Setting it to -1 is just a
trick to avoid calling continue.

It relies to have this code just afterwards:

	if (!ready) fepriv->auto_sub_step++;

Simplify the code by simply setting it to zero and use
continue to return to the while loop.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/dvb-core/dvb_frontend.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
index d48f48fda87c..c9283100332a 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -443,8 +443,8 @@ static int dvb_frontend_swzigzag_autotune(struct dvb_frontend *fe, int check_wra
 
 		default:
 			fepriv->auto_step++;
-			fepriv->auto_sub_step = -1; /* it'll be incremented to 0 in a moment */
-			break;
+			fepriv->auto_sub_step = 0;
+			continue;
 		}
 
 		if (!ready) fepriv->auto_sub_step++;
-- 
2.47.0

[PATCH 05/13] media: mgb4: protect driver against spectre

[Mauro Carvalho Chehab]

Frequency range is set from sysfs via frequency_range_store(),
being vulnerable to spectre, as reported by smatch:

	drivers/media/pci/mgb4/mgb4_cmt.c:231 mgb4_cmt_set_vin_freq_range() warn: potential spectre issue 'cmt_vals_in' [r]
	drivers/media/pci/mgb4/mgb4_cmt.c:238 mgb4_cmt_set_vin_freq_range() warn: possible spectre second half.  'reg_set'

Fix it.

Fixes: 0ab13674a9bd ("media: pci: mgb4: Added Digiteq Automotive MGB4 driver")
Cc: stable@vger.kernel.org
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/pci/mgb4/mgb4_cmt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/pci/mgb4/mgb4_cmt.c b/drivers/media/pci/mgb4/mgb4_cmt.c
index 70dc78ef193c..a25b68403bc6 100644
--- a/drivers/media/pci/mgb4/mgb4_cmt.c
+++ b/drivers/media/pci/mgb4/mgb4_cmt.c
@@ -227,6 +227,8 @@ void mgb4_cmt_set_vin_freq_range(struct mgb4_vin_dev *vindev,
 	u32 config;
 	size_t i;
 
+	freq_range = array_index_nospec(freq_range, ARRAY_SIZE(cmt_vals_in));
+
 	addr = cmt_addrs_in[vindev->config->id];
 	reg_set = cmt_vals_in[freq_range];
 
-- 
2.47.0

Re: [PATCH 05/13] media: mgb4: protect driver against spectre

[Martin Tůma]

On 16. 10. 24 12:22 odp., Mauro Carvalho Chehab wrote:
> Frequency range is set from sysfs via frequency_range_store(),
> being vulnerable to spectre, as reported by smatch:
> 
> 	drivers/media/pci/mgb4/mgb4_cmt.c:231 mgb4_cmt_set_vin_freq_range() warn: potential spectre issue 'cmt_vals_in' [r]
> 	drivers/media/pci/mgb4/mgb4_cmt.c:238 mgb4_cmt_set_vin_freq_range() warn: possible spectre second half.  'reg_set'
> 
> Fix it.
> 
> Fixes: 0ab13674a9bd ("media: pci: mgb4: Added Digiteq Automotive MGB4 driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
>   drivers/media/pci/mgb4/mgb4_cmt.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/media/pci/mgb4/mgb4_cmt.c b/drivers/media/pci/mgb4/mgb4_cmt.c
> index 70dc78ef193c..a25b68403bc6 100644
> --- a/drivers/media/pci/mgb4/mgb4_cmt.c
> +++ b/drivers/media/pci/mgb4/mgb4_cmt.c
> @@ -227,6 +227,8 @@ void mgb4_cmt_set_vin_freq_range(struct mgb4_vin_dev *vindev,
>   	u32 config;
>   	size_t i;
>   
> +	freq_range = array_index_nospec(freq_range, ARRAY_SIZE(cmt_vals_in));
> +
>   	addr = cmt_addrs_in[vindev->config->id];
>   	reg_set = cmt_vals_in[freq_range];
>   

I still do not fully understand the exact vulnerability here, but the 
patch should definitely not do any harm, so I'm ok with it even if it's 
real purpose would only be to silence the smatch warning :-)

Reviewed-by: Martin Tůma <martin.tuma@digiteqautomotive.com>

Re: [PATCH 05/13] media: mgb4: protect driver against spectre

[Mauro Carvalho Chehab]

Em Wed, 16 Oct 2024 13:59:18 +0200
Martin Tůma <tumic@gpxsee.org> escreveu:

> On 16. 10. 24 12:22 odp., Mauro Carvalho Chehab wrote:
> > Frequency range is set from sysfs via frequency_range_store(),
> > being vulnerable to spectre, as reported by smatch:
> > 
> > 	drivers/media/pci/mgb4/mgb4_cmt.c:231 mgb4_cmt_set_vin_freq_range() warn: potential spectre issue 'cmt_vals_in' [r]
> > 	drivers/media/pci/mgb4/mgb4_cmt.c:238 mgb4_cmt_set_vin_freq_range() warn: possible spectre second half.  'reg_set'
> > 
> > Fix it.
> > 
> > Fixes: 0ab13674a9bd ("media: pci: mgb4: Added Digiteq Automotive MGB4 driver")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > ---
> >   drivers/media/pci/mgb4/mgb4_cmt.c | 2 ++
> >   1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/media/pci/mgb4/mgb4_cmt.c b/drivers/media/pci/mgb4/mgb4_cmt.c
> > index 70dc78ef193c..a25b68403bc6 100644
> > --- a/drivers/media/pci/mgb4/mgb4_cmt.c
> > +++ b/drivers/media/pci/mgb4/mgb4_cmt.c
> > @@ -227,6 +227,8 @@ void mgb4_cmt_set_vin_freq_range(struct mgb4_vin_dev *vindev,
> >   	u32 config;
> >   	size_t i;
> >   
> > +	freq_range = array_index_nospec(freq_range, ARRAY_SIZE(cmt_vals_in));
> > +
> >   	addr = cmt_addrs_in[vindev->config->id];
> >   	reg_set = cmt_vals_in[freq_range];
> >     
> 
> I still do not fully understand the exact vulnerability here, but the 
> patch should definitely not do any harm, so I'm ok with it even if it's 
> real purpose would only be to silence the smatch warning :-)

With Spectre, just checking if freq_range is between 0 and the
size of the array is not enough, as malicious code could use CPU
speculative logic to retrieve data from memory outside the limits
of the array.

As freq_range is specified by the user via sysfs attribute
frequency_range, it is subject to Spectre v1 attack as described
at Documentation/admin-guide/hw-vuln/spectre.rst. 

Silencing smatch is a plus.

> 
> Reviewed-by: Martin Tůma <martin.tuma@digiteqautomotive.com>

Thanks!

Thanks,
Mauro

Re: [PATCH 05/13] media: mgb4: protect driver against spectre

[Martin Tůma]

Hi,

On 18. 10. 24 6:32 dop., Mauro Carvalho Chehab wrote:
> Em Wed, 16 Oct 2024 13:59:18 +0200
> Martin Tůma <tumic@gpxsee.org> escreveu:
> 
>> On 16. 10. 24 12:22 odp., Mauro Carvalho Chehab wrote:
>>> Frequency range is set from sysfs via frequency_range_store(),
>>> being vulnerable to spectre, as reported by smatch:
>>>
>>> 	drivers/media/pci/mgb4/mgb4_cmt.c:231 mgb4_cmt_set_vin_freq_range() warn: potential spectre issue 'cmt_vals_in' [r]
>>> 	drivers/media/pci/mgb4/mgb4_cmt.c:238 mgb4_cmt_set_vin_freq_range() warn: possible spectre second half.  'reg_set'
>>>
>>> Fix it.
>>>
>>> Fixes: 0ab13674a9bd ("media: pci: mgb4: Added Digiteq Automotive MGB4 driver")
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
>>> ---
>>>    drivers/media/pci/mgb4/mgb4_cmt.c | 2 ++
>>>    1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/media/pci/mgb4/mgb4_cmt.c b/drivers/media/pci/mgb4/mgb4_cmt.c
>>> index 70dc78ef193c..a25b68403bc6 100644
>>> --- a/drivers/media/pci/mgb4/mgb4_cmt.c
>>> +++ b/drivers/media/pci/mgb4/mgb4_cmt.c
>>> @@ -227,6 +227,8 @@ void mgb4_cmt_set_vin_freq_range(struct mgb4_vin_dev *vindev,
>>>    	u32 config;
>>>    	size_t i;
>>>    
>>> +	freq_range = array_index_nospec(freq_range, ARRAY_SIZE(cmt_vals_in));
>>> +
>>>    	addr = cmt_addrs_in[vindev->config->id];
>>>    	reg_set = cmt_vals_in[freq_range];
>>>      
>>
>> I still do not fully understand the exact vulnerability here, but the
>> patch should definitely not do any harm, so I'm ok with it even if it's
>> real purpose would only be to silence the smatch warning :-)
> 
> With Spectre, just checking if freq_range is between 0 and the
> size of the array is not enough, as malicious code could use CPU
> speculative logic to retrieve data from memory outside the limits
> of the array.
> 
> As freq_range is specified by the user via sysfs attribute
> frequency_range, it is subject to Spectre v1 attack as described
> at Documentation/admin-guide/hw-vuln/spectre.rst.
> 

I do know the general theory about the "spectre bounds check 
fix/workaround", what I was referring is this part in the documentation:

"Such speculative memory accesses can leave side effects, creating side 
channels which leak information to the attacker."

I do not see/understand the exact "information leak" that could happen 
here on this particular place. But as already stated in the original 
answer, I don't have to understand everything ;-)

M.

> Silencing smatch is a plus.
> 
>>
>> Reviewed-by: Martin Tůma <martin.tuma@digiteqautomotive.com>
> 
> Thanks!
> 
> Thanks,
> Mauro
> 

[PATCH 06/13] media: av7110: fix a spectre vulnerability

[Mauro Carvalho Chehab]

As warned by smatch:
	drivers/staging/media/av7110/av7110_ca.c:270 dvb_ca_ioctl() warn: potential spectre issue 'av7110->ci_slot' [w] (local cap)

There is a spectre-related vulnerability at the code. Fix it.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/staging/media/av7110/av7110.h    |  4 +++-
 drivers/staging/media/av7110/av7110_ca.c | 25 ++++++++++++++++--------
 2 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/media/av7110/av7110.h b/drivers/staging/media/av7110/av7110.h
index ec461fd187af..b584754f4be0 100644
--- a/drivers/staging/media/av7110/av7110.h
+++ b/drivers/staging/media/av7110/av7110.h
@@ -88,6 +88,8 @@ struct infrared {
 	u32			ir_config;
 };
 
+#define MAX_CI_SLOTS	2
+
 /* place to store all the necessary device information */
 struct av7110 {
 	/* devices */
@@ -163,7 +165,7 @@ struct av7110 {
 
 	/* CA */
 
-	struct ca_slot_info	ci_slot[2];
+	struct ca_slot_info	ci_slot[MAX_CI_SLOTS];
 
 	enum av7110_video_mode	vidmode;
 	struct dmxdev		dmxdev;
diff --git a/drivers/staging/media/av7110/av7110_ca.c b/drivers/staging/media/av7110/av7110_ca.c
index 6ce212c64e5d..fce4023c9dea 100644
--- a/drivers/staging/media/av7110/av7110_ca.c
+++ b/drivers/staging/media/av7110/av7110_ca.c
@@ -26,23 +26,28 @@
 
 void CI_handle(struct av7110 *av7110, u8 *data, u16 len)
 {
+	unsigned slot_num;
+
 	dprintk(8, "av7110:%p\n", av7110);
 
 	if (len < 3)
 		return;
 	switch (data[0]) {
 	case CI_MSG_CI_INFO:
-		if (data[2] != 1 && data[2] != 2)
+		if (data[2] != 1 && data[2] != MAX_CI_SLOTS)
 			break;
+
+		slot_num = array_index_nospec(data[2] - 1, MAX_CI_SLOTS);
+
 		switch (data[1]) {
 		case 0:
-			av7110->ci_slot[data[2] - 1].flags = 0;
+			av7110->ci_slot[slot_num].flags = 0;
 			break;
 		case 1:
-			av7110->ci_slot[data[2] - 1].flags |= CA_CI_MODULE_PRESENT;
+			av7110->ci_slot[slot_num].flags |= CA_CI_MODULE_PRESENT;
 			break;
 		case 2:
-			av7110->ci_slot[data[2] - 1].flags |= CA_CI_MODULE_READY;
+			av7110->ci_slot[slot_num].flags |= CA_CI_MODULE_READY;
 			break;
 		}
 		break;
@@ -262,15 +267,19 @@ static int dvb_ca_ioctl(struct file *file, unsigned int cmd, void *parg)
 	case CA_GET_SLOT_INFO:
 	{
 		struct ca_slot_info *info = (struct ca_slot_info *)parg;
+		unsigned int slot_num;
 
 		if (info->num < 0 || info->num > 1) {
 			mutex_unlock(&av7110->ioctl_mutex);
 			return -EINVAL;
 		}
-		av7110->ci_slot[info->num].num = info->num;
-		av7110->ci_slot[info->num].type = FW_CI_LL_SUPPORT(av7110->arm_app) ?
-							CA_CI_LINK : CA_CI;
-		memcpy(info, &av7110->ci_slot[info->num], sizeof(struct ca_slot_info));
+		slot_num = array_index_nospec(info->num, MAX_CI_SLOTS);
+
+		av7110->ci_slot[slot_num].num = info->num;
+		av7110->ci_slot[slot_num].type = FW_CI_LL_SUPPORT(av7110->arm_app) ?
+						 CA_CI_LINK : CA_CI;
+		memcpy(info, &av7110->ci_slot[slot_num],
+		       sizeof(struct ca_slot_info));
 		break;
 	}
 
-- 
2.47.0

[PATCH 07/13] media: s5p-jpeg: prevent buffer overflows

[Mauro Carvalho Chehab]

The current logic allows word to be less than 2. If this happens,
there will be buffer overflows. Add extra checks to prevent it.

While here, remove an unused word = 0 assignment.

Fixes: 6c96dbbc2aa9 ("[media] s5p-jpeg: add support for 5433")
Cc: stable@vger.kernel.org
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 .../media/platform/samsung/s5p-jpeg/jpeg-core.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/samsung/s5p-jpeg/jpeg-core.c b/drivers/media/platform/samsung/s5p-jpeg/jpeg-core.c
index d2c4a0178b3c..1db4609b3557 100644
--- a/drivers/media/platform/samsung/s5p-jpeg/jpeg-core.c
+++ b/drivers/media/platform/samsung/s5p-jpeg/jpeg-core.c
@@ -775,11 +775,14 @@ static void exynos4_jpeg_parse_decode_h_tbl(struct s5p_jpeg_ctx *ctx)
 		(unsigned long)vb2_plane_vaddr(&vb->vb2_buf, 0) + ctx->out_q.sos + 2;
 	jpeg_buffer.curr = 0;
 
-	word = 0;
-
 	if (get_word_be(&jpeg_buffer, &word))
 		return;
-	jpeg_buffer.size = (long)word - 2;
+
+	if (word < 2)
+		jpeg_buffer.size = 0;
+	else
+		jpeg_buffer.size = (long)word - 2;
+
 	jpeg_buffer.data += 2;
 	jpeg_buffer.curr = 0;
 
@@ -1058,6 +1061,7 @@ static int get_word_be(struct s5p_jpeg_buffer *buf, unsigned int *word)
 	if (byte == -1)
 		return -1;
 	*word = (unsigned int)byte | temp;
+
 	return 0;
 }
 
@@ -1145,7 +1149,7 @@ static bool s5p_jpeg_parse_hdr(struct s5p_jpeg_q_data *result,
 			if (get_word_be(&jpeg_buffer, &word))
 				break;
 			length = (long)word - 2;
-			if (!length)
+			if (length <= 0)
 				return false;
 			sof = jpeg_buffer.curr; /* after 0xffc0 */
 			sof_len = length;
@@ -1176,7 +1180,7 @@ static bool s5p_jpeg_parse_hdr(struct s5p_jpeg_q_data *result,
 			if (get_word_be(&jpeg_buffer, &word))
 				break;
 			length = (long)word - 2;
-			if (!length)
+			if (length <= 0)
 				return false;
 			if (n_dqt >= S5P_JPEG_MAX_MARKER)
 				return false;
@@ -1189,7 +1193,7 @@ static bool s5p_jpeg_parse_hdr(struct s5p_jpeg_q_data *result,
 			if (get_word_be(&jpeg_buffer, &word))
 				break;
 			length = (long)word - 2;
-			if (!length)
+			if (length <= 0)
 				return false;
 			if (n_dht >= S5P_JPEG_MAX_MARKER)
 				return false;
@@ -1214,6 +1218,7 @@ static bool s5p_jpeg_parse_hdr(struct s5p_jpeg_q_data *result,
 			if (get_word_be(&jpeg_buffer, &word))
 				break;
 			length = (long)word - 2;
+			/* No need to check underflows as skip() does it  */
 			skip(&jpeg_buffer, length);
 			break;
 		}
-- 
2.47.0

Re: [PATCH 07/13] media: s5p-jpeg: prevent buffer overflows

[Jacek Anaszewski]

Hi Mauro,

On 10/16/24 12:22, Mauro Carvalho Chehab wrote:
> The current logic allows word to be less than 2. If this happens,
> there will be buffer overflows. Add extra checks to prevent it.
> 
> While here, remove an unused word = 0 assignment.
> 
> Fixes: 6c96dbbc2aa9 ("[media] s5p-jpeg: add support for 5433")
> Cc: stable@vger.kernel.org
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
>   .../media/platform/samsung/s5p-jpeg/jpeg-core.c | 17 +++++++++++------
>   1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/samsung/s5p-jpeg/jpeg-core.c b/drivers/media/platform/samsung/s5p-jpeg/jpeg-core.c
> index d2c4a0178b3c..1db4609b3557 100644
> --- a/drivers/media/platform/samsung/s5p-jpeg/jpeg-core.c
> +++ b/drivers/media/platform/samsung/s5p-jpeg/jpeg-core.c
> @@ -775,11 +775,14 @@ static void exynos4_jpeg_parse_decode_h_tbl(struct s5p_jpeg_ctx *ctx)
>   		(unsigned long)vb2_plane_vaddr(&vb->vb2_buf, 0) + ctx->out_q.sos + 2;
>   	jpeg_buffer.curr = 0;
>   
> -	word = 0;
> -
>   	if (get_word_be(&jpeg_buffer, &word))
>   		return;
> -	jpeg_buffer.size = (long)word - 2;
> +
> +	if (word < 2)
> +		jpeg_buffer.size = 0;
> +	else
> +		jpeg_buffer.size = (long)word - 2;
> +
>   	jpeg_buffer.data += 2;
>   	jpeg_buffer.curr = 0;
>   
> @@ -1058,6 +1061,7 @@ static int get_word_be(struct s5p_jpeg_buffer *buf, unsigned int *word)
>   	if (byte == -1)
>   		return -1;
>   	*word = (unsigned int)byte | temp;
> +
>   	return 0;
>   }
>   
> @@ -1145,7 +1149,7 @@ static bool s5p_jpeg_parse_hdr(struct s5p_jpeg_q_data *result,
>   			if (get_word_be(&jpeg_buffer, &word))
>   				break;
>   			length = (long)word - 2;
> -			if (!length)
> +			if (length <= 0)
>   				return false;
>   			sof = jpeg_buffer.curr; /* after 0xffc0 */
>   			sof_len = length;
> @@ -1176,7 +1180,7 @@ static bool s5p_jpeg_parse_hdr(struct s5p_jpeg_q_data *result,
>   			if (get_word_be(&jpeg_buffer, &word))
>   				break;
>   			length = (long)word - 2;
> -			if (!length)
> +			if (length <= 0)
>   				return false;
>   			if (n_dqt >= S5P_JPEG_MAX_MARKER)
>   				return false;
> @@ -1189,7 +1193,7 @@ static bool s5p_jpeg_parse_hdr(struct s5p_jpeg_q_data *result,
>   			if (get_word_be(&jpeg_buffer, &word))
>   				break;
>   			length = (long)word - 2;
> -			if (!length)
> +			if (length <= 0)
>   				return false;
>   			if (n_dht >= S5P_JPEG_MAX_MARKER)
>   				return false;
> @@ -1214,6 +1218,7 @@ static bool s5p_jpeg_parse_hdr(struct s5p_jpeg_q_data *result,
>   			if (get_word_be(&jpeg_buffer, &word))
>   				break;
>   			length = (long)word - 2;
> +			/* No need to check underflows as skip() does it  */
>   			skip(&jpeg_buffer, length);
>   			break;
>   		}

Seems reasonable.

Reviewed-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>

-- 
Best regards,
Jacek Anaszewski

[PATCH 08/13] media: ar0521: don't overflow when checking PLL values

[Mauro Carvalho Chehab]

The PLL checks are comparing 64 bit integers with 32 bit
ones. Depending on the values of the variables, this may
underflow.

Fix it ensuring that both sides of the expression are u64.

Fixes: 852b50aeed15 ("media: On Semi AR0521 sensor driver")
Cc: stable@vger.kernel.org
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/i2c/ar0521.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
index fc27238dd4d3..24873149096c 100644
--- a/drivers/media/i2c/ar0521.c
+++ b/drivers/media/i2c/ar0521.c
@@ -255,10 +255,10 @@ static u32 calc_pll(struct ar0521_dev *sensor, u32 freq, u16 *pre_ptr, u16 *mult
 			continue; /* Minimum value */
 		if (new_mult > 254)
 			break; /* Maximum, larger pre won't work either */
-		if (sensor->extclk_freq * (u64)new_mult < AR0521_PLL_MIN *
+		if (sensor->extclk_freq * (u64)new_mult < (u64)AR0521_PLL_MIN *
 		    new_pre)
 			continue;
-		if (sensor->extclk_freq * (u64)new_mult > AR0521_PLL_MAX *
+		if (sensor->extclk_freq * (u64)new_mult > (u64)AR0521_PLL_MAX *
 		    new_pre)
 			break; /* Larger pre won't work either */
 		new_pll = div64_round_up(sensor->extclk_freq * (u64)new_mult,
-- 
2.47.0

Re: [PATCH 08/13] media: ar0521: don't overflow when checking PLL values

[Sakari Ailus]

Hi Mauro,

On Wed, Oct 16, 2024 at 12:22:24PM +0200, Mauro Carvalho Chehab wrote:
> The PLL checks are comparing 64 bit integers with 32 bit
> ones. Depending on the values of the variables, this may
> underflow.
> 
> Fix it ensuring that both sides of the expression are u64.
> 
> Fixes: 852b50aeed15 ("media: On Semi AR0521 sensor driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
>  drivers/media/i2c/ar0521.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
> index fc27238dd4d3..24873149096c 100644
> --- a/drivers/media/i2c/ar0521.c
> +++ b/drivers/media/i2c/ar0521.c
> @@ -255,10 +255,10 @@ static u32 calc_pll(struct ar0521_dev *sensor, u32 freq, u16 *pre_ptr, u16 *mult
>  			continue; /* Minimum value */
>  		if (new_mult > 254)
>  			break; /* Maximum, larger pre won't work either */
> -		if (sensor->extclk_freq * (u64)new_mult < AR0521_PLL_MIN *
> +		if (sensor->extclk_freq * (u64)new_mult < (u64)AR0521_PLL_MIN *
>  		    new_pre)
>  			continue;
> -		if (sensor->extclk_freq * (u64)new_mult > AR0521_PLL_MAX *
> +		if (sensor->extclk_freq * (u64)new_mult > (u64)AR0521_PLL_MAX *
>  		    new_pre)
>  			break; /* Larger pre won't work either */
>  		new_pll = div64_round_up(sensor->extclk_freq * (u64)new_mult,

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Sakari Ailus

[PATCH 09/13] media: cx24116: prevent overflows on SNR calculus

[Mauro Carvalho Chehab]

If reading SNR registers fail, a negative number will be
returned, causing an underflow when reading SNR registers.

Prevent that.

Fixes: 8953db793d5b ("V4L/DVB (9178): cx24116: Add module parameter to return SNR as ESNO.")
Cc: stable@vger.kernel.org
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/dvb-frontends/cx24116.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/media/dvb-frontends/cx24116.c b/drivers/media/dvb-frontends/cx24116.c
index 8b978a9f74a4..f5dd3a81725a 100644
--- a/drivers/media/dvb-frontends/cx24116.c
+++ b/drivers/media/dvb-frontends/cx24116.c
@@ -741,6 +741,7 @@ static int cx24116_read_snr_pct(struct dvb_frontend *fe, u16 *snr)
 {
 	struct cx24116_state *state = fe->demodulator_priv;
 	u8 snr_reading;
+	int ret;
 	static const u32 snr_tab[] = { /* 10 x Table (rounded up) */
 		0x00000, 0x0199A, 0x03333, 0x04ccD, 0x06667,
 		0x08000, 0x0999A, 0x0b333, 0x0cccD, 0x0e667,
@@ -749,7 +750,11 @@ static int cx24116_read_snr_pct(struct dvb_frontend *fe, u16 *snr)
 
 	dprintk("%s()\n", __func__);
 
-	snr_reading = cx24116_readreg(state, CX24116_REG_QUALITY0);
+	ret = cx24116_readreg(state, CX24116_REG_QUALITY0);
+	if (ret  < 0)
+		return ret;
+
+	snr_reading = ret;
 
 	if (snr_reading >= 0xa0 /* 100% */)
 		*snr = 0xffff;
-- 
2.47.0

[PATCH 10/13] media: adv7604 prevent underflow condition when reporting colorspace

[Mauro Carvalho Chehab]

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@vger.kernel.org
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 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]);
 
-- 
2.47.0

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

[Hans Verkuil]

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@vger.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.

> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

Reviewed-by: Hans Verkuil <hverkuil@xs4all.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]);
>  

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

[Mauro Carvalho Chehab]

Em Wed, 16 Oct 2024 12:57:53 +0200
Hans Verkuil <hverkuil@xs4all.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@vger.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.

-

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.

> 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>  
> 
> Reviewed-by: Hans Verkuil <hverkuil@xs4all.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

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

[Hans Verkuil]

On 16/10/2024 13:24, Mauro Carvalho Chehab wrote:
> Em Wed, 16 Oct 2024 12:57:53 +0200
> Hans Verkuil <hverkuil@xs4all.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@vger.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@kernel.org>  
>>
>> Reviewed-by: Hans Verkuil <hverkuil@xs4all.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

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

[Mauro Carvalho Chehab]

Em Wed, 16 Oct 2024 13:58:48 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On 16/10/2024 13:24, Mauro Carvalho Chehab wrote:
> > Em Wed, 16 Oct 2024 12:57:53 +0200
> > Hans Verkuil <hverkuil@xs4all.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@vger.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.

Sure, logs can change, but this is an user-visible bug.

> 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.

Agreed. Will drop cc stable.

> > 
> > -
> > 
> > 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 :-)

We can't count on it. Civil infrastructure is now working with a 10 years
SLTS:

	https://www.linuxfoundation.org/press/civil-infrastructure-platform-expands-slt-stable-kernel-program

I heard somewhere that having a 15 years or 20 years stable Kernel is a
need for certain usages.

Even commercial distros have a minimum of 10 years as LTS.

Suse is now working with a 13-years support. Both Canonical and Red Hat
announced a 12-years ELTS support. As they usually takes the last year's 
LTS Kernel, it means support will end with a 14 years old Kernel (so, 
support will end in 2037 or 2038 if they release an LTS distro next year),
and don't decide to extend it further.

I also heard during LPC that there's an increased pressure from Linux
customers from commercial distros to extend it even further.


Thanks,
Mauro

[PATCH 11/13] media: stb0899_algo: initialize cfr before using it

[Mauro Carvalho Chehab]

The loop at stb0899_search_carrier() starts with a random
value for cfr. Initialize it to zero, just like
stb0899_dvbs_algo() to ensure that carrier search won't
bail out.

Fixes: 8bd135bab91f ("V4L/DVB (9375): Add STB0899 support")
Cc: stable@vger.kernel.org
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/dvb-frontends/stb0899_algo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/dvb-frontends/stb0899_algo.c b/drivers/media/dvb-frontends/stb0899_algo.c
index df89c33dac23..40537c4ccb0d 100644
--- a/drivers/media/dvb-frontends/stb0899_algo.c
+++ b/drivers/media/dvb-frontends/stb0899_algo.c
@@ -269,7 +269,7 @@ static enum stb0899_status stb0899_search_carrier(struct stb0899_state *state)
 
 	short int derot_freq = 0, last_derot_freq = 0, derot_limit, next_loop = 3;
 	int index = 0;
-	u8 cfr[2];
+	u8 cfr[2] = {0};
 	u8 reg;
 
 	internal->status = NOCARRIER;
-- 
2.47.0

[PATCH 12/13] media: cec: extron-da-hd-4k-plus: don't use -1 as an error code

[Mauro Carvalho Chehab]

The logic at get_edid_tag_location() returns either an offset
or an error condition. However, the error condition uses a
non-standard "-1" value.

Use instead -ENOENT to indicate that the tag was not found.

Fixes: 056f2821b631 ("media: cec: extron-da-hd-4k-plus: add the Extron DA HD 4K Plus CEC driver")
Cc: stable@vger.kernel.org
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 .../cec/usb/extron-da-hd-4k-plus/extron-da-hd-4k-plus.c     | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/cec/usb/extron-da-hd-4k-plus/extron-da-hd-4k-plus.c b/drivers/media/cec/usb/extron-da-hd-4k-plus/extron-da-hd-4k-plus.c
index a526464af88c..7d03a36df5cf 100644
--- a/drivers/media/cec/usb/extron-da-hd-4k-plus/extron-da-hd-4k-plus.c
+++ b/drivers/media/cec/usb/extron-da-hd-4k-plus/extron-da-hd-4k-plus.c
@@ -348,12 +348,12 @@ static int get_edid_tag_location(const u8 *edid, unsigned int size,
 
 	/* Return if not a CTA-861 extension block */
 	if (size < 256 || edid[0] != 0x02 || edid[1] != 0x03)
-		return -1;
+		return -ENOENT;
 
 	/* search tag */
 	d = edid[0x02] & 0x7f;
 	if (d <= 4)
-		return -1;
+		return -ENOENT;
 
 	i = 0x04;
 	end = 0x00 + d;
@@ -371,7 +371,7 @@ static int get_edid_tag_location(const u8 *edid, unsigned int size,
 			return offset + i;
 		i += len + 1;
 	} while (i < end);
-	return -1;
+	return -ENOENT;
 }
 
 static void extron_edid_crc(u8 *edid)
-- 
2.47.0

Re: [PATCH 12/13] media: cec: extron-da-hd-4k-plus: don't use -1 as an error code

[Hans Verkuil]

On 16/10/2024 12:22, Mauro Carvalho Chehab wrote:
> The logic at get_edid_tag_location() returns either an offset
> or an error condition. However, the error condition uses a
> non-standard "-1" value.
> 
> Use instead -ENOENT to indicate that the tag was not found.
> 
> Fixes: 056f2821b631 ("media: cec: extron-da-hd-4k-plus: add the Extron DA HD 4K Plus CEC driver")

Not a fix, since it isn't broken. It is returning an offset, and -1 is used if it isn't
found. It's fine to change it to -ENOENT since the code calling it just checks if the
result > 0.

So it is a slight improvement of the code, but not a fix.

> Cc: stable@vger.kernel.org

So this isn't needed either.

Regards,

	Hans

> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
>  .../cec/usb/extron-da-hd-4k-plus/extron-da-hd-4k-plus.c     | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/cec/usb/extron-da-hd-4k-plus/extron-da-hd-4k-plus.c b/drivers/media/cec/usb/extron-da-hd-4k-plus/extron-da-hd-4k-plus.c
> index a526464af88c..7d03a36df5cf 100644
> --- a/drivers/media/cec/usb/extron-da-hd-4k-plus/extron-da-hd-4k-plus.c
> +++ b/drivers/media/cec/usb/extron-da-hd-4k-plus/extron-da-hd-4k-plus.c
> @@ -348,12 +348,12 @@ static int get_edid_tag_location(const u8 *edid, unsigned int size,
>  
>  	/* Return if not a CTA-861 extension block */
>  	if (size < 256 || edid[0] != 0x02 || edid[1] != 0x03)
> -		return -1;
> +		return -ENOENT;
>  
>  	/* search tag */
>  	d = edid[0x02] & 0x7f;
>  	if (d <= 4)
> -		return -1;
> +		return -ENOENT;
>  
>  	i = 0x04;
>  	end = 0x00 + d;
> @@ -371,7 +371,7 @@ static int get_edid_tag_location(const u8 *edid, unsigned int size,
>  			return offset + i;
>  		i += len + 1;
>  	} while (i < end);
> -	return -1;
> +	return -ENOENT;
>  }
>  
>  static void extron_edid_crc(u8 *edid)

[PATCH 13/13] media: pulse8-cec: fix data timestamp at pulse8_setup()

[Mauro Carvalho Chehab]

There is a hidden overflow condition there. As date is signed
and u8 is unsigned, doing:

	date = (data[0] << 24)

With a value bigger than 07f will make all upper bits of date
0xffffffff. This can be demonstrated with this small code:

<code>

typedef int64_t time64_t;
typedef uint8_t u8;

int main(void)
{
	u8 data[] = { 0xde ,0xad , 0xbe, 0xef };
	time64_t date;

	date = (data[0] << 24) | (data[1] << 16) | (data[2] << 8) | data[3];
	printf("Invalid data = 0x%08lx\n", date);

	date = ((unsigned)data[0] << 24) | (data[1] << 16) | (data[2] << 8) | data[3];
	printf("Expected data = 0x%08lx\n", date);

	return 0;
}
</code>

Fix it by converting the upper bit calculation to unsigned.

Fixes: cea28e7a55e7 ("media: pulse8-cec: reorganize function order")
Cc: stable@vger.kernel.org
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/cec/usb/pulse8/pulse8-cec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/cec/usb/pulse8/pulse8-cec.c b/drivers/media/cec/usb/pulse8/pulse8-cec.c
index ba67587bd43e..171366fe3544 100644
--- a/drivers/media/cec/usb/pulse8/pulse8-cec.c
+++ b/drivers/media/cec/usb/pulse8/pulse8-cec.c
@@ -685,7 +685,7 @@ static int pulse8_setup(struct pulse8 *pulse8, struct serio *serio,
 	err = pulse8_send_and_wait(pulse8, cmd, 1, cmd[0], 4);
 	if (err)
 		return err;
-	date = (data[0] << 24) | (data[1] << 16) | (data[2] << 8) | data[3];
+	date = ((unsigned)data[0] << 24) | (data[1] << 16) | (data[2] << 8) | data[3];
 	dev_info(pulse8->dev, "Firmware build date %ptT\n", &date);
 
 	dev_dbg(pulse8->dev, "Persistent config:\n");
-- 
2.47.0

Re: [PATCH 13/13] media: pulse8-cec: fix data timestamp at pulse8_setup()

[Hans Verkuil]

On 16/10/2024 12:22, Mauro Carvalho Chehab wrote:
> There is a hidden overflow condition there. As date is signed
> and u8 is unsigned, doing:
> 
> 	date = (data[0] << 24)
> 
> With a value bigger than 07f will make all upper bits of date
> 0xffffffff. This can be demonstrated with this small code:
> 
> <code>
> 
> typedef int64_t time64_t;
> typedef uint8_t u8;
> 
> int main(void)
> {
> 	u8 data[] = { 0xde ,0xad , 0xbe, 0xef };
> 	time64_t date;
> 
> 	date = (data[0] << 24) | (data[1] << 16) | (data[2] << 8) | data[3];
> 	printf("Invalid data = 0x%08lx\n", date);
> 
> 	date = ((unsigned)data[0] << 24) | (data[1] << 16) | (data[2] << 8) | data[3];
> 	printf("Expected data = 0x%08lx\n", date);
> 
> 	return 0;
> }
> </code>
> 
> Fix it by converting the upper bit calculation to unsigned.
> 
> Fixes: cea28e7a55e7 ("media: pulse8-cec: reorganize function order")
> Cc: stable@vger.kernel.org

Not a fix either, just an improvement. The worst that can happen is that in 2038
the wrong date is shown, provided they release new firmware for this device in
that year :-)

Regards,

	Hans

> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
>  drivers/media/cec/usb/pulse8/pulse8-cec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/cec/usb/pulse8/pulse8-cec.c b/drivers/media/cec/usb/pulse8/pulse8-cec.c
> index ba67587bd43e..171366fe3544 100644
> --- a/drivers/media/cec/usb/pulse8/pulse8-cec.c
> +++ b/drivers/media/cec/usb/pulse8/pulse8-cec.c
> @@ -685,7 +685,7 @@ static int pulse8_setup(struct pulse8 *pulse8, struct serio *serio,
>  	err = pulse8_send_and_wait(pulse8, cmd, 1, cmd[0], 4);
>  	if (err)
>  		return err;
> -	date = (data[0] << 24) | (data[1] << 16) | (data[2] << 8) | data[3];
> +	date = ((unsigned)data[0] << 24) | (data[1] << 16) | (data[2] << 8) | data[3];
>  	dev_info(pulse8->dev, "Firmware build date %ptT\n", &date);
>  
>  	dev_dbg(pulse8->dev, "Persistent config:\n");