Re: [PATCH v2] media: ivtv: prevent going past the hw arrays

From: Hans Verkuil
Date: Mon Jun 21 2021 - 13:18:55 EST

On 21/06/2021 18:39, 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@xxxxxxxxxx>

Reviewed-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>

Thanks!

Hans

> ---
> 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..c252733df340 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,
> + IVTV_HW_BIT_SAA7115,
> + IVTV_HW_BIT_SAA7127,
> + IVTV_HW_BIT_MSP34XX,
> + IVTV_HW_BIT_TUNER,
> + IVTV_HW_BIT_WM8775,
> + IVTV_HW_BIT_CS53L32A,
> + IVTV_HW_BIT_TVEEPROM,
> + IVTV_HW_BIT_SAA7114,
> + IVTV_HW_BIT_UPD64031A,
> + IVTV_HW_BIT_UPD6408X,
> + IVTV_HW_BIT_SAA717X,
> + IVTV_HW_BIT_WM8739,
> + IVTV_HW_BIT_VP27SMPX,
> + IVTV_HW_BIT_M52790,
> + IVTV_HW_BIT_GPIO,
> + IVTV_HW_BIT_I2C_IR_RX_AVER,
> + IVTV_HW_BIT_I2C_IR_RX_HAUP_EXT, /* External before internal */
> + IVTV_HW_BIT_I2C_IR_RX_HAUP_INT,
> + IVTV_HW_BIT_Z8F0811_IR_HAUP,
> + IVTV_HW_BIT_I2C_IR_RX_ADAPTEC,
> +
> + IVTV_HW_MAX_BITS /* Should be the last one */
> +};
> +
> +#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 */
>