diff mbox series

[RESEND,v2,04/13] backlight: qcom-wled: Fix off-by-one maximum with default num_strings

Message ID 20211112002706.453289-5-marijn.suijten@somainline.org (mailing list archive)
State Not Applicable
Headers show
Series backlight: qcom-wled: fix and solidify handling of enabled-strings | expand

Commit Message

Marijn Suijten Nov. 12, 2021, 12:26 a.m. UTC 
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 parsing the
property separately much like qcom,enabled-strings.

This also allows more stringent checks on the maximum value when
qcom,enabled-strings is provided in the DT.  Note that num-strings is
parsed after enabled-strings to give it final sign-off over the length,
which DT currently utilizes to get around an incorrect fixed read of
four elements from that array (has been addressed in a prior patch).

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 | 51 +++++++++++------------------
 1 file changed, 19 insertions(+), 32 deletions(-)

--
2.33.0

Comments

Daniel Thompson Nov. 12, 2021, 12:08 p.m. UTC | #1 
On Fri, Nov 12, 2021 at 01:26:57AM +0100, 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 parsing the
> property separately much like qcom,enabled-strings.
> 
> This also allows more stringent checks on the maximum value when
> qcom,enabled-strings is provided in the DT.  Note that num-strings is
> parsed after enabled-strings to give it final sign-off over the length,
> which DT currently utilizes to get around an incorrect fixed read of
> four elements from that array (has been addressed in a prior patch).
> 
> 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 | 51 +++++++++++------------------
>  1 file changed, 19 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
> index 977cd75827d7..c5232478a343 100644
> --- a/drivers/video/backlight/qcom-wled.c
> +++ b/drivers/video/backlight/qcom-wled.c
> @@ -1552,6 +1520,25 @@ static int wled_configure(struct wled *wled)
>  		}
>  	}
> 
> +	rc = of_property_read_u32(dev->of_node, "qcom,num-strings", &val);
> +	if (!rc) {
> +		if (val < 1 || val > wled->max_string_count) {
> +			dev_err(dev, "qcom,num-strings must be between 1 and %d\n",
> +				wled->max_string_count);
> +			return -EINVAL;
> +		}
> +
> +		if (string_len > 0) {
> +			dev_warn(dev, "qcom,num-strings and qcom,enabled-strings are ambiguous\n");

This warning occurs even when there is no ambiguity.

This could be:

	if (string_len > 0 && val != string_len)

The warning should also be below the error message on the next if statement.
Combined these changes allows us to give a much more helpful and assertive
warning message:

qcom,num-strings mis-matches and will partially override
qcom,enabled-strings (remove qcom,num-strings?)


> +			if (val > string_len) {
> +				dev_err(dev, "qcom,num-strings exceeds qcom,enabled-strings\n");
> +				return -EINVAL;
> +			}
> +		}


Daniel.
Marijn Suijten Nov. 12, 2021, 12:35 p.m. UTC | #2 
On 2021-11-12 12:08:39, Daniel Thompson wrote:
> On Fri, Nov 12, 2021 at 01:26:57AM +0100, 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 parsing the
> > property separately much like qcom,enabled-strings.
> > 
> > This also allows more stringent checks on the maximum value when
> > qcom,enabled-strings is provided in the DT.  Note that num-strings is
> > parsed after enabled-strings to give it final sign-off over the length,
> > which DT currently utilizes to get around an incorrect fixed read of
> > four elements from that array (has been addressed in a prior patch).
> > 
> > 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 | 51 +++++++++++------------------
> >  1 file changed, 19 insertions(+), 32 deletions(-)
> > 
> > diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
> > index 977cd75827d7..c5232478a343 100644
> > --- a/drivers/video/backlight/qcom-wled.c
> > +++ b/drivers/video/backlight/qcom-wled.c
> > @@ -1552,6 +1520,25 @@ static int wled_configure(struct wled *wled)
> >  		}
> >  	}
> > 
> > +	rc = of_property_read_u32(dev->of_node, "qcom,num-strings", &val);
> > +	if (!rc) {
> > +		if (val < 1 || val > wled->max_string_count) {
> > +			dev_err(dev, "qcom,num-strings must be between 1 and %d\n",
> > +				wled->max_string_count);
> > +			return -EINVAL;
> > +		}
> > +
> > +		if (string_len > 0) {
> > +			dev_warn(dev, "qcom,num-strings and qcom,enabled-strings are ambiguous\n");
> 
> The warning should also be below the error message on the next if statement.

Agreed.

> This warning occurs even when there is no ambiguity.
> 
> This could be:
> 
> 	if (string_len > 0 && val != string_len)
> 
> Combined these changes allows us to give a much more helpful and assertive
> warning message:
> 
> qcom,num-strings mis-matches and will partially override
> qcom,enabled-strings (remove qcom,num-strings?)

I want to let the user know it's set regardless of whether they're
equivalent; no need to set both.

How about:

    Only one of qcom,num-strings or qcom,enabled-strings should be set

That should be more descriptive?  Otherwise, let me know if you really
want to allow users to (unnecessarily) set both - or if it can / should
be caught in DT validation instead.

- Marijn

> > +			if (val > string_len) {
> > +				dev_err(dev, "qcom,num-strings exceeds qcom,enabled-strings\n");
> > +				return -EINVAL;
> > +			}
> > +		}
> 
> 
> Daniel.
Daniel Thompson Nov. 12, 2021, 1:19 p.m. UTC | #3 
On Fri, Nov 12, 2021 at 01:35:01PM +0100, Marijn Suijten wrote:
> On 2021-11-12 12:08:39, Daniel Thompson wrote:
> > On Fri, Nov 12, 2021 at 01:26:57AM +0100, Marijn Suijten wrote:
> > > +		if (string_len > 0) {
> > > +			dev_warn(dev, "qcom,num-strings and qcom,enabled-strings are ambiguous\n");
> > 
> > The warning should also be below the error message on the next if statement.
> 
> Agreed.
> 
> > This warning occurs even when there is no ambiguity.
> > 
> > This could be:
> > 
> > 	if (string_len > 0 && val != string_len)
> > 
> > Combined these changes allows us to give a much more helpful and assertive
> > warning message:
> > 
> > qcom,num-strings mis-matches and will partially override
> > qcom,enabled-strings (remove qcom,num-strings?)
> 
> I want to let the user know it's set regardless of whether they're
> equivalent; no need to set both.
> 
> How about:
> 
>     Only one of qcom,num-strings or qcom,enabled-strings should be set
> 
> That should be more descriptive?  Otherwise, let me know if you really
> want to allow users to (unnecessarily) set both - or if it can / should
> be caught in DT validation instead.

Yes. I can live with that text. Let's use that.

Maybe I wouldn't if there gazilions of existing DTs with both
properties but IIRC the number is likely to be small or zero
(although we couldn't be 100% sure which).


Daniel.
Marijn Suijten Nov. 12, 2021, 9:43 p.m. UTC | #4 
On 2021-11-12 13:35:03, Marijn Suijten wrote:
> On 2021-11-12 12:08:39, Daniel Thompson wrote:
> > On Fri, Nov 12, 2021 at 01:26:57AM +0100, 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 parsing the
> > > property separately much like qcom,enabled-strings.
> > > 
> > > This also allows more stringent checks on the maximum value when
> > > qcom,enabled-strings is provided in the DT.  Note that num-strings is
> > > parsed after enabled-strings to give it final sign-off over the length,
> > > which DT currently utilizes to get around an incorrect fixed read of
> > > four elements from that array (has been addressed in a prior patch).
> > > 
> > > 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 | 51 +++++++++++------------------
> > >  1 file changed, 19 insertions(+), 32 deletions(-)
> > > 
> > > diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
> > > index 977cd75827d7..c5232478a343 100644
> > > --- a/drivers/video/backlight/qcom-wled.c
> > > +++ b/drivers/video/backlight/qcom-wled.c
> > > @@ -1552,6 +1520,25 @@ static int wled_configure(struct wled *wled)
> > >  		}
> > >  	}
> > > 
> > > +	rc = of_property_read_u32(dev->of_node, "qcom,num-strings", &val);
> > > +	if (!rc) {
> > > +		if (val < 1 || val > wled->max_string_count) {
> > > +			dev_err(dev, "qcom,num-strings must be between 1 and %d\n",
> > > +				wled->max_string_count);
> > > +			return -EINVAL;
> > > +		}
> > > +
> > > +		if (string_len > 0) {
> > > +			dev_warn(dev, "qcom,num-strings and qcom,enabled-strings are ambiguous\n");
> > 
> > The warning should also be below the error message on the next if statement.
> 
> Agreed.

Thinking about this again while reworking the patches, I initially put
this above the error to make DT writers aware.  There's no point telling
them that their values are out of sync (num-strings >
len(enabled-strings)), when they "shouldn't even" (don't need to) set
both in the first place.  They might needlessly fix the discrepancy, see
the driver finally probe (working backlight) and carry on without
noticing this warning that now appears.

Sorry for bringing this back up, but I'm curious about your opinion.

- Marijn
Daniel Thompson Nov. 15, 2021, 11:23 a.m. UTC | #5 
On Fri, Nov 12, 2021 at 10:43:37PM +0100, Marijn Suijten wrote:
> On 2021-11-12 13:35:03, Marijn Suijten wrote:
> > On 2021-11-12 12:08:39, Daniel Thompson wrote:
> > > On Fri, Nov 12, 2021 at 01:26:57AM +0100, 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 parsing the
> > > > property separately much like qcom,enabled-strings.
> > > > 
> > > > This also allows more stringent checks on the maximum value when
> > > > qcom,enabled-strings is provided in the DT.  Note that num-strings is
> > > > parsed after enabled-strings to give it final sign-off over the length,
> > > > which DT currently utilizes to get around an incorrect fixed read of
> > > > four elements from that array (has been addressed in a prior patch).
> > > > 
> > > > 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 | 51 +++++++++++------------------
> > > >  1 file changed, 19 insertions(+), 32 deletions(-)
> > > > 
> > > > diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
> > > > index 977cd75827d7..c5232478a343 100644
> > > > --- a/drivers/video/backlight/qcom-wled.c
> > > > +++ b/drivers/video/backlight/qcom-wled.c
> > > > @@ -1552,6 +1520,25 @@ static int wled_configure(struct wled *wled)
> > > >  		}
> > > >  	}
> > > > 
> > > > +	rc = of_property_read_u32(dev->of_node, "qcom,num-strings", &val);
> > > > +	if (!rc) {
> > > > +		if (val < 1 || val > wled->max_string_count) {
> > > > +			dev_err(dev, "qcom,num-strings must be between 1 and %d\n",
> > > > +				wled->max_string_count);
> > > > +			return -EINVAL;
> > > > +		}
> > > > +
> > > > +		if (string_len > 0) {
> > > > +			dev_warn(dev, "qcom,num-strings and qcom,enabled-strings are ambiguous\n");
> > > 
> > > The warning should also be below the error message on the next if statement.
> > 
> > Agreed.
> 
> Thinking about this again while reworking the patches, I initially put
> this above the error to make DT writers aware.  There's no point telling
> them that their values are out of sync (num-strings >
> len(enabled-strings)), when they "shouldn't even" (don't need to) set
> both in the first place.  They might needlessly fix the discrepancy, see
> the driver finally probe (working backlight) and carry on without
> noticing this warning that now appears.
> 
> Sorry for bringing this back up, but I'm curious about your opinion.

With a more helpful warning about how to fix then I think it is OK to
have both the warning and the error.


Daniel.
Marijn Suijten Nov. 15, 2021, 7:46 p.m. UTC | #6 
On 2021-11-15 11:23:27, Daniel Thompson wrote:
> On Fri, Nov 12, 2021 at 10:43:37PM +0100, Marijn Suijten wrote:
> > On 2021-11-12 13:35:03, Marijn Suijten wrote:
> > > On 2021-11-12 12:08:39, Daniel Thompson wrote:
> > > > On Fri, Nov 12, 2021 at 01:26:57AM +0100, 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 parsing the
> > > > > property separately much like qcom,enabled-strings.
> > > > > 
> > > > > This also allows more stringent checks on the maximum value when
> > > > > qcom,enabled-strings is provided in the DT.  Note that num-strings is
> > > > > parsed after enabled-strings to give it final sign-off over the length,
> > > > > which DT currently utilizes to get around an incorrect fixed read of
> > > > > four elements from that array (has been addressed in a prior patch).
> > > > > 
> > > > > 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 | 51 +++++++++++------------------
> > > > >  1 file changed, 19 insertions(+), 32 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
> > > > > index 977cd75827d7..c5232478a343 100644
> > > > > --- a/drivers/video/backlight/qcom-wled.c
> > > > > +++ b/drivers/video/backlight/qcom-wled.c
> > > > > @@ -1552,6 +1520,25 @@ static int wled_configure(struct wled *wled)
> > > > >  		}
> > > > >  	}
> > > > > 
> > > > > +	rc = of_property_read_u32(dev->of_node, "qcom,num-strings", &val);
> > > > > +	if (!rc) {
> > > > > +		if (val < 1 || val > wled->max_string_count) {
> > > > > +			dev_err(dev, "qcom,num-strings must be between 1 and %d\n",
> > > > > +				wled->max_string_count);
> > > > > +			return -EINVAL;
> > > > > +		}
> > > > > +
> > > > > +		if (string_len > 0) {
> > > > > +			dev_warn(dev, "qcom,num-strings and qcom,enabled-strings are ambiguous\n");
> > > > 
> > > > The warning should also be below the error message on the next if statement.
> > > 
> > > Agreed.
> > 
> > Thinking about this again while reworking the patches, I initially put
> > this above the error to make DT writers aware.  There's no point telling
> > them that their values are out of sync (num-strings >
> > len(enabled-strings)), when they "shouldn't even" (don't need to) set
> > both in the first place.  They might needlessly fix the discrepancy, see
> > the driver finally probe (working backlight) and carry on without
> > noticing this warning that now appears.
> > 
> > Sorry for bringing this back up, but I'm curious about your opinion.
> 
> With a more helpful warning about how to fix then I think it is OK to
> have both the warning and the error.

Thanks - I presume the message we settled upon last time is helpful
enough:

    Only one of qcom,num-strings or qcom,enabled-strings should be set

I'll respin this, together with this warning reordered into the next
commit, and using __le16 for the cpu_to_le16 output.

- Marijn
diff mbox series

Patch

diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
index 977cd75827d7..c5232478a343 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c
@@ -1253,21 +1253,6 @@  static const struct wled_var_cfg wled5_ovp_cfg = {
 	.size = 16,
 };

-static u32 wled3_num_strings_values_fn(u32 idx)
-{
-	return idx + 1;
-}
-
-static const struct wled_var_cfg wled3_num_strings_cfg = {
-	.fn = wled3_num_strings_values_fn,
-	.size = 3,
-};
-
-static const struct wled_var_cfg wled4_num_strings_cfg = {
-	.fn = wled3_num_strings_values_fn,
-	.size = 4,
-};
-
 static u32 wled3_switch_freq_values_fn(u32 idx)
 {
 	return 19200 / (2 * (1 + idx));
@@ -1341,11 +1326,6 @@  static int wled_configure(struct wled *wled)
 			.val_ptr = &cfg->switch_freq,
 			.cfg = &wled3_switch_freq_cfg,
 		},
-		{
-			.name = "qcom,num-strings",
-			.val_ptr = &cfg->num_strings,
-			.cfg = &wled3_num_strings_cfg,
-		},
 	};

 	const struct wled_u32_opts wled4_opts[] = {
@@ -1369,11 +1349,6 @@  static int wled_configure(struct wled *wled)
 			.val_ptr = &cfg->switch_freq,
 			.cfg = &wled3_switch_freq_cfg,
 		},
-		{
-			.name = "qcom,num-strings",
-			.val_ptr = &cfg->num_strings,
-			.cfg = &wled4_num_strings_cfg,
-		},
 	};

 	const struct wled_u32_opts wled5_opts[] = {
@@ -1397,11 +1372,6 @@  static int wled_configure(struct wled *wled)
 			.val_ptr = &cfg->switch_freq,
 			.cfg = &wled3_switch_freq_cfg,
 		},
-		{
-			.name = "qcom,num-strings",
-			.val_ptr = &cfg->num_strings,
-			.cfg = &wled4_num_strings_cfg,
-		},
 		{
 			.name = "qcom,modulator-sel",
 			.val_ptr = &cfg->mod_sel,
@@ -1520,8 +1490,6 @@  static int wled_configure(struct wled *wled)
 			*bool_opts[i].val_ptr = true;
 	}

-	cfg->num_strings = cfg->num_strings + 1;
-
 	string_len = of_property_count_elems_of_size(dev->of_node,
 						     "qcom,enabled-strings",
 						     sizeof(u32));
@@ -1552,6 +1520,25 @@  static int wled_configure(struct wled *wled)
 		}
 	}

+	rc = of_property_read_u32(dev->of_node, "qcom,num-strings", &val);
+	if (!rc) {
+		if (val < 1 || val > wled->max_string_count) {
+			dev_err(dev, "qcom,num-strings must be between 1 and %d\n",
+				wled->max_string_count);
+			return -EINVAL;
+		}
+
+		if (string_len > 0) {
+			dev_warn(dev, "qcom,num-strings and qcom,enabled-strings are ambiguous\n");
+			if (val > string_len) {
+				dev_err(dev, "qcom,num-strings exceeds qcom,enabled-strings\n");
+				return -EINVAL;
+			}
+		}
+
+		cfg->num_strings = val;
+	}
+
 	return 0;
 }