Re: [PATCH 8/8] Input: cap11xx - add support for CAP1114

[Jun Yan <jerrysteve1101@gmail.com>]

> Thank you for your contribution! Sashiko AI review found 3 potential issue(=
> s) to consider:
> - [High] Buffer overflow when reading the 'microchip,calib-sensitivity' dev=
> ice tree property for devices with >8 channels.
> - [High] Failure to mask `STATUS1` before ORing `STATUS2` can cause phantom=
>  key presses for channels 7 and 8.
> - [Low] Bitmask calculation for 'microchip,signal-guard' truncates settings=
>  for channels 8 and above.
> --
> 
> commit 8885b1d0f98cb8d5ca53ba5fd9a458743481d5be
> Author: Jun Yan <jerrysteve1101@gmail.com>
> 
> Input: cap11xx - add support for CAP1114
> 
> CAP1114 is a 14-channel capacitive touch sensor with 11 LED outputs
> and hardware reset support. The CAP1114 uses two separate control registers
> for LED output management and requires two button status registers for touch
> input state reporting.
> 
> > diff --git a/drivers/input/keyboard/cap11xx.c b/drivers/input/keyboard/ca=
> p11xx.c
> > index 408880cf65167..2aba3b508947d 100644
> > --- a/drivers/input/keyboard/cap11xx.c
> > +++ b/drivers/input/keyboard/cap11xx.c
> 
> [ ... ]
> 
> > @@ -311,6 +337,20 @@ static irqreturn_t cap11xx_thread_func(int irq_num, =
> void *data)
> >  	if (ret < 0)
> >  		goto out;
> > =20
> > +	if (priv->model->num_channels > 8) {
> > +		unsigned int status2;
> > +
> > +		ret =3D regmap_read(priv->regmap, priv->model->sensor_input_reg_base +=
>  1, &status2);
> > +		if (ret < 0)
> > +			goto out;
> > +
> > +		/*
> > +		 * CAP1114 STATUS1 register only contains data for the first 6 channel=
> s.
> > +		 * the remaining channels is stored in STATUS2.
> > +		 */
> > +		status |=3D FIELD_PREP(GENMASK(13, 6), status2);
> 
> [Severity: High]
> If the hardware sets reserved bits 6 or 7 in the first status register to 1,
> will they be preserved and incorrectly reported as touch events for channels
> 7 and 8?
> 
> Since the status variable (which holds STATUS1) is not masked before this OR
> operation, any reserved bits set in STATUS1 could overlap with the shifted
> status2 values.
> 
> Should the status variable be masked to clear bits 6 and 7 before ORing in
> status2?
> 

Thanks. Will mask bits 6/7 via status &= GENMASK(5, 0) after reading STATUS1 in v2.

> > +	}
> > +
> >  	for (i =3D 0; i < priv->idev->keycodemax; i++)
> 
> [ ... ]
> 
> > @@ -572,6 +625,14 @@ static const struct cap11xx_hw_model cap1106_model =
> =3D {
> >  	.has_repeat_en =3D true,
> >  };
> > =20
> > +static const struct cap11xx_hw_model cap1114_model =3D {
> > +	.product_id =3D 0x3a,
> > +	.led_output_control_reg_base =3D CAP1114_REG_LED_OUTPUT_CONTROL1,
> > +	.sensor_input_reg_base =3D CAP1114_REG_BUTTON_STATUS1,
> > +	.num_channels =3D 14, .num_leds =3D 11, .num_sensor_thresholds =3D 8,
> 
> [Severity: High]
> Does configuring num_channels to 14 cause a buffer overflow when parsing the
> device tree?
> 
> In struct cap11xx_priv, calib_sensitivities is defined as a fixed 8-element
> array:
> 
> struct cap11xx_priv {
>         ...
>         u32 calib_sensitivities[8];
>         u32 keycodes[];
> };
> 
> When cap11xx_init_keys() parses the microchip,calib-sensitivity property, it
> requests num_channels elements:
> 
> of_property_read_u32_array(node, "microchip,calib-sensitivity",
>                            priv->calib_sensitivities,
>                            priv->model->num_channels)
> 
> If the device tree provides 14 elements, won't this write 56 bytes into the
> 32-byte array, overflowing into the adjacent keycodes flexible array?
> 
> [Severity: Low]
> Additionally, does setting num_channels to 14 cause truncation in the
> microchip,signal-guard bitmask logic?
> 
> In cap11xx_init_keys(), priv->signal_guard_inputs_mask is typed as a u8, but
> the initialization loop runs up to num_channels:

I will exclude CAP1114 from using microchip,calib-sensitivity and microchip,
signal-guard in the dt-bindings to resolve these two issues in V2.

> 
> if (u32_val)
>         priv->signal_guard_inputs_mask |=3D 0x01 << i;
> 
> For i >=3D 8, does the shifted bit exceed the 8-bit width and get silently
> truncated to 0 during the assignment?
> 
> > +	.has_grouped_sensors =3D true,
> > +};
> > +
> 
> --=20
> Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260606150458.2506=
> 06-1-jerrysteve1101@gmail.com?part=3D8