[PATCH 0/5] some smatch fixes

[PATCH 0/5] some smatch fixes

[Mauro Carvalho Chehab]

This 5 patch series addresses a couple of other smatch warnings.

One of the patches seem to be fixing an user-visible bug:

	media: uvc: don't do DMA on stack

Basically, input selection at the UVC driver seems broken, as it
is usind DMA on stack, which stopped working on Kernel 4.9.

In practice, the number of affected devices is probably small, as this
affects UVC devices with multiple inputs. The vast majority of UVC
ones have just one input.

Mauro Carvalho Chehab (5):
  media: dib8000: rewrite the init prbs logic
  media: uvc: don't do DMA on stack
  media: v4l2-flash-led-class: drop an useless check
  media: ivtv: prevent going past the hw arrays
  media: sti: don't copy past the size

 drivers/media/dvb-frontends/dib8000.c         | 56 ++++++++++-----
 drivers/media/pci/ivtv/ivtv-cards.h           | 68 +++++++++++++------
 drivers/media/pci/ivtv/ivtv-i2c.c             | 16 +++--
 drivers/media/platform/sti/delta/delta-ipc.c  |  3 +-
 drivers/media/usb/uvc/uvc_v4l2.c              | 10 ++-
 drivers/media/usb/uvc/uvcvideo.h              |  3 +
 .../media/v4l2-core/v4l2-flash-led-class.c    |  2 +-
 7 files changed, 106 insertions(+), 52 deletions(-)

-- 
2.31.1

[PATCH 1/5] media: dib8000: rewrite the init prbs logic

[Mauro Carvalho Chehab]

The logic at dib8000_get_init_prbs() has a few issues:

1. the tables used there has an extra unused value at the beginning;
2. the dprintk() message doesn't write the right value when
   transmission mode is not 8K;
3. the array overflow validation is done by the callers.

Rewrite the code to fix such issues.

This should also shut up those smatch warnings:

	drivers/media/dvb-frontends/dib8000.c:2125 dib8000_get_init_prbs() error: buffer overflow 'lut_prbs_8k' 14 <= 14
	drivers/media/dvb-frontends/dib8000.c:2129 dib8000_get_init_prbs() error: buffer overflow 'lut_prbs_2k' 14 <= 14
	drivers/media/dvb-frontends/dib8000.c:2131 dib8000_get_init_prbs() error: buffer overflow 'lut_prbs_4k' 14 <= 14
	drivers/media/dvb-frontends/dib8000.c:2134 dib8000_get_init_prbs() error: buffer overflow 'lut_prbs_8k' 14 <= 14

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/dvb-frontends/dib8000.c | 56 +++++++++++++++++++--------
 1 file changed, 39 insertions(+), 17 deletions(-)

diff --git a/drivers/media/dvb-frontends/dib8000.c b/drivers/media/dvb-frontends/dib8000.c
index 082796534b0a..541f8b9f5f8a 100644
--- a/drivers/media/dvb-frontends/dib8000.c
+++ b/drivers/media/dvb-frontends/dib8000.c
@@ -2107,32 +2107,53 @@ static void dib8000_load_ana_fe_coefs(struct dib8000_state *state, const s16 *an
 			dib8000_write_word(state, 117 + mode, ana_fe[mode]);
 }
 
-static const u16 lut_prbs_2k[14] = {
-	0, 0x423, 0x009, 0x5C7, 0x7A6, 0x3D8, 0x527, 0x7FF, 0x79B, 0x3D6, 0x3A2, 0x53B, 0x2F4, 0x213
+static const u16 lut_prbs_2k[13] = {
+	0x423, 0x009, 0x5C7,
+	0x7A6, 0x3D8, 0x527,
+	0x7FF, 0x79B, 0x3D6,
+	0x3A2, 0x53B, 0x2F4,
+	0x213
 };
-static const u16 lut_prbs_4k[14] = {
-	0, 0x208, 0x0C3, 0x7B9, 0x423, 0x5C7, 0x3D8, 0x7FF, 0x3D6, 0x53B, 0x213, 0x029, 0x0D0, 0x48E
+static const u16 lut_prbs_4k[13] = {
+	0x208, 0x0C3, 0x7B9,
+	0x423, 0x5C7, 0x3D8,
+	0x7FF, 0x3D6, 0x53B,
+	0x213, 0x029, 0x0D0,
+	0x48E
 };
-static const u16 lut_prbs_8k[14] = {
-	0, 0x740, 0x069, 0x7DD, 0x208, 0x7B9, 0x5C7, 0x7FF, 0x53B, 0x029, 0x48E, 0x4C4, 0x367, 0x684
+static const u16 lut_prbs_8k[13] = {
+	0x740, 0x069, 0x7DD,
+	0x208, 0x7B9, 0x5C7,
+	0x7FF, 0x53B, 0x029,
+	0x48E, 0x4C4, 0x367,
+	0x684
 };
 
 static u16 dib8000_get_init_prbs(struct dib8000_state *state, u16 subchannel)
 {
 	int sub_channel_prbs_group = 0;
+	int prbs_group;
 
-	sub_channel_prbs_group = (subchannel / 3) + 1;
-	dprintk("sub_channel_prbs_group = %d , subchannel =%d prbs = 0x%04x\n", sub_channel_prbs_group, subchannel, lut_prbs_8k[sub_channel_prbs_group]);
+	sub_channel_prbs_group = subchannel / 3;
+	if (sub_channel_prbs_group >= ARRAY_SIZE(lut_prbs_2k))
+		return 0;
 
 	switch (state->fe[0]->dtv_property_cache.transmission_mode) {
 	case TRANSMISSION_MODE_2K:
-			return lut_prbs_2k[sub_channel_prbs_group];
+		prbs_group = lut_prbs_2k[sub_channel_prbs_group];
+		break;
 	case TRANSMISSION_MODE_4K:
-			return lut_prbs_4k[sub_channel_prbs_group];
+		prbs_group =  lut_prbs_4k[sub_channel_prbs_group];
+		break;
 	default:
 	case TRANSMISSION_MODE_8K:
-			return lut_prbs_8k[sub_channel_prbs_group];
+		prbs_group = lut_prbs_8k[sub_channel_prbs_group];
 	}
+
+	dprintk("sub_channel_prbs_group = %d , subchannel =%d prbs = 0x%04x\n",
+		sub_channel_prbs_group, subchannel, prbs_group);
+
+	return prbs_group;
 }
 
 static void dib8000_set_13seg_channel(struct dib8000_state *state)
@@ -2409,10 +2430,8 @@ static void dib8000_set_isdbt_common_channel(struct dib8000_state *state, u8 seq
 	/* TSB or ISDBT ? apply it now */
 	if (c->isdbt_sb_mode) {
 		dib8000_set_sb_channel(state);
-		if (c->isdbt_sb_subchannel < 14)
-			init_prbs = dib8000_get_init_prbs(state, c->isdbt_sb_subchannel);
-		else
-			init_prbs = 0;
+		init_prbs = dib8000_get_init_prbs(state,
+						  c->isdbt_sb_subchannel);
 	} else {
 		dib8000_set_13seg_channel(state);
 		init_prbs = 0xfff;
@@ -3004,6 +3023,7 @@ static int dib8000_tune(struct dvb_frontend *fe)
 
 	unsigned long *timeout = &state->timeout;
 	unsigned long now = jiffies;
+	u16 init_prbs;
 #ifdef DIB8000_AGC_FREEZE
 	u16 agc1, agc2;
 #endif
@@ -3302,8 +3322,10 @@ static int dib8000_tune(struct dvb_frontend *fe)
 		break;
 
 	case CT_DEMOD_STEP_11:  /* 41 : init prbs autosearch */
-		if (state->subchannel <= 41) {
-			dib8000_set_subchannel_prbs(state, dib8000_get_init_prbs(state, state->subchannel));
+		init_prbs = dib8000_get_init_prbs(state, state->subchannel);
+
+		if (init_prbs) {
+			dib8000_set_subchannel_prbs(state, init_prbs);
 			*tune_state = CT_DEMOD_STEP_9;
 		} else {
 			*tune_state = CT_DEMOD_STOP;
-- 
2.31.1

[PATCH 2/5] media: uvc: don't do DMA on stack

[Mauro Carvalho Chehab]

As warned by smatch:
	drivers/media/usb/uvc/uvc_v4l2.c:911 uvc_ioctl_g_input() error: doing dma on the stack (&i)
	drivers/media/usb/uvc/uvc_v4l2.c:943 uvc_ioctl_s_input() error: doing dma on the stack (&i)

those two functions call uvc_query_ctrl passing a pointer to
a data at the DMA stack. those are used to send URBs via
usb_control_msg(). Using DMA stack is not supported and should
not work anymore on modern Linux versions.

So, use a temporary buffer, allocated together with
struct uvc_video_chain.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/usb/uvc/uvc_v4l2.c | 10 ++++------
 drivers/media/usb/uvc/uvcvideo.h |  3 +++
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 252136cc885c..e60d4675881a 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -900,7 +900,6 @@ static int uvc_ioctl_g_input(struct file *file, void *fh, unsigned int *input)
 	struct uvc_fh *handle = fh;
 	struct uvc_video_chain *chain = handle->chain;
 	int ret;
-	u8 i;
 
 	if (chain->selector == NULL ||
 	    (chain->dev->quirks & UVC_QUIRK_IGNORE_SELECTOR_UNIT)) {
@@ -910,11 +909,11 @@ static int uvc_ioctl_g_input(struct file *file, void *fh, unsigned int *input)
 
 	ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, chain->selector->id,
 			     chain->dev->intfnum,  UVC_SU_INPUT_SELECT_CONTROL,
-			     &i, 1);
+			     &chain->input, 1);
 	if (ret < 0)
 		return ret;
 
-	*input = i - 1;
+	*input = chain->input - 1;
 	return 0;
 }
 
@@ -923,7 +922,6 @@ static int uvc_ioctl_s_input(struct file *file, void *fh, unsigned int input)
 	struct uvc_fh *handle = fh;
 	struct uvc_video_chain *chain = handle->chain;
 	int ret;
-	u32 i;
 
 	ret = uvc_acquire_privileges(handle);
 	if (ret < 0)
@@ -939,10 +937,10 @@ static int uvc_ioctl_s_input(struct file *file, void *fh, unsigned int input)
 	if (input >= chain->selector->bNrInPins)
 		return -EINVAL;
 
-	i = input + 1;
+	chain->input = input + 1;
 	return uvc_query_ctrl(chain->dev, UVC_SET_CUR, chain->selector->id,
 			      chain->dev->intfnum, UVC_SU_INPUT_SELECT_CONTROL,
-			      &i, 1);
+			      &chain->input, 1);
 }
 
 static int uvc_ioctl_queryctrl(struct file *file, void *fh,
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index cce5e38133cd..3c0ed90d6912 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -475,6 +475,9 @@ struct uvc_video_chain {
 	struct mutex ctrl_mutex;		/* Protects ctrl.info */
 
 	struct v4l2_prio_state prio;		/* V4L2 priority state */
+
+	u8 input;				/* buffer for set/get input */
+
 	u32 caps;				/* V4L2 chain-wide caps */
 };
 
-- 
2.31.1

[PATCH 3/5] media: v4l2-flash-led-class: drop an useless check

[Mauro Carvalho Chehab]

As pointed by smatch:
	drivers/media/v4l2-core/v4l2-flash-led-class.c:264 v4l2_flash_s_ctrl() error: we previously assumed 'fled_cdev' could be null (see line 197)

It is too late to check if fled_cdev is NULL there. If such check is
needed, it should be, instead, inside v4l2_flash_init().

On other words, if v4l2_flash->fled_cdev() is NULL at
v4l2_flash_s_ctrl(), all led_*() function calls inside the function
would try to de-reference a NULL pointer, as the logic won't prevent
it.

So, remove the useless check.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/v4l2-core/v4l2-flash-led-class.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-flash-led-class.c b/drivers/media/v4l2-core/v4l2-flash-led-class.c
index 10ddcc48aa17..a1653c635d82 100644
--- a/drivers/media/v4l2-core/v4l2-flash-led-class.c
+++ b/drivers/media/v4l2-core/v4l2-flash-led-class.c
@@ -194,7 +194,7 @@ static int v4l2_flash_s_ctrl(struct v4l2_ctrl *c)
 {
 	struct v4l2_flash *v4l2_flash = v4l2_ctrl_to_v4l2_flash(c);
 	struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
-	struct led_classdev *led_cdev = fled_cdev ? &fled_cdev->led_cdev : NULL;
+	struct led_classdev *led_cdev = &fled_cdev->led_cdev;
 	struct v4l2_ctrl **ctrls = v4l2_flash->ctrls;
 	bool external_strobe;
 	int ret = 0;
-- 
2.31.1

[PATCH 4/5] media: ivtv: prevent going past the hw arrays

[Mauro Carvalho Chehab]

As warned by smatch:

	drivers/media/pci/ivtv/ivtv-i2c.c:245 ivtv_i2c_register() error: buffer overflow 'hw_devicenames' 21 <= 31
	drivers/media/pci/ivtv/ivtv-i2c.c:266 ivtv_i2c_register() error: buffer overflow 'hw_addrs' 21 <= 31
	drivers/media/pci/ivtv/ivtv-i2c.c:269 ivtv_i2c_register() error: buffer overflow 'hw_addrs' 21 <= 31
	drivers/media/pci/ivtv/ivtv-i2c.c:275 ivtv_i2c_register() error: buffer overflow 'hw_addrs' 21 <= 31
	drivers/media/pci/ivtv/ivtv-i2c.c:280 ivtv_i2c_register() error: buffer overflow 'hw_addrs' 21 <= 31
	drivers/media/pci/ivtv/ivtv-i2c.c:290 ivtv_i2c_register() error: buffer overflow 'hw_addrs' 21 <= 31

The logic at ivtv_i2c_register() could let buffer overflows at
hw_devicenames and hw_addrs arrays.

This won't happen in practice due to a carefully-contructed
logic, but it is not error-prune.

Change the logic in a way that will make clearer that the
I2C hardware flags will affect the size of those two
arrays, and add an explicit check to avoid buffer overflows.

While here, use the bit macro.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/pci/ivtv/ivtv-cards.h | 68 ++++++++++++++++++++---------
 drivers/media/pci/ivtv/ivtv-i2c.c   | 16 ++++---
 2 files changed, 58 insertions(+), 26 deletions(-)

diff --git a/drivers/media/pci/ivtv/ivtv-cards.h b/drivers/media/pci/ivtv/ivtv-cards.h
index f3e2c5634962..494982e4165d 100644
--- a/drivers/media/pci/ivtv/ivtv-cards.h
+++ b/drivers/media/pci/ivtv/ivtv-cards.h
@@ -78,27 +78,53 @@
 #define IVTV_PCI_ID_SONY		0x104d
 
 /* hardware flags, no gaps allowed */
-#define IVTV_HW_CX25840			(1 << 0)
-#define IVTV_HW_SAA7115			(1 << 1)
-#define IVTV_HW_SAA7127			(1 << 2)
-#define IVTV_HW_MSP34XX			(1 << 3)
-#define IVTV_HW_TUNER			(1 << 4)
-#define IVTV_HW_WM8775			(1 << 5)
-#define IVTV_HW_CS53L32A		(1 << 6)
-#define IVTV_HW_TVEEPROM		(1 << 7)
-#define IVTV_HW_SAA7114			(1 << 8)
-#define IVTV_HW_UPD64031A		(1 << 9)
-#define IVTV_HW_UPD6408X		(1 << 10)
-#define IVTV_HW_SAA717X			(1 << 11)
-#define IVTV_HW_WM8739			(1 << 12)
-#define IVTV_HW_VP27SMPX		(1 << 13)
-#define IVTV_HW_M52790			(1 << 14)
-#define IVTV_HW_GPIO			(1 << 15)
-#define IVTV_HW_I2C_IR_RX_AVER		(1 << 16)
-#define IVTV_HW_I2C_IR_RX_HAUP_EXT	(1 << 17) /* External before internal */
-#define IVTV_HW_I2C_IR_RX_HAUP_INT	(1 << 18)
-#define IVTV_HW_Z8F0811_IR_HAUP		(1 << 19)
-#define IVTV_HW_I2C_IR_RX_ADAPTEC	(1 << 20)
+enum ivtv_hw_bits {
+	IVTV_HW_BIT_CX25840		= 0,
+	IVTV_HW_BIT_SAA7115		= 1,
+	IVTV_HW_BIT_SAA7127		= 2,
+	IVTV_HW_BIT_MSP34XX		= 3,
+	IVTV_HW_BIT_TUNER		= 4,
+	IVTV_HW_BIT_WM8775		= 5,
+	IVTV_HW_BIT_CS53L32A		= 6,
+	IVTV_HW_BIT_TVEEPROM		= 7,
+	IVTV_HW_BIT_SAA7114		= 8,
+	IVTV_HW_BIT_UPD64031A		= 9,
+	IVTV_HW_BIT_UPD6408X		= 10,
+	IVTV_HW_BIT_SAA717X		= 11,
+	IVTV_HW_BIT_WM8739		= 12,
+	IVTV_HW_BIT_VP27SMPX		= 13,
+	IVTV_HW_BIT_M52790		= 14,
+	IVTV_HW_BIT_GPIO		= 15,
+	IVTV_HW_BIT_I2C_IR_RX_AVER	= 16,
+	IVTV_HW_BIT_I2C_IR_RX_HAUP_EXT	= 17, /* External before internal */
+	IVTV_HW_BIT_I2C_IR_RX_HAUP_INT	= 18,
+	IVTV_HW_BIT_Z8F0811_IR_HAUP	= 19,
+	IVTV_HW_BIT_I2C_IR_RX_ADAPTEC	= 20,
+
+	IVTV_HW_MAX_BITS		= 21	/* Should be the last bit + 1 */
+};
+
+#define IVTV_HW_CX25840			BIT(IVTV_HW_BIT_CX25840)
+#define IVTV_HW_SAA7115			BIT(IVTV_HW_BIT_SAA7115)
+#define IVTV_HW_SAA7127			BIT(IVTV_HW_BIT_SAA7127)
+#define IVTV_HW_MSP34XX			BIT(IVTV_HW_BIT_MSP34XX)
+#define IVTV_HW_TUNER			BIT(IVTV_HW_BIT_TUNER)
+#define IVTV_HW_WM8775			BIT(IVTV_HW_BIT_WM8775)
+#define IVTV_HW_CS53L32A		BIT(IVTV_HW_BIT_CS53L32A)
+#define IVTV_HW_TVEEPROM		BIT(IVTV_HW_BIT_TVEEPROM)
+#define IVTV_HW_SAA7114			BIT(IVTV_HW_BIT_SAA7114)
+#define IVTV_HW_UPD64031A		BIT(IVTV_HW_BIT_UPD64031A)
+#define IVTV_HW_UPD6408X		BIT(IVTV_HW_BIT_UPD6408X)
+#define IVTV_HW_SAA717X			BIT(IVTV_HW_BIT_SAA717X)
+#define IVTV_HW_WM8739			BIT(IVTV_HW_BIT_WM8739)
+#define IVTV_HW_VP27SMPX		BIT(IVTV_HW_BIT_VP27SMPX)
+#define IVTV_HW_M52790			BIT(IVTV_HW_BIT_M52790)
+#define IVTV_HW_GPIO			BIT(IVTV_HW_BIT_GPIO)
+#define IVTV_HW_I2C_IR_RX_AVER		BIT(IVTV_HW_BIT_I2C_IR_RX_AVER)
+#define IVTV_HW_I2C_IR_RX_HAUP_EXT	BIT(IVTV_HW_BIT_I2C_IR_RX_HAUP_EXT)
+#define IVTV_HW_I2C_IR_RX_HAUP_INT	BIT(IVTV_HW_BIT_I2C_IR_RX_HAUP_INT)
+#define IVTV_HW_Z8F0811_IR_HAUP		BIT(IVTV_HW_BIT_Z8F0811_IR_HAUP)
+#define IVTV_HW_I2C_IR_RX_ADAPTEC	BIT(IVTV_HW_BIT_I2C_IR_RX_ADAPTEC)
 
 #define IVTV_HW_SAA711X   (IVTV_HW_SAA7115 | IVTV_HW_SAA7114)
 
diff --git a/drivers/media/pci/ivtv/ivtv-i2c.c b/drivers/media/pci/ivtv/ivtv-i2c.c
index 982045c4eea8..c052c57c6dce 100644
--- a/drivers/media/pci/ivtv/ivtv-i2c.c
+++ b/drivers/media/pci/ivtv/ivtv-i2c.c
@@ -85,7 +85,7 @@
 #define IVTV_ADAPTEC_IR_ADDR		0x6b
 
 /* This array should match the IVTV_HW_ defines */
-static const u8 hw_addrs[] = {
+static const u8 hw_addrs[IVTV_HW_MAX_BITS] = {
 	IVTV_CX25840_I2C_ADDR,
 	IVTV_SAA7115_I2C_ADDR,
 	IVTV_SAA7127_I2C_ADDR,
@@ -110,7 +110,7 @@ static const u8 hw_addrs[] = {
 };
 
 /* This array should match the IVTV_HW_ defines */
-static const char * const hw_devicenames[] = {
+static const char * const hw_devicenames[IVTV_HW_MAX_BITS] = {
 	"cx25840",
 	"saa7115",
 	"saa7127_auto",	/* saa7127 or saa7129 */
@@ -240,10 +240,16 @@ void ivtv_i2c_new_ir_legacy(struct ivtv *itv)
 
 int ivtv_i2c_register(struct ivtv *itv, unsigned idx)
 {
-	struct v4l2_subdev *sd;
 	struct i2c_adapter *adap = &itv->i2c_adap;
-	const char *type = hw_devicenames[idx];
-	u32 hw = 1 << idx;
+	struct v4l2_subdev *sd;
+	const char *type;
+	u32 hw;
+
+	if (idx >= IVTV_HW_MAX_BITS)
+		return -ENODEV;
+
+	type = hw_devicenames[idx];
+	hw = 1 << idx;
 
 	if (hw == IVTV_HW_TUNER) {
 		/* special tuner handling */
-- 
2.31.1

[PATCH 5/5] media: sti: don't copy past the size

[Mauro Carvalho Chehab]

The logic at delta_ipc_open() tries to copy past the size of
the name passed to it:

	drivers/media/platform/sti/delta/delta-ipc.c:178 delta_ipc_open() error: __memcpy() 'name' too small (17 vs 32)

Basically,this function is called just one with:

	ret = delta_ipc_open(pctx, "JPEG_DECODER_HW0", ...);

The string used there has just 17 bytes. Yet, the logic tries
to copy the entire name size (32 bytes), which is plain wrong.

Replace it by strscpy, which is good enough to copy the string,
warranting that this will be NUL-terminated.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/platform/sti/delta/delta-ipc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/media/platform/sti/delta/delta-ipc.c b/drivers/media/platform/sti/delta/delta-ipc.c
index 186d88f02ecd..21d3e08e259a 100644
--- a/drivers/media/platform/sti/delta/delta-ipc.c
+++ b/drivers/media/platform/sti/delta/delta-ipc.c
@@ -175,8 +175,7 @@ int delta_ipc_open(struct delta_ctx *pctx, const char *name,
 	msg.ipc_buf_size = ipc_buf_size;
 	msg.ipc_buf_paddr = ctx->ipc_buf->paddr;
 
-	memcpy(msg.name, name, sizeof(msg.name));
-	msg.name[sizeof(msg.name) - 1] = 0;
+	strscpy(msg.name, name, sizeof(msg.name));
 
 	msg.param_size = param->size;
 	memcpy(ctx->ipc_buf->vaddr, param->data, msg.param_size);
-- 
2.31.1

Re: [PATCH 2/5] media: uvc: don't do DMA on stack

[Laurent Pinchart]

Hi Mauro,

Thank you for the patch.

On Mon, Jun 21, 2021 at 01:56:46PM +0200, Mauro Carvalho Chehab wrote:
> As warned by smatch:
> 	drivers/media/usb/uvc/uvc_v4l2.c:911 uvc_ioctl_g_input() error: doing dma on the stack (&i)
> 	drivers/media/usb/uvc/uvc_v4l2.c:943 uvc_ioctl_s_input() error: doing dma on the stack (&i)
> 
> those two functions call uvc_query_ctrl passing a pointer to
> a data at the DMA stack. those are used to send URBs via
> usb_control_msg(). Using DMA stack is not supported and should
> not work anymore on modern Linux versions.
> 
> So, use a temporary buffer, allocated together with
> struct uvc_video_chain.

DMA in a memory location that may share a cache line with something else
isn't a great idea either. The buffer should be kmalloc()ed in
uvc_ioctl_g_input() and uvc_ioctl_s_input().

> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
>  drivers/media/usb/uvc/uvc_v4l2.c | 10 ++++------
>  drivers/media/usb/uvc/uvcvideo.h |  3 +++
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 252136cc885c..e60d4675881a 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -900,7 +900,6 @@ static int uvc_ioctl_g_input(struct file *file, void *fh, unsigned int *input)
>  	struct uvc_fh *handle = fh;
>  	struct uvc_video_chain *chain = handle->chain;
>  	int ret;
> -	u8 i;
>  
>  	if (chain->selector == NULL ||
>  	    (chain->dev->quirks & UVC_QUIRK_IGNORE_SELECTOR_UNIT)) {
> @@ -910,11 +909,11 @@ static int uvc_ioctl_g_input(struct file *file, void *fh, unsigned int *input)
>  
>  	ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, chain->selector->id,
>  			     chain->dev->intfnum,  UVC_SU_INPUT_SELECT_CONTROL,
> -			     &i, 1);
> +			     &chain->input, 1);
>  	if (ret < 0)
>  		return ret;
>  
> -	*input = i - 1;
> +	*input = chain->input - 1;
>  	return 0;
>  }
>  
> @@ -923,7 +922,6 @@ static int uvc_ioctl_s_input(struct file *file, void *fh, unsigned int input)
>  	struct uvc_fh *handle = fh;
>  	struct uvc_video_chain *chain = handle->chain;
>  	int ret;
> -	u32 i;
>  
>  	ret = uvc_acquire_privileges(handle);
>  	if (ret < 0)
> @@ -939,10 +937,10 @@ static int uvc_ioctl_s_input(struct file *file, void *fh, unsigned int input)
>  	if (input >= chain->selector->bNrInPins)
>  		return -EINVAL;
>  
> -	i = input + 1;
> +	chain->input = input + 1;
>  	return uvc_query_ctrl(chain->dev, UVC_SET_CUR, chain->selector->id,
>  			      chain->dev->intfnum, UVC_SU_INPUT_SELECT_CONTROL,
> -			      &i, 1);
> +			      &chain->input, 1);
>  }
>  
>  static int uvc_ioctl_queryctrl(struct file *file, void *fh,
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index cce5e38133cd..3c0ed90d6912 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -475,6 +475,9 @@ struct uvc_video_chain {
>  	struct mutex ctrl_mutex;		/* Protects ctrl.info */
>  
>  	struct v4l2_prio_state prio;		/* V4L2 priority state */
> +
> +	u8 input;				/* buffer for set/get input */
> +
>  	u32 caps;				/* V4L2 chain-wide caps */
>  };
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/5] media: ivtv: prevent going past the hw arrays

[Hans Verkuil]

On 21/06/2021 13:56, Mauro Carvalho Chehab wrote:
> As warned by smatch:
> 
> 	drivers/media/pci/ivtv/ivtv-i2c.c:245 ivtv_i2c_register() error: buffer overflow 'hw_devicenames' 21 <= 31
> 	drivers/media/pci/ivtv/ivtv-i2c.c:266 ivtv_i2c_register() error: buffer overflow 'hw_addrs' 21 <= 31
> 	drivers/media/pci/ivtv/ivtv-i2c.c:269 ivtv_i2c_register() error: buffer overflow 'hw_addrs' 21 <= 31
> 	drivers/media/pci/ivtv/ivtv-i2c.c:275 ivtv_i2c_register() error: buffer overflow 'hw_addrs' 21 <= 31
> 	drivers/media/pci/ivtv/ivtv-i2c.c:280 ivtv_i2c_register() error: buffer overflow 'hw_addrs' 21 <= 31
> 	drivers/media/pci/ivtv/ivtv-i2c.c:290 ivtv_i2c_register() error: buffer overflow 'hw_addrs' 21 <= 31
> 
> The logic at ivtv_i2c_register() could let buffer overflows at
> hw_devicenames and hw_addrs arrays.
> 
> This won't happen in practice due to a carefully-contructed
> logic, but it is not error-prune.
> 
> Change the logic in a way that will make clearer that the
> I2C hardware flags will affect the size of those two
> arrays, and add an explicit check to avoid buffer overflows.
> 
> While here, use the bit macro.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
>  drivers/media/pci/ivtv/ivtv-cards.h | 68 ++++++++++++++++++++---------
>  drivers/media/pci/ivtv/ivtv-i2c.c   | 16 ++++---
>  2 files changed, 58 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/media/pci/ivtv/ivtv-cards.h b/drivers/media/pci/ivtv/ivtv-cards.h
> index f3e2c5634962..494982e4165d 100644
> --- a/drivers/media/pci/ivtv/ivtv-cards.h
> +++ b/drivers/media/pci/ivtv/ivtv-cards.h
> @@ -78,27 +78,53 @@
>  #define IVTV_PCI_ID_SONY		0x104d
>  
>  /* hardware flags, no gaps allowed */
> -#define IVTV_HW_CX25840			(1 << 0)
> -#define IVTV_HW_SAA7115			(1 << 1)
> -#define IVTV_HW_SAA7127			(1 << 2)
> -#define IVTV_HW_MSP34XX			(1 << 3)
> -#define IVTV_HW_TUNER			(1 << 4)
> -#define IVTV_HW_WM8775			(1 << 5)
> -#define IVTV_HW_CS53L32A		(1 << 6)
> -#define IVTV_HW_TVEEPROM		(1 << 7)
> -#define IVTV_HW_SAA7114			(1 << 8)
> -#define IVTV_HW_UPD64031A		(1 << 9)
> -#define IVTV_HW_UPD6408X		(1 << 10)
> -#define IVTV_HW_SAA717X			(1 << 11)
> -#define IVTV_HW_WM8739			(1 << 12)
> -#define IVTV_HW_VP27SMPX		(1 << 13)
> -#define IVTV_HW_M52790			(1 << 14)
> -#define IVTV_HW_GPIO			(1 << 15)
> -#define IVTV_HW_I2C_IR_RX_AVER		(1 << 16)
> -#define IVTV_HW_I2C_IR_RX_HAUP_EXT	(1 << 17) /* External before internal */
> -#define IVTV_HW_I2C_IR_RX_HAUP_INT	(1 << 18)
> -#define IVTV_HW_Z8F0811_IR_HAUP		(1 << 19)
> -#define IVTV_HW_I2C_IR_RX_ADAPTEC	(1 << 20)
> +enum ivtv_hw_bits {
> +	IVTV_HW_BIT_CX25840		= 0,
> +	IVTV_HW_BIT_SAA7115		= 1,
> +	IVTV_HW_BIT_SAA7127		= 2,
> +	IVTV_HW_BIT_MSP34XX		= 3,
> +	IVTV_HW_BIT_TUNER		= 4,
> +	IVTV_HW_BIT_WM8775		= 5,
> +	IVTV_HW_BIT_CS53L32A		= 6,
> +	IVTV_HW_BIT_TVEEPROM		= 7,
> +	IVTV_HW_BIT_SAA7114		= 8,
> +	IVTV_HW_BIT_UPD64031A		= 9,
> +	IVTV_HW_BIT_UPD6408X		= 10,
> +	IVTV_HW_BIT_SAA717X		= 11,
> +	IVTV_HW_BIT_WM8739		= 12,
> +	IVTV_HW_BIT_VP27SMPX		= 13,
> +	IVTV_HW_BIT_M52790		= 14,
> +	IVTV_HW_BIT_GPIO		= 15,
> +	IVTV_HW_BIT_I2C_IR_RX_AVER	= 16,
> +	IVTV_HW_BIT_I2C_IR_RX_HAUP_EXT	= 17, /* External before internal */
> +	IVTV_HW_BIT_I2C_IR_RX_HAUP_INT	= 18,
> +	IVTV_HW_BIT_Z8F0811_IR_HAUP	= 19,
> +	IVTV_HW_BIT_I2C_IR_RX_ADAPTEC	= 20,
> +
> +	IVTV_HW_MAX_BITS		= 21	/* Should be the last bit + 1 */

It's an enum, so you can drop the '= nr' bit.

Other than that it looks OK to me.

Regards,

	Hans

> +};
> +
> +#define IVTV_HW_CX25840			BIT(IVTV_HW_BIT_CX25840)
> +#define IVTV_HW_SAA7115			BIT(IVTV_HW_BIT_SAA7115)
> +#define IVTV_HW_SAA7127			BIT(IVTV_HW_BIT_SAA7127)
> +#define IVTV_HW_MSP34XX			BIT(IVTV_HW_BIT_MSP34XX)
> +#define IVTV_HW_TUNER			BIT(IVTV_HW_BIT_TUNER)
> +#define IVTV_HW_WM8775			BIT(IVTV_HW_BIT_WM8775)
> +#define IVTV_HW_CS53L32A		BIT(IVTV_HW_BIT_CS53L32A)
> +#define IVTV_HW_TVEEPROM		BIT(IVTV_HW_BIT_TVEEPROM)
> +#define IVTV_HW_SAA7114			BIT(IVTV_HW_BIT_SAA7114)
> +#define IVTV_HW_UPD64031A		BIT(IVTV_HW_BIT_UPD64031A)
> +#define IVTV_HW_UPD6408X		BIT(IVTV_HW_BIT_UPD6408X)
> +#define IVTV_HW_SAA717X			BIT(IVTV_HW_BIT_SAA717X)
> +#define IVTV_HW_WM8739			BIT(IVTV_HW_BIT_WM8739)
> +#define IVTV_HW_VP27SMPX		BIT(IVTV_HW_BIT_VP27SMPX)
> +#define IVTV_HW_M52790			BIT(IVTV_HW_BIT_M52790)
> +#define IVTV_HW_GPIO			BIT(IVTV_HW_BIT_GPIO)
> +#define IVTV_HW_I2C_IR_RX_AVER		BIT(IVTV_HW_BIT_I2C_IR_RX_AVER)
> +#define IVTV_HW_I2C_IR_RX_HAUP_EXT	BIT(IVTV_HW_BIT_I2C_IR_RX_HAUP_EXT)
> +#define IVTV_HW_I2C_IR_RX_HAUP_INT	BIT(IVTV_HW_BIT_I2C_IR_RX_HAUP_INT)
> +#define IVTV_HW_Z8F0811_IR_HAUP		BIT(IVTV_HW_BIT_Z8F0811_IR_HAUP)
> +#define IVTV_HW_I2C_IR_RX_ADAPTEC	BIT(IVTV_HW_BIT_I2C_IR_RX_ADAPTEC)
>  
>  #define IVTV_HW_SAA711X   (IVTV_HW_SAA7115 | IVTV_HW_SAA7114)
>  
> diff --git a/drivers/media/pci/ivtv/ivtv-i2c.c b/drivers/media/pci/ivtv/ivtv-i2c.c
> index 982045c4eea8..c052c57c6dce 100644
> --- a/drivers/media/pci/ivtv/ivtv-i2c.c
> +++ b/drivers/media/pci/ivtv/ivtv-i2c.c
> @@ -85,7 +85,7 @@
>  #define IVTV_ADAPTEC_IR_ADDR		0x6b
>  
>  /* This array should match the IVTV_HW_ defines */
> -static const u8 hw_addrs[] = {
> +static const u8 hw_addrs[IVTV_HW_MAX_BITS] = {
>  	IVTV_CX25840_I2C_ADDR,
>  	IVTV_SAA7115_I2C_ADDR,
>  	IVTV_SAA7127_I2C_ADDR,
> @@ -110,7 +110,7 @@ static const u8 hw_addrs[] = {
>  };
>  
>  /* This array should match the IVTV_HW_ defines */
> -static const char * const hw_devicenames[] = {
> +static const char * const hw_devicenames[IVTV_HW_MAX_BITS] = {
>  	"cx25840",
>  	"saa7115",
>  	"saa7127_auto",	/* saa7127 or saa7129 */
> @@ -240,10 +240,16 @@ void ivtv_i2c_new_ir_legacy(struct ivtv *itv)
>  
>  int ivtv_i2c_register(struct ivtv *itv, unsigned idx)
>  {
> -	struct v4l2_subdev *sd;
>  	struct i2c_adapter *adap = &itv->i2c_adap;
> -	const char *type = hw_devicenames[idx];
> -	u32 hw = 1 << idx;
> +	struct v4l2_subdev *sd;
> +	const char *type;
> +	u32 hw;
> +
> +	if (idx >= IVTV_HW_MAX_BITS)
> +		return -ENODEV;
> +
> +	type = hw_devicenames[idx];
> +	hw = 1 << idx;
>  
>  	if (hw == IVTV_HW_TUNER) {
>  		/* special tuner handling */
> 

Re: [PATCH 3/5] media: v4l2-flash-led-class: drop an useless check

[Sakari Ailus]

Hi Mauro,

Could you check if your mail client could be configured not to add junk to
To: field? It often leads anything in the Cc: field being dropped.

On Mon, Jun 21, 2021 at 01:56:47PM +0200, Mauro Carvalho Chehab wrote:
> As pointed by smatch:
> 	drivers/media/v4l2-core/v4l2-flash-led-class.c:264 v4l2_flash_s_ctrl() error: we previously assumed 'fled_cdev' could be null (see line 197)
> 
> It is too late to check if fled_cdev is NULL there. If such check is
> needed, it should be, instead, inside v4l2_flash_init().
> 
> On other words, if v4l2_flash->fled_cdev() is NULL at
> v4l2_flash_s_ctrl(), all led_*() function calls inside the function
> would try to de-reference a NULL pointer, as the logic won't prevent
> it.
> 
> So, remove the useless check.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
>  drivers/media/v4l2-core/v4l2-flash-led-class.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-flash-led-class.c b/drivers/media/v4l2-core/v4l2-flash-led-class.c
> index 10ddcc48aa17..a1653c635d82 100644
> --- a/drivers/media/v4l2-core/v4l2-flash-led-class.c
> +++ b/drivers/media/v4l2-core/v4l2-flash-led-class.c
> @@ -194,7 +194,7 @@ static int v4l2_flash_s_ctrl(struct v4l2_ctrl *c)
>  {
>  	struct v4l2_flash *v4l2_flash = v4l2_ctrl_to_v4l2_flash(c);
>  	struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
> -	struct led_classdev *led_cdev = fled_cdev ? &fled_cdev->led_cdev : NULL;
> +	struct led_classdev *led_cdev = &fled_cdev->led_cdev;

fled_cdev may be NULL here. The reason is that some controls are for flash
LEDs only but the same sub-device may also control an indicator. This is
covered when the controls are created, so that the NULL pointer isn't
dereferenced.

If you wish the false positive to be addressed while also improving the
implementation, that could be done by e.g. splitting the switch into two,
the part that needs fled_cdev and another that doesn't.

I can send a patch for that.

Please also cc me to V4L2 flash class patches. I noticed this one by
accident only.

>  	struct v4l2_ctrl **ctrls = v4l2_flash->ctrls;
>  	bool external_strobe;
>  	int ret = 0;

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 3/5] media: v4l2-flash-led-class: drop an useless check

[Mauro Carvalho Chehab]

Em Thu, 24 Jun 2021 12:31:53 +0300
Sakari Ailus <sakari.ailus@iki.fi> escreveu:

> Hi Mauro,
> 
> Could you check if your mail client could be configured not to add junk to
> To: field? It often leads anything in the Cc: field being dropped.

I have no idea why it is doing that. I'm just using git send-email
here. Perhaps a git bug?

	$ git --version
	git version 2.31.1

The setup is like this one:

	[sendemail]
	        confirm = always
	        multiedit = true
	        chainreplyto = false
	        aliasesfile = /home/mchehab/.addressbook
	        aliasfiletype = pine
	        assume8bitencoding = UTF-8


> 
> On Mon, Jun 21, 2021 at 01:56:47PM +0200, Mauro Carvalho Chehab wrote:
> > As pointed by smatch:
> > 	drivers/media/v4l2-core/v4l2-flash-led-class.c:264 v4l2_flash_s_ctrl() error: we previously assumed 'fled_cdev' could be null (see line 197)
> > 
> > It is too late to check if fled_cdev is NULL there. If such check is
> > needed, it should be, instead, inside v4l2_flash_init().
> > 
> > On other words, if v4l2_flash->fled_cdev() is NULL at
> > v4l2_flash_s_ctrl(), all led_*() function calls inside the function
> > would try to de-reference a NULL pointer, as the logic won't prevent
> > it.
> > 
> > So, remove the useless check.
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > ---
> >  drivers/media/v4l2-core/v4l2-flash-led-class.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-flash-led-class.c b/drivers/media/v4l2-core/v4l2-flash-led-class.c
> > index 10ddcc48aa17..a1653c635d82 100644
> > --- a/drivers/media/v4l2-core/v4l2-flash-led-class.c
> > +++ b/drivers/media/v4l2-core/v4l2-flash-led-class.c
> > @@ -194,7 +194,7 @@ static int v4l2_flash_s_ctrl(struct v4l2_ctrl *c)
> >  {
> >  	struct v4l2_flash *v4l2_flash = v4l2_ctrl_to_v4l2_flash(c);
> >  	struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
> > -	struct led_classdev *led_cdev = fled_cdev ? &fled_cdev->led_cdev : NULL;
> > +	struct led_classdev *led_cdev = &fled_cdev->led_cdev;  
> 
> fled_cdev may be NULL here. The reason is that some controls are for flash
> LEDs only but the same sub-device may also control an indicator. This is
> covered when the controls are created, so that the NULL pointer isn't
> dereferenced.

I double-checked the code: if a a NULL pointer is passed, the calls
to the leds framework will try to de-reference it or will return an
error.

For instance, those will return an error:

	static inline int led_set_flash_strobe(struct led_classdev_flash *fled_cdev,
	                                        bool state)
	{
	        if (!fled_cdev)
	                return -EINVAL;
	        return fled_cdev->ops->strobe_set(fled_cdev, state);
	}

	#define call_flash_op(fled_cdev, op, args...)           \
	        ((has_flash_op(fled_cdev, op)) ?                        \
	                        (fled_cdev->ops->op(fled_cdev, args)) : \
	                        -EINVAL)

No big issue here (except that the function will do nothing but
return an error).

However, there are places that it will cause it to de-reference 
a NULL pointer:

	int led_set_brightness_sync(struct led_classdev *led_cdev, unsigned int value)
	{
	        if (led_cdev->blink_delay_on || led_cdev->blink_delay_off)
	                return -EBUSY;
	
	        led_cdev->brightness = min(value, led_cdev->max_brightness);

	        if (led_cdev->flags & LED_SUSPENDED)
	                return 0;

	        return __led_set_brightness_blocking(led_cdev, led_cdev->brightness);
	}

So, this is not a false-positive, but, instead, a real issue.

So, if led_cdev/fled_cdev can indeed be NULL, IMO, the right solution would be
to explicitly check it, and return an error, e. g.:

	static int v4l2_flash_s_ctrl(struct v4l2_ctrl *c)
	{
        	struct v4l2_flash *v4l2_flash = v4l2_ctrl_to_v4l2_flash(c);
        	struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
		struct led_classdev *led_cdev;
        	struct v4l2_ctrl **ctrls = v4l2_flash->ctrls;
        	bool external_strobe;
        	int ret = 0;

		if (!fled_cdev)
			return -EINVAL;

		led_cdev = &fled_cdev->led_cdev;

		...

> 
> If you wish the false positive to be addressed while also improving the
> implementation, that could be done by e.g. splitting the switch into two,
> the part that needs fled_cdev and another that doesn't.
> 
> I can send a patch for that.
> 
> Please also cc me to V4L2 flash class patches. I noticed this one by
> accident only.

Better to add you as a reviewer at the MAINTAINERS file, to
ensure that you'll always be c/c on such code.

Thanks,
Mauro

Re: [PATCH 3/5] media: v4l2-flash-led-class: drop an useless check

[Sakari Ailus]

Hi Mauro,

On Thu, Jun 24, 2021 at 11:59:25AM +0200, Mauro Carvalho Chehab wrote:
> Em Thu, 24 Jun 2021 12:31:53 +0300
> Sakari Ailus <sakari.ailus@iki.fi> escreveu:
> 
> > Hi Mauro,
> > 
> > Could you check if your mail client could be configured not to add junk to
> > To: field? It often leads anything in the Cc: field being dropped.
> 
> I have no idea why it is doing that. I'm just using git send-email
> here. Perhaps a git bug?
> 
> 	$ git --version
> 	git version 2.31.1
> 
> The setup is like this one:
> 
> 	[sendemail]
> 	        confirm = always
> 	        multiedit = true
> 	        chainreplyto = false
> 	        aliasesfile = /home/mchehab/.addressbook
> 	        aliasfiletype = pine
> 	        assume8bitencoding = UTF-8

I tried sending a message to myself using git send-email with an empty To:
field and it came through just fine, with To: field remaining empty. I
wonder if it could be the list server?

It could be difficult to fix, but what I'm saying leaving the To: field
empty now has this effect. :-I

> 
> 
> > 
> > On Mon, Jun 21, 2021 at 01:56:47PM +0200, Mauro Carvalho Chehab wrote:
> > > As pointed by smatch:
> > > 	drivers/media/v4l2-core/v4l2-flash-led-class.c:264 v4l2_flash_s_ctrl() error: we previously assumed 'fled_cdev' could be null (see line 197)
> > > 
> > > It is too late to check if fled_cdev is NULL there. If such check is
> > > needed, it should be, instead, inside v4l2_flash_init().
> > > 
> > > On other words, if v4l2_flash->fled_cdev() is NULL at
> > > v4l2_flash_s_ctrl(), all led_*() function calls inside the function
> > > would try to de-reference a NULL pointer, as the logic won't prevent
> > > it.
> > > 
> > > So, remove the useless check.
> > > 
> > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > > ---
> > >  drivers/media/v4l2-core/v4l2-flash-led-class.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/media/v4l2-core/v4l2-flash-led-class.c b/drivers/media/v4l2-core/v4l2-flash-led-class.c
> > > index 10ddcc48aa17..a1653c635d82 100644
> > > --- a/drivers/media/v4l2-core/v4l2-flash-led-class.c
> > > +++ b/drivers/media/v4l2-core/v4l2-flash-led-class.c
> > > @@ -194,7 +194,7 @@ static int v4l2_flash_s_ctrl(struct v4l2_ctrl *c)
> > >  {
> > >  	struct v4l2_flash *v4l2_flash = v4l2_ctrl_to_v4l2_flash(c);
> > >  	struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
> > > -	struct led_classdev *led_cdev = fled_cdev ? &fled_cdev->led_cdev : NULL;
> > > +	struct led_classdev *led_cdev = &fled_cdev->led_cdev;  
> > 
> > fled_cdev may be NULL here. The reason is that some controls are for flash
> > LEDs only but the same sub-device may also control an indicator. This is
> > covered when the controls are created, so that the NULL pointer isn't
> > dereferenced.
> 
> I double-checked the code: if a a NULL pointer is passed, the calls
> to the leds framework will try to de-reference it or will return an
> error.
> 
> For instance, those will return an error:
> 
> 	static inline int led_set_flash_strobe(struct led_classdev_flash *fled_cdev,
> 	                                        bool state)
> 	{
> 	        if (!fled_cdev)
> 	                return -EINVAL;
> 	        return fled_cdev->ops->strobe_set(fled_cdev, state);
> 	}
> 
> 	#define call_flash_op(fled_cdev, op, args...)           \
> 	        ((has_flash_op(fled_cdev, op)) ?                        \
> 	                        (fled_cdev->ops->op(fled_cdev, args)) : \
> 	                        -EINVAL)
> 
> No big issue here (except that the function will do nothing but
> return an error).
> 
> However, there are places that it will cause it to de-reference 
> a NULL pointer:
> 
> 	int led_set_brightness_sync(struct led_classdev *led_cdev, unsigned int value)
> 	{
> 	        if (led_cdev->blink_delay_on || led_cdev->blink_delay_off)
> 	                return -EBUSY;
> 	
> 	        led_cdev->brightness = min(value, led_cdev->max_brightness);
> 
> 	        if (led_cdev->flags & LED_SUSPENDED)
> 	                return 0;
> 
> 	        return __led_set_brightness_blocking(led_cdev, led_cdev->brightness);
> 	}
> 
> So, this is not a false-positive, but, instead, a real issue.
> 
> So, if led_cdev/fled_cdev can indeed be NULL, IMO, the right solution would be
> to explicitly check it, and return an error, e. g.:
> 
> 	static int v4l2_flash_s_ctrl(struct v4l2_ctrl *c)
> 	{
>         	struct v4l2_flash *v4l2_flash = v4l2_ctrl_to_v4l2_flash(c);
>         	struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
> 		struct led_classdev *led_cdev;
>         	struct v4l2_ctrl **ctrls = v4l2_flash->ctrls;
>         	bool external_strobe;
>         	int ret = 0;
> 
> 		if (!fled_cdev)
> 			return -EINVAL;

The approach is correct, but as noted, the check needs to be done later.

Could you drop this patch, please?

> 
> 		led_cdev = &fled_cdev->led_cdev;
> 
> 		...
> 
> > 
> > If you wish the false positive to be addressed while also improving the
> > implementation, that could be done by e.g. splitting the switch into two,
> > the part that needs fled_cdev and another that doesn't.
> > 
> > I can send a patch for that.
> > 
> > Please also cc me to V4L2 flash class patches. I noticed this one by
> > accident only.
> 
> Better to add you as a reviewer at the MAINTAINERS file, to
> ensure that you'll always be c/c on such code.

There's no separate entry for flash class, just like the rest of the V4L2
core. I think it could be worth addressing this for all the bits in V4L2
core, but that's separate from this issue in any case.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 3/5] media: v4l2-flash-led-class: drop an useless check

[Sakari Ailus]

On Thu, Jun 24, 2021 at 01:14:43PM +0300, Sakari Ailus wrote:

...

> > > On Mon, Jun 21, 2021 at 01:56:47PM +0200, Mauro Carvalho Chehab wrote:
> > > > As pointed by smatch:
> > > > 	drivers/media/v4l2-core/v4l2-flash-led-class.c:264 v4l2_flash_s_ctrl() error: we previously assumed 'fled_cdev' could be null (see line 197)
> > > > 
> > > > It is too late to check if fled_cdev is NULL there. If such check is
> > > > needed, it should be, instead, inside v4l2_flash_init().
> > > > 
> > > > On other words, if v4l2_flash->fled_cdev() is NULL at
> > > > v4l2_flash_s_ctrl(), all led_*() function calls inside the function
> > > > would try to de-reference a NULL pointer, as the logic won't prevent
> > > > it.
> > > > 
> > > > So, remove the useless check.
> > > > 
> > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > > > ---
> > > >  drivers/media/v4l2-core/v4l2-flash-led-class.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/media/v4l2-core/v4l2-flash-led-class.c b/drivers/media/v4l2-core/v4l2-flash-led-class.c
> > > > index 10ddcc48aa17..a1653c635d82 100644
> > > > --- a/drivers/media/v4l2-core/v4l2-flash-led-class.c
> > > > +++ b/drivers/media/v4l2-core/v4l2-flash-led-class.c
> > > > @@ -194,7 +194,7 @@ static int v4l2_flash_s_ctrl(struct v4l2_ctrl *c)
> > > >  {
> > > >  	struct v4l2_flash *v4l2_flash = v4l2_ctrl_to_v4l2_flash(c);
> > > >  	struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
> > > > -	struct led_classdev *led_cdev = fled_cdev ? &fled_cdev->led_cdev : NULL;
> > > > +	struct led_classdev *led_cdev = &fled_cdev->led_cdev;  
> > > 
> > > fled_cdev may be NULL here. The reason is that some controls are for flash
> > > LEDs only but the same sub-device may also control an indicator. This is
> > > covered when the controls are created, so that the NULL pointer isn't
> > > dereferenced.
> > 
> > I double-checked the code: if a a NULL pointer is passed, the calls
> > to the leds framework will try to de-reference it or will return an
> > error.
> > 
> > For instance, those will return an error:
> > 
> > 	static inline int led_set_flash_strobe(struct led_classdev_flash *fled_cdev,
> > 	                                        bool state)
> > 	{
> > 	        if (!fled_cdev)
> > 	                return -EINVAL;
> > 	        return fled_cdev->ops->strobe_set(fled_cdev, state);
> > 	}
> > 
> > 	#define call_flash_op(fled_cdev, op, args...)           \
> > 	        ((has_flash_op(fled_cdev, op)) ?                        \
> > 	                        (fled_cdev->ops->op(fled_cdev, args)) : \
> > 	                        -EINVAL)
> > 
> > No big issue here (except that the function will do nothing but
> > return an error).
> > 
> > However, there are places that it will cause it to de-reference 
> > a NULL pointer:
> > 
> > 	int led_set_brightness_sync(struct led_classdev *led_cdev, unsigned int value)
> > 	{
> > 	        if (led_cdev->blink_delay_on || led_cdev->blink_delay_off)
> > 	                return -EBUSY;
> > 	
> > 	        led_cdev->brightness = min(value, led_cdev->max_brightness);
> > 
> > 	        if (led_cdev->flags & LED_SUSPENDED)
> > 	                return 0;
> > 
> > 	        return __led_set_brightness_blocking(led_cdev, led_cdev->brightness);
> > 	}
> > 
> > So, this is not a false-positive, but, instead, a real issue.
> > 
> > So, if led_cdev/fled_cdev can indeed be NULL, IMO, the right solution would be
> > to explicitly check it, and return an error, e. g.:
> > 
> > 	static int v4l2_flash_s_ctrl(struct v4l2_ctrl *c)
> > 	{
> >         	struct v4l2_flash *v4l2_flash = v4l2_ctrl_to_v4l2_flash(c);
> >         	struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
> > 		struct led_classdev *led_cdev;
> >         	struct v4l2_ctrl **ctrls = v4l2_flash->ctrls;
> >         	bool external_strobe;
> >         	int ret = 0;
> > 
> > 		if (!fled_cdev)
> > 			return -EINVAL;
> 
> The approach is correct, but as noted, the check needs to be done later.

I checked that the same pattern is used throughout much of the file. I
suppose if smatch isn't happy with this instance, it may not be happy with
the rest either. Admittedly, the pattern isn't entirely trouble-free, as it
requires the parts of the file to be in sync.

Addressing this takes probably a few patches at least.

-- 
Sakari Ailus

Re: [PATCH 3/5] media: v4l2-flash-led-class: drop an useless check

[Mauro Carvalho Chehab]

Em Thu, 24 Jun 2021 13:14:43 +0300
Sakari Ailus <sakari.ailus@iki.fi> escreveu:

> Hi Mauro,
> 
> On Thu, Jun 24, 2021 at 11:59:25AM +0200, Mauro Carvalho Chehab wrote:
> > Em Thu, 24 Jun 2021 12:31:53 +0300
> > Sakari Ailus <sakari.ailus@iki.fi> escreveu:
> >   
> > > Hi Mauro,
> > > 
> > > Could you check if your mail client could be configured not to add junk to
> > > To: field? It often leads anything in the Cc: field being dropped.  
> > 
> > I have no idea why it is doing that. I'm just using git send-email
> > here. Perhaps a git bug?
> > 
> > 	$ git --version
> > 	git version 2.31.1
> > 
> > The setup is like this one:
> > 
> > 	[sendemail]
> > 	        confirm = always
> > 	        multiedit = true
> > 	        chainreplyto = false
> > 	        aliasesfile = /home/mchehab/.addressbook
> > 	        aliasfiletype = pine
> > 	        assume8bitencoding = UTF-8  
> 
> I tried sending a message to myself using git send-email with an empty To:
> field and it came through just fine, with To: field remaining empty. I
> wonder if it could be the list server?

It seems so.

> > So, this is not a false-positive, but, instead, a real issue.
> > 
> > So, if led_cdev/fled_cdev can indeed be NULL, IMO, the right solution would be
> > to explicitly check it, and return an error, e. g.:
> > 
> > 	static int v4l2_flash_s_ctrl(struct v4l2_ctrl *c)
> > 	{
> >         	struct v4l2_flash *v4l2_flash = v4l2_ctrl_to_v4l2_flash(c);
> >         	struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
> > 		struct led_classdev *led_cdev;
> >         	struct v4l2_ctrl **ctrls = v4l2_flash->ctrls;
> >         	bool external_strobe;
> >         	int ret = 0;
> > 
> > 		if (!fled_cdev)
> > 			return -EINVAL;  
> 
> The approach is correct, but as noted, the check needs to be done later.

> I checked that the same pattern is used throughout much of the file. I
> suppose if smatch isn't happy with this instance, it may not be happy with
> the rest either. Admittedly, the pattern isn't entirely trouble-free, as it
> requires the parts of the file to be in sync.
>
> Addressing this takes probably a few patches at least.

See, the main issue is not the smatch report, but the point that, on
some cases, it will de-reference a NULL pointer.

And yeah, the same pattern is everywhere within the core.

IMO, the right fix would be to ensure that fled_cdev will always
be not NULL, but if there are good reasons why this can't happen,
extra checks are needed along the core (or at leds core), in order
to prevent de-referencing NULL pointers.

> 
> Could you drop this patch, please?

Just dropped from media_stage. It didn't reach media_tree.

> > > Please also cc me to V4L2 flash class patches. I noticed this one by
> > > accident only.  
> > 
> > Better to add you as a reviewer at the MAINTAINERS file, to
> > ensure that you'll always be c/c on such code.  
> 
> There's no separate entry for flash class, just like the rest of the V4L2
> core. I think it could be worth addressing this for all the bits in V4L2
> core, but that's separate from this issue in any case.

It makes sense to add entries at MAINTAINERS, but yeah, this
is OOT here.

Thanks,
Mauro

Re: [PATCH 3/5] media: v4l2-flash-led-class: drop an useless check

[Sakari Ailus]

On Thu, Jun 24, 2021 at 01:32:38PM +0200, Mauro Carvalho Chehab wrote:
> Em Thu, 24 Jun 2021 13:14:43 +0300
> Sakari Ailus <sakari.ailus@iki.fi> escreveu:
> 
> > Hi Mauro,
> > 
> > On Thu, Jun 24, 2021 at 11:59:25AM +0200, Mauro Carvalho Chehab wrote:
> > > Em Thu, 24 Jun 2021 12:31:53 +0300
> > > Sakari Ailus <sakari.ailus@iki.fi> escreveu:
> > >   
> > > > Hi Mauro,
> > > > 
> > > > Could you check if your mail client could be configured not to add junk to
> > > > To: field? It often leads anything in the Cc: field being dropped.  
> > > 
> > > I have no idea why it is doing that. I'm just using git send-email
> > > here. Perhaps a git bug?
> > > 
> > > 	$ git --version
> > > 	git version 2.31.1
> > > 
> > > The setup is like this one:
> > > 
> > > 	[sendemail]
> > > 	        confirm = always
> > > 	        multiedit = true
> > > 	        chainreplyto = false
> > > 	        aliasesfile = /home/mchehab/.addressbook
> > > 	        aliasfiletype = pine
> > > 	        assume8bitencoding = UTF-8  
> > 
> > I tried sending a message to myself using git send-email with an empty To:
> > field and it came through just fine, with To: field remaining empty. I
> > wonder if it could be the list server?
> 
> It seems so.
> 
> > > So, this is not a false-positive, but, instead, a real issue.
> > > 
> > > So, if led_cdev/fled_cdev can indeed be NULL, IMO, the right solution would be
> > > to explicitly check it, and return an error, e. g.:
> > > 
> > > 	static int v4l2_flash_s_ctrl(struct v4l2_ctrl *c)
> > > 	{
> > >         	struct v4l2_flash *v4l2_flash = v4l2_ctrl_to_v4l2_flash(c);
> > >         	struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
> > > 		struct led_classdev *led_cdev;
> > >         	struct v4l2_ctrl **ctrls = v4l2_flash->ctrls;
> > >         	bool external_strobe;
> > >         	int ret = 0;
> > > 
> > > 		if (!fled_cdev)
> > > 			return -EINVAL;  
> > 
> > The approach is correct, but as noted, the check needs to be done later.
> 
> > I checked that the same pattern is used throughout much of the file. I
> > suppose if smatch isn't happy with this instance, it may not be happy with
> > the rest either. Admittedly, the pattern isn't entirely trouble-free, as it
> > requires the parts of the file to be in sync.
> >
> > Addressing this takes probably a few patches at least.
> 
> See, the main issue is not the smatch report, but the point that, on
> some cases, it will de-reference a NULL pointer.

It does not, since the controls aren't added for devices that do not have
these parts to control. For instance, if there's no flash LED, the flash
related controls aren't created. So this is primariy a static checker
issue, and secondarily perhaps an issue of the cleanness of the code. But
much of that originates from how the LED flash API connects with the V4L2
flash API.

> 
> And yeah, the same pattern is everywhere within the core.
> 
> IMO, the right fix would be to ensure that fled_cdev will always
> be not NULL, but if there are good reasons why this can't happen,
> extra checks are needed along the core (or at leds core), in order
> to prevent de-referencing NULL pointers.
> 
> > 
> > Could you drop this patch, please?
> 
> Just dropped from media_stage. It didn't reach media_tree.

Thanks.

-- 
Sakari Ailus