[PATCH v2 00/13] Media: fix several issues on drivers

[PATCH v2 00/13] Media: fix several issues on drivers

[Mauro Carvalho Chehab]

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

-- 
2.47.0

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

[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")
Cc: stable@vger.kernel.org
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/v4l2-core/v4l2-ctrls-api.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c
index e5a364efd5e6..a0de7eeaf085 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);
@@ -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;
 }
-- 
2.47.0

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

[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")
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..940bfbf275ce 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_ONCE(!tpg->src_width || !tpg->scaled_width))
+		return;
+
 	switch (tpg->pattern) {
 	case TPG_PAT_GREEN:
 		contrast = TPG_COLOR_100_RED;
-- 
2.47.0

[PATCH v2 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
like Coverity.

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

Fixes: 5dd3f3071070 ("V4L/DVB (9361): Dynamic DVB minor allocation")
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 v2 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, 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")
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 v2 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>
Reviewed-by: Martin Tůma <martin.tuma@digiteqautomotive.com>
---
 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

[PATCH v2 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 v2 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, 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")
Cc: stable@vger.kernel.org
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Reviewed-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
---
 .../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

[PATCH v2 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, 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")
Cc: stable@vger.kernel.org
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 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

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

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

[PATCH v2 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, 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")
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 v2 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. 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")
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

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

[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")
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 v2 01/13] media: v4l2-ctrls-api: fix error handling for v4l2_g_ctrl()

[Hans Verkuil]

On 18/10/2024 07:53, Mauro Carvalho Chehab wrote:
> 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")
> Cc: stable@vger.kernel.org
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
>  drivers/media/v4l2-core/v4l2-ctrls-api.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c
> index e5a364efd5e6..a0de7eeaf085 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);
> @@ -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);

>  	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

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

[Mauro Carvalho Chehab]

Em Fri, 18 Oct 2024 08:13:01 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On 18/10/2024 07:53, Mauro Carvalho Chehab wrote:
> > 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")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > ---
> >  drivers/media/v4l2-core/v4l2-ctrls-api.c | 16 +++++++++++-----
> >  1 file changed, 11 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c
> > index e5a364efd5e6..a0de7eeaf085 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);
> > @@ -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);
> 
> >  	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")
Cc: stable@vger.kernel.org
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/v4l2-core/v4l2-ctrls-api.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c
index e5a364efd5e6..95a2202879d8 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);
@@ -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;
 }
-- 
2.47.0

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

[Krzysztof Hałasa]

Hi Mauro,

Mauro Carvalho Chehab <mchehab+huawei@kernel.org> writes:

> 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")
> Cc: stable@vger.kernel.org
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Acked-by: Krzysztof Hałasa <khalasa@piap.pl>

> --- 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,
-- 
Krzysztof "Chris" Hałasa

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

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

[Philipp Stanner]

On Fri, 2024-10-18 at 07:53 +0200, Mauro Carvalho Chehab wrote:
> 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: "?

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

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

Reviewed-by: Philipp Stanner <pstanner@redhat.com>

> ---
>  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++;

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

[Kees Cook]

On October 18, 2024 4:44:20 AM PDT, Philipp Stanner <pstanner@redhat.com> wrote:
>On Fri, 2024-10-18 at 07:53 +0200, Mauro Carvalho Chehab wrote:
>> 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: "?

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.

>
>> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
>
>Anyways, this in my eyes does what it's intended to do:
>
>Reviewed-by: Philipp Stanner <pstanner@redhat.com>
>
>> ---
>>  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++;
>

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

-- 
Kees Cook

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

[Philipp Stanner]

On Fri, 2024-10-18 at 07:37 -0700, Kees Cook wrote:
> 
> 
> On October 18, 2024 4:44:20 AM PDT, Philipp Stanner
> <pstanner@redhat.com> wrote:
> > On Fri, 2024-10-18 at 07:53 +0200, Mauro Carvalho Chehab wrote:
> > > 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: "?
> 
> 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:"

> 
> > 
> > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > 
> > Anyways, this in my eyes does what it's intended to do:
> > 
> > Reviewed-by: Philipp Stanner <pstanner@redhat.com>
> > 
> > > ---
> > >  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++;
> > 
> 
> 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.

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


> 
> -Kees
> 

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

[Mauro Carvalho Chehab]

Em Fri, 18 Oct 2024 07:37:52 -0700
Kees Cook <kees@kernel.org> escreveu:

> On October 18, 2024 4:44:20 AM PDT, Philipp Stanner <pstanner@redhat.com> wrote:
> >On Fri, 2024-10-18 at 07:53 +0200, Mauro Carvalho Chehab wrote:  
> >> 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?

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

	https://lore.kernel.org/all/20240714192914.1e1d3448@gandalf.local.home/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.

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

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

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

> 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