Skip to contentSkip to conversation

Media: fix several issues on drivers

PATCH In review Mauro Carvalho Chehab 6 participants 22 messages Oct 18, 2024 – Oct 21, 2024

Subsystems (11)
lore
Share & tools
lore
#0 lore raw

v2 cover Media: fix several issues on drivers

There are a number of issues that aren't passing on smatch or Coverity.

Address some of them.

Mauro Carvalho Chehab (13):
media: v4l2-ctrls-api: fix error handling for v4l2_g_ctrl()
media: v4l2-tpg: prevent the risk of a division by zero
media: dvbdev: prevent the risk of out of memory access
media: dvb_frontend: don't play tricks with underflow values
media: mgb4: protect driver against spectre
media: av7110: fix a spectre vulnerability
media: s5p-jpeg: prevent buffer overflows
media: ar0521: don't overflow when checking PLL values
media: cx24116: prevent overflows on SNR calculus
media: adv7604: prevent underflow condition when reporting colorspace
media: stb0899_algo: initialize cfr before using it
media: cec: extron-da-hd-4k-plus: don't use -1 as an error code
media: pulse8-cec: fix data timestamp at pulse8_setup()

 .../extron-da-hd-4k-plus.c                    |  6 ++---
 drivers/media/cec/usb/pulse8/pulse8-cec.c     |  2 +-
 drivers/media/common/v4l2-tpg/v4l2-tpg-core.c |  3 +++
 drivers/media/dvb-core/dvb_frontend.c         |  4 +--
 drivers/media/dvb-core/dvbdev.c               | 17 ++++++++++--
 drivers/media/dvb-frontends/cx24116.c         |  7 ++++-
 drivers/media/dvb-frontends/stb0899_algo.c    |  2 +-
 drivers/media/i2c/adv7604.c                   | 26 ++++++++++++-------
 drivers/media/i2c/ar0521.c                    |  4 +--
 drivers/media/pci/mgb4/mgb4_cmt.c             |  2 ++
 .../platform/samsung/s5p-jpeg/jpeg-core.c     | 17 +++++++-----
 drivers/media/v4l2-core/v4l2-ctrls-api.c      | 16 ++++++++----
 drivers/staging/media/av7110/av7110.h         |  4 ++-
 drivers/staging/media/av7110/av7110_ca.c      | 25 ++++++++++++------
 14 files changed, 94 insertions(+), 41 deletions(-)
Signature · 1 line
2.47.0
#1 lore raw

PATCH v2 1/13 media: v4l2-ctrls-api: fix error handling for v4l2_g_ctrl()

As detected by Coverity, 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")
---
 drivers/media/v4l2-core/v4l2-ctrls-api.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)
drivers/media/v4l2-core/v4l2-ctrls-api.c +11 -5
index e5a364efd5e6..a0de7eeaf085 100644
@@ -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);
@@ -811,10 +815,12 @@ static int set_ctrl_lock(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl,
 	int ret;
 
 	v4l2_ctrl_lock(ctrl);
-	user_to_new(c, ctrl);
+	ret = user_to_new(c, ctrl);
+	if (ret)
+		return ret;
 	ret = set_ctrl(fh, ctrl, 0);
 	if (!ret)
-		cur_to_user(c, ctrl);
+		ret = cur_to_user(c, ctrl);
 	v4l2_ctrl_unlock(ctrl);
 	return ret;
 }
Signature · 1 line
2.47.0
#2 lore raw

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

Quoted 46 lines · Mauro Carvalho Chehab

As detected by Coverity, 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")
---
 drivers/media/v4l2-core/v4l2-ctrls-api.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)
drivers/media/v4l2-core/v4l2-ctrls-api.c +7 -3
index e5a364efd5e6..a0de7eeaf085 100644
@@ -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);

drivers/media/v4l2-core/v4l2-ctrls-api.c static int set_ctrl_lock(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, · line 820

Quoted 7 lines
drivers/media/v4l2-core/v4l2-ctrls-api.c +3 -1
@@ -811,10 +815,12 @@ static int set_ctrl_lock(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl,
 	int ret;
 
 	v4l2_ctrl_lock(ctrl);
-	user_to_new(c, ctrl);
+	ret = user_to_new(c, ctrl);
+	if (ret)
+		return ret;

A lock was taken above and that isn't unlocked here.

It is better to write this as:

	if (!ret)
		ret = set_ctrl(fh, ctrl, 0);

drivers/media/v4l2-core/v4l2-ctrls-api.c static int set_ctrl_lock(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, · line 826

Quoted 7 lines
drivers/media/v4l2-core/v4l2-ctrls-api.c +1 -1
 	ret = set_ctrl(fh, ctrl, 0);
 	if (!ret)
-		cur_to_user(c, ctrl);
+		ret = cur_to_user(c, ctrl);
 	v4l2_ctrl_unlock(ctrl);
 	return ret;
 }

Regards,

Hans

#3 lore raw

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

Em Fri, 18 Oct 2024 08:13:01 +0200
Hans Verkuil <[email protected]> escreveu:

Quoted 62 lines · Mauro Carvalho Chehab
Quoted 46 lines

As detected by Coverity, 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")
---
 drivers/media/v4l2-core/v4l2-ctrls-api.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)
drivers/media/v4l2-core/v4l2-ctrls-api.c +7 -3
index e5a364efd5e6..a0de7eeaf085 100644
@@ -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);

drivers/media/v4l2-core/v4l2-ctrls-api.c static int set_ctrl_lock(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, · line 820

Quoted 7 lines
drivers/media/v4l2-core/v4l2-ctrls-api.c +3 -1
@@ -811,10 +815,12 @@ static int set_ctrl_lock(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl,
 	int ret;
 
 	v4l2_ctrl_lock(ctrl);
-	user_to_new(c, ctrl);
+	ret = user_to_new(c, ctrl);
+	if (ret)
+		return ret;

A lock was taken above and that isn't unlocked here.

It is better to write this as:

	if (!ret)
		ret = set_ctrl(fh, ctrl, 0);
Quoted 7 lines

drivers/media/v4l2-core/v4l2-ctrls-api.c static int set_ctrl_lock(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, · line 826

Quoted 7 lines
drivers/media/v4l2-core/v4l2-ctrls-api.c +1 -1
 	ret = set_ctrl(fh, ctrl, 0);
 	if (!ret)
-		cur_to_user(c, ctrl);
+		ret = cur_to_user(c, ctrl);
 	v4l2_ctrl_unlock(ctrl);
 	return ret;
 }

True. See below.

Thanks,
Mauro

-

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

As detected by Coverity, 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")
---
 drivers/media/v4l2-core/v4l2-ctrls-api.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)
drivers/media/v4l2-core/v4l2-ctrls-api.c +11 -6
index e5a364efd5e6..95a2202879d8 100644
@@ -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);
@@ -811,10 +815,11 @@ static int set_ctrl_lock(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl,
 	int ret;
 
 	v4l2_ctrl_lock(ctrl);
-	user_to_new(c, ctrl);
-	ret = set_ctrl(fh, ctrl, 0);
+	ret = user_to_new(c, ctrl);
+	if (!ret)
+		ret = set_ctrl(fh, ctrl, 0);
 	if (!ret)
-		cur_to_user(c, ctrl);
+		ret = cur_to_user(c, ctrl);
 	v4l2_ctrl_unlock(ctrl);
 	return ret;
 }
Signature · 1 line
2.47.0
#4 lore raw

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

In reply to Mauro Carvalho Chehab

Hi Mauro,

kernel test robot noticed the following build warnings:

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Mauro-Carvalho-Chehab/media-v4l2-ctrls-api-fix-error-handling-for-v4l2_g_ctrl/20241018-181159
base:   https://git.linuxtv.org/media_stage.git master
patch link:    https://lore.kernel.org/r/4d36ccf66f48a870c9bc0f7e9fda595505c4d342.1729230718.git.mchehab%2Bhuawei%40kernel.org
patch subject: [PATCH v2 01/13] media: v4l2-ctrls-api: fix error handling for v4l2_g_ctrl()
config: i386-randconfig-141-20241021 (https://download.01.org/0day-ci/archive/20241022/[email protected]/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags

| Reported-by: kernel test robot <[email protected]>
| Reported-by: Dan Carpenter <[email protected]>
| Closes: https://lore.kernel.org/r/[email protected]/

smatch warnings:
drivers/media/v4l2-core/v4l2-ctrls-api.c:825 set_ctrl_lock() warn: inconsistent returns 'ctrl->handler->lock'.

vim +825 drivers/media/v4l2-core/v4l2-ctrls-api.c

71c689dc2e732d4 Hans Verkuil          2021-04-27  812  static int set_ctrl_lock(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl,
71c689dc2e732d4 Hans Verkuil          2021-04-27  813  			 struct v4l2_ext_control *c)
71c689dc2e732d4 Hans Verkuil          2021-04-27  814  {
71c689dc2e732d4 Hans Verkuil          2021-04-27  815  	int ret;
71c689dc2e732d4 Hans Verkuil          2021-04-27  816  
71c689dc2e732d4 Hans Verkuil          2021-04-27  817  	v4l2_ctrl_lock(ctrl);
54c5e2c49a4358b Mauro Carvalho Chehab 2024-10-18  818  	ret = user_to_new(c, ctrl);
54c5e2c49a4358b Mauro Carvalho Chehab 2024-10-18  819  	if (ret)
54c5e2c49a4358b Mauro Carvalho Chehab 2024-10-18  820  		return ret;

v4l2_ctrl_unlock() before returning

71c689dc2e732d4 Hans Verkuil          2021-04-27  821  	ret = set_ctrl(fh, ctrl, 0);
71c689dc2e732d4 Hans Verkuil          2021-04-27  822  	if (!ret)
54c5e2c49a4358b Mauro Carvalho Chehab 2024-10-18  823  		ret = cur_to_user(c, ctrl);
71c689dc2e732d4 Hans Verkuil          2021-04-27  824  	v4l2_ctrl_unlock(ctrl);
71c689dc2e732d4 Hans Verkuil          2021-04-27 @825  	return ret;
71c689dc2e732d4 Hans Verkuil          2021-04-27  826  }
Signature · 2 lines
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
#5 lore raw

PATCH v2 2/13 media: v4l2-tpg: prevent the risk of a division by zero

In reply to Mauro Carvalho Chehab

As reported by Coverity, 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_ONCE() to trigger such cases and return without doing any precalculation.

Fixes 63881df94d3e ("[media] vivid: add the Test Pattern Generator")
---
 drivers/media/common/v4l2-tpg/v4l2-tpg-core.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/media/common/v4l2-tpg/v4l2-tpg-core.c +3 -0
index c86343a4d0bf..940bfbf275ce 100644
@@ -1795,6 +1795,9 @@ static void tpg_precalculate_line(struct tpg_data *tpg)
 	unsigned p;
 	unsigned x;
 
+	if (WARN_ON_ONCE(!tpg->src_width || !tpg->scaled_width))
+		return;
+
 	switch (tpg->pattern) {
 	case TPG_PAT_GREEN:
 		contrast = TPG_COLOR_100_RED;
Signature · 1 line
2.47.0
#6 lore raw

PATCH v2 3/13 media: dvbdev: prevent the risk of out of memory access

In reply to 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 like Coverity.

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

Fixes 5dd3f3071070 ("V4L/DVB (9361): Dynamic DVB minor allocation")
---
 drivers/media/dvb-core/dvbdev.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)
drivers/media/dvb-core/dvbdev.c +15 -2
index b43695bc51e7..14f323fbada7 100644
@@ -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);
Signature · 1 line
2.47.0
#7 lore raw

PATCH v2 4/13 media: dvb_frontend: don't play tricks with underflow values

In reply to Mauro Carvalho Chehab

fepriv->auto_sub_step is unsigned. Setting it to -1 is just a trick to avoid calling continue, as reported by Coverity.

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")
---
 drivers/media/dvb-core/dvb_frontend.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/media/dvb-core/dvb_frontend.c +2 -2
index d48f48fda87c..c9283100332a 100644
@@ -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++;
Signature · 1 line
2.47.0
Reviewed-by on v2 4/13
#8 lore raw

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

Quoted 11 lines · Mauro Carvalho Chehab

fepriv->auto_sub_step is unsigned. Setting it to -1 is just a trick to avoid calling continue, as reported by Coverity.

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

Oh wow, back to the big-bang-commit ^^'

So is this a bug or not? It seems to me that the uint underflows to UINT_MAX, and then wrapps around to 0 again through the ++..

I take the liberty of ++CCing Kees, since I heard him talk a lot about overflowing on Plumbers.

If it's not a bug, I would not use "Fixes". If it is a bug, it should be backported to stable, agreed?

Plus, is there a report-link somewhere by Coverty that could be linked with "Closes: "?

Quoted 1 line

Anyways, this in my eyes does what it's intended to do:

Quoted 22 lines

---
 drivers/media/dvb-core/dvb_frontend.c | 4 ++--

 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/media/dvb-core/dvb_frontend.c +0 -0
index d48f48fda87c..c9283100332a 100644
@@ -443,8 +443,8 @@ static int dvb_frontend_swzigzag_autotune(struct

dvb_frontend *fe, int check_wra

  default:
  fepriv->auto_step++;

drivers/media/dvb-core/dvb_frontend.c +0 -1
-			fepriv->auto_sub_step = -1; /* it'll be

incremented to 0 in a moment */

drivers/media/dvb-core/dvb_frontend.c +2 -1
-			break;
+			fepriv->auto_sub_step = 0;
+			continue;

  }

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

#9 lore raw

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

Quoted 18 lines · Philipp Stanner
Quoted 11 lines · Mauro Carvalho Chehab

fepriv->auto_sub_step is unsigned. Setting it to -1 is just a trick to avoid calling continue, as reported by Coverity.

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

Oh wow, back to the big-bang-commit ^^'

So is this a bug or not? It seems to me that the uint underflows to UINT_MAX, and then wrapps around to 0 again through the ++..

Quoted 8 lines

I take the liberty of ++CCing Kees, since I heard him talk a lot about overflowing on Plumbers.

If it's not a bug, I would not use "Fixes". If it is a bug, it should be backported to stable, agreed?

Plus, is there a report-link somewhere by Coverty that could be linked with "Closes: "?

Yeah, this is "avoid currently harmless overflow" fix. It is just avoiding depending on the wrapping behavior, which is an improvement but not really a "bug fix"; more a code style that will keep future work of making the kernel wrapping-safe.

Quoted 21 lines
Quoted 2 lines

Anyways, this in my eyes does what it's intended to do:

Quoted 14 lines

---
 drivers/media/dvb-core/dvb_frontend.c | 4 ++--

 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/media/dvb-core/dvb_frontend.c +0 -0
index d48f48fda87c..c9283100332a 100644
@@ -443,8 +443,8 @@ static int dvb_frontend_swzigzag_autotune(struct

dvb_frontend *fe, int check_wra

  default:
  fepriv->auto_step++;

Quoted 9 lines
Quoted 8 lines
drivers/media/dvb-core/dvb_frontend.c +0 -1
-			fepriv->auto_sub_step = -1; /* it'll be

incremented to 0 in a moment */

drivers/media/dvb-core/dvb_frontend.c +2 -1
-			break;
+			fepriv->auto_sub_step = 0;
+			continue;

  }

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

But this change seems incomplete. The above line is no longer needed.

And I actually think this could be refractored to avoid needing "ready" at all?

-Kees

Signature · 1 line
Kees Cook
#10 lore raw

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

Quoted 30 lines · Kees Cook
Quoted 26 lines · Philipp Stanner
Quoted 11 lines · Mauro Carvalho Chehab

fepriv->auto_sub_step is unsigned. Setting it to -1 is just a trick to avoid calling continue, as reported by Coverity.

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

Oh wow, back to the big-bang-commit ^^'

So is this a bug or not? It seems to me that the uint underflows to UINT_MAX, and then wrapps around to 0 again through the ++..

I take the liberty of ++CCing Kees, since I heard him talk a lot about
overflowing on Plumbers.

If it's not a bug, I would not use "Fixes". If it is a bug, it should
be backported to stable, agreed?

Quoted 8 lines
Quoted 3 lines

Plus, is there a report-link somewhere by Coverty that could be linked
with "Closes: "?

Yeah, this is "avoid currently harmless overflow" fix. It is just avoiding depending on the wrapping behavior, which is an improvement but not really a "bug fix"; more a code style that will keep future work of making the kernel wrapping-safe.

Alright, then it shouldn't be backported, ack?
So I'd drop "Fixes:"

Quoted 33 lines
Quoted 23 lines
Quoted 3 lines

Anyways, this in my eyes does what it's intended to do:

Quoted 15 lines

---
 drivers/media/dvb-core/dvb_frontend.c | 4 ++--

 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/media/dvb-core/dvb_frontend.c +0 -0
index d48f48fda87c..c9283100332a 100644
@@ -443,8 +443,8 @@ static int

dvb_frontend_swzigzag_autotune(struct
dvb_frontend *fe, int check_wra

  default:
  fepriv->auto_step++;

Quoted 9 lines
Quoted 8 lines
drivers/media/dvb-core/dvb_frontend.c +0 -1
-			fepriv->auto_sub_step = -1; /* it'll be

incremented to 0 in a moment */

drivers/media/dvb-core/dvb_frontend.c +2 -1
-			break;
+			fepriv->auto_sub_step = 0;
+			continue;

  }

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

Quoted 1 line

But this change seems incomplete. The above line is no longer needed.

I haven't super duper intensively reviewed it, but wouldn't make that statement – all the other branches in the switch-case reach this line. And auto_sub_step might be changed above in the if-check again if lnb_drift has changed; and it is changed in the switch-case.

Quoted 3 lines

And I actually think this could be refractored to avoid needing "ready" at all?

Could be. But that'd be indeed some work to get it right without introducing a subtle bug, and Mauro just seems to want to fix a warning he encountered on the way.

Thx
P.

Quoted 3 lines

-Kees

#11 lore raw

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

In reply to Kees Cook

Em Fri, 18 Oct 2024 07:37:52 -0700
Kees Cook <[email protected]> escreveu:

Quoted 16 lines · Philipp Stanner
Quoted 15 lines · Mauro Carvalho Chehab
Quoted 11 lines

fepriv->auto_sub_step is unsigned. Setting it to -1 is just a trick to avoid calling continue, as reported by Coverity.

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

Oh wow, back to the big-bang-commit ^^'

Quoted 8 lines
Quoted 8 lines

So is this a bug or not? It seems to me that the uint underflows to UINT_MAX, and then wrapps around to 0 again through the ++..

I take the liberty of ++CCing Kees, since I heard him talk a lot about overflowing on Plumbers.

If it's not a bug, I would not use "Fixes". If it is a bug, it should be backported to stable, agreed?

There is a long thread about Fixes: tag at ksummit ML.

	https://lore.kernel.org/all/[email protected]/T/

My conclusions for it is that:

1. Fixes: != Cc: stable.
This is even somewhat stated at Documentation/process/stable-kernel-rules.rst when it defines additional rules for Cc: stable;

2. As result of (1), all Cc: stable need fixes, but not all fixes: need a Cc: stable. Btw, I double-checked it with a -stable maintainer (Greg);

3. It seems that most of people at ksummit discussion (including me) use Fixes: when the patch is not doing an improvement.

Quoted 2 lines
Quoted 2 lines

Plus, is there a report-link somewhere by Coverty that could be linked with "Closes: "?

Coverity issues are not publicly visible (and IMO it shouldn't). We should not add closes: to something that only the ones with access to it may see.

Quoted 1 line

Yeah, this is "avoid currently harmless overflow" fix. It is just avoiding depending on the wrapping behavior, which is an improvement but not really a "bug fix"; more a code style that will keep future work of making the kernel wrapping-safe.

It is a fix in the sense that it solves an issue reported by Coverity.

Quoted 4 lines
Quoted 2 lines
Quoted 1 line

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

But this change seems incomplete. The above line is no longer needed.

Yes, this is now a dead code.

Quoted 1 line

And I actually think this could be refractored to avoid needing "ready" at all?

Yeah, it sounds a good idea to place the zig-zag drift calculus on a separate function, doing some cleanups in the process.

I'll add it to my todo list.

Thanks,
Mauro

#12 lore raw

PATCH v2 5/13 media: mgb4: protect driver against spectre

In reply to 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")
---
 drivers/media/pci/mgb4/mgb4_cmt.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/media/pci/mgb4/mgb4_cmt.c +2 -0
index 70dc78ef193c..a25b68403bc6 100644
@@ -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];
 
Signature · 1 line
2.47.0
#13 lore raw

PATCH v2 6/13 media: av7110: fix a spectre vulnerability

In reply to 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")
---
 drivers/staging/media/av7110/av7110.h    |  4 +++-
 drivers/staging/media/av7110/av7110_ca.c | 25 ++++++++++++++++--------
 2 files changed, 20 insertions(+), 9 deletions(-)
drivers/staging/media/av7110/av7110.h +3 -1
index ec461fd187af..b584754f4be0 100644
@@ -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;
drivers/staging/media/av7110/av7110_ca.c +17 -8
index 6ce212c64e5d..fce4023c9dea 100644
@@ -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;
 	}
 
Signature · 1 line
2.47.0
#14 lore raw

PATCH v2 7/13 media: s5p-jpeg: prevent buffer overflows

In reply to Mauro Carvalho Chehab

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

While here, remove an unused word = 0 assignment.

Fixes 6c96dbbc2aa9 ("[media] s5p-jpeg: add support for 5433")
---
 .../media/platform/samsung/s5p-jpeg/jpeg-core.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)
drivers/media/platform/samsung/s5p-jpeg/jpeg-core.c +11 -6
index d2c4a0178b3c..1db4609b3557 100644
@@ -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;
 		}
Signature · 1 line
2.47.0
#15 lore raw

PATCH v2 8/13 media: ar0521: don't overflow when checking PLL values

In reply to Mauro Carvalho Chehab

The PLL checks are comparing 64 bit integers with 32 bit ones, as reported by Coverity. 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")
---
 drivers/media/i2c/ar0521.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/media/i2c/ar0521.c +2 -2
index fc27238dd4d3..24873149096c 100644
@@ -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,
Signature · 1 line
2.47.0
maintainer Acked-by on v2 8/13
#16 lore raw

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

Hi Mauro,

Quoted 10 lines · Mauro Carvalho Chehab

The PLL checks are comparing 64 bit integers with 32 bit ones, as reported by Coverity. 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")
Quoted 15 lines
drivers/media/i2c/ar0521.c +2 -2
@@ -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,
Signature · 5 lines
Krzysztof "Chris" Hałasa

Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa
#17 lore raw

PATCH v2 9/13 media: cx24116: prevent overflows on SNR calculus

In reply to Mauro Carvalho Chehab

as reported by Coverity, 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.")
---
 drivers/media/dvb-frontends/cx24116.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/media/dvb-frontends/cx24116.c +6 -1
index 8b978a9f74a4..f5dd3a81725a 100644
@@ -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;
Signature · 1 line
2.47.0
#18 lore raw

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

In reply to Mauro Carvalho Chehab

Currently, adv76xx_log_status() reads some date using io_read() which may return negative values. The current logic doesn't check such errors, causing colorspace to be reported on a wrong way at adv76xx_log_status(), as reported by Coverity.

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")
---
 drivers/media/i2c/adv7604.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)
drivers/media/i2c/adv7604.c +17 -9
index 48230d5109f0..272945a878b3 100644
@@ -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]);
 
Signature · 1 line
2.47.0
#19 lore raw

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

In reply to Mauro Carvalho Chehab

The loop at stb0899_search_carrier() starts with a random value for cfr, as reported by Coverity.

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")
---
 drivers/media/dvb-frontends/stb0899_algo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/media/dvb-frontends/stb0899_algo.c +1 -1
index df89c33dac23..40537c4ccb0d 100644
@@ -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;
Signature · 1 line
2.47.0
#20 lore raw

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

In reply to 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. This hits a Coverity bug, as Coverity assumes that positive values are underflow. While this is a false positive, returning error codes as -1 is an issue.

So, instead, use -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")
---
 .../cec/usb/extron-da-hd-4k-plus/extron-da-hd-4k-plus.c     | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/media/cec/usb/extron-da-hd-4k-plus/extron-da-hd-4k-plus.c +3 -3
index a526464af88c..7d03a36df5cf 100644
@@ -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)
Signature · 1 line
2.47.0
#21 lore raw

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

In reply to Mauro Carvalho Chehab

As pointed by Coverity, 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")
---
 drivers/media/cec/usb/pulse8/pulse8-cec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/media/cec/usb/pulse8/pulse8-cec.c +1 -1
index ba67587bd43e..171366fe3544 100644
@@ -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");
Signature · 1 line
2.47.0