Re: [PATCH 05/10] backlight: qcom-wled: Fix off-by-one maximum with default num_strings

[Daniel Thompson <daniel.thompson@linaro.org>]

On Tue, Oct 05, 2021 at 12:06:06PM +0200, Marijn Suijten wrote:
> On 2021-10-05 10:19:47, Daniel Thompson wrote:
> > On Mon, Oct 04, 2021 at 09:27:36PM +0200, Marijn Suijten wrote:
> > > When not specifying num-strings in the DT the default is used, but +1 is
> > > added to it which turns wled3 into 4 and wled4/5 into 5 strings instead
> > > of 3 and 4 respectively, causing out of bounds reads and register
> > > read/writes.  This +1 exists for a deficiency in the DT parsing code,
> > > and is simply omitted entirely - solving this oob issue - by allowing
> > > one extra iteration of the wled_var_cfg function parsing this particular
> > > property.
> > > 
> > > Fixes: 93c64f1ea1e8 ("leds: add Qualcomm PM8941 WLED driver")
> > > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> > > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> > > ---
> > >  drivers/video/backlight/qcom-wled.c | 8 +++-----
> > >  1 file changed, 3 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
> > > index 27e8949c7922..66ce77ee3099 100644
> > > --- a/drivers/video/backlight/qcom-wled.c
> > > +++ b/drivers/video/backlight/qcom-wled.c
> > > @@ -1255,17 +1255,17 @@ static const struct wled_var_cfg wled5_ovp_cfg = {
> > >  
> > >  static u32 wled3_num_strings_values_fn(u32 idx)
> > >  {
> > > -	return idx + 1;
> > > +	return idx;


> > >  }
> > >  
> > >  static const struct wled_var_cfg wled3_num_strings_cfg = {
> > >  	.fn = wled3_num_strings_values_fn,
> > > -	.size = 3,
> > > +	.size = 4, /* [0, 3] */
> > 
> > 0 is not a valid value for this property.
> 
> These comments represent the possible loop iterations the DT "cfg
> parser" runs through, starting at j=0 and running up until and including
> j=3.  Should I make that more clear or omit these comments entirely?

The role of wled3_num_strings_values_fn() is to enumerate the list of
legal values for the property [ 1, 2, 3 ]. Your changes cause the
enumeration to include a non-legal value so that you can have an
identity mapping between the symbol and the enumerate value.

An alternative approach would be to leave the enumeration logic
alone but set the num_string default to UINT_MAX in all cases:

-	cfg->num_strings = cfg->num_strings + 1;
+	if (cfg->num_strings == UINT_MAX)
+		cfg->num_strings = 
+	else
+               /* Convert from enumerated to numeric form */
+		cfg->num_strings = wled3_num_strings_values_fn(
+						cfg->num_strings);


Daniel.