| Message ID | 20211004192741.621870-5-marijn.suijten@somainline.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | backlight: qcom-wled: fix and solidify handling of enabled-strings | expand |
On Mon, Oct 04, 2021 at 09:27:35PM +0200, Marijn Suijten wrote: > The strings passed in DT may possibly cause out-of-bounds register > accesses and should be validated before use. > > Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for WLED3") The first half of this patch actually fixes patch 1 from this patch set. It would be better to move that code there. Daniel. > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org> > --- > drivers/video/backlight/qcom-wled.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c > index 29910e603c42..27e8949c7922 100644 > --- a/drivers/video/backlight/qcom-wled.c > +++ b/drivers/video/backlight/qcom-wled.c > @@ -1526,6 +1526,12 @@ static int wled_configure(struct wled *wled) > "qcom,enabled-strings", > sizeof(u32)); > if (string_len > 0) { > + if (string_len > wled->max_string_count) { > + dev_err(dev, "Cannot have more than %d strings\n", > + wled->max_string_count); > + return -EINVAL; > + } > + > rc = of_property_read_u32_array(dev->of_node, > "qcom,enabled-strings", > wled->cfg.enabled_strings, > @@ -1537,6 +1543,14 @@ static int wled_configure(struct wled *wled) > return -EINVAL; > } > > + for (i = 0; i < string_len; ++i) { > + if (wled->cfg.enabled_strings[i] >= wled->max_string_count) { > + dev_err(dev, "qcom,enabled-strings index %d at %d is out of bounds\n", > + wled->cfg.enabled_strings[i], i); > + return -EINVAL; > + } > + } > + > cfg->num_strings = string_len; > } > > -- > 2.33.0 >
On 2021-10-05 10:14:52, Daniel Thompson wrote: > On Mon, Oct 04, 2021 at 09:27:35PM +0200, Marijn Suijten wrote: > > The strings passed in DT may possibly cause out-of-bounds register > > accesses and should be validated before use. > > > > Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for WLED3") > > The first half of this patch actually fixes patch 1 from this patch set. > It would be better to move that code there. It only helps guarding against a maximum of 3 leds for WLED3, while using string_len instead of an unintentional sizeof(u32) (resulting in a fixed size of 4) is a different issue requiring a separate patch to fix. Would it help to reorder this patch before 1/10, and mention in patch 1/10 (then 2/10) that, besides properly using string_len instead of hardcoded 4 (which causes wrong reads from DT on top of this), it relies on the previous patch to prevent against an array longer than 3 for WLED3? - Marijn > Daniel. > > > > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> > > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org> > > --- > > drivers/video/backlight/qcom-wled.c | 14 ++++++++++++++ > > 1 file changed, 14 insertions(+) > > > > diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c > > index 29910e603c42..27e8949c7922 100644 > > --- a/drivers/video/backlight/qcom-wled.c > > +++ b/drivers/video/backlight/qcom-wled.c > > @@ -1526,6 +1526,12 @@ static int wled_configure(struct wled *wled) > > "qcom,enabled-strings", > > sizeof(u32)); > > if (string_len > 0) { > > + if (string_len > wled->max_string_count) { > > + dev_err(dev, "Cannot have more than %d strings\n", > > + wled->max_string_count); > > + return -EINVAL; > > + } > > + > > rc = of_property_read_u32_array(dev->of_node, > > "qcom,enabled-strings", > > wled->cfg.enabled_strings, > > @@ -1537,6 +1543,14 @@ static int wled_configure(struct wled *wled) > > return -EINVAL; > > } > > > > + for (i = 0; i < string_len; ++i) { > > + if (wled->cfg.enabled_strings[i] >= wled->max_string_count) { > > + dev_err(dev, "qcom,enabled-strings index %d at %d is out of bounds\n", > > + wled->cfg.enabled_strings[i], i); > > + return -EINVAL; > > + } > > + } > > + > > cfg->num_strings = string_len; > > } > > > > -- > > 2.33.0 > >
On Tue, Oct 05, 2021 at 12:03:50PM +0200, Marijn Suijten wrote: > On 2021-10-05 10:14:52, Daniel Thompson wrote: > > On Mon, Oct 04, 2021 at 09:27:35PM +0200, Marijn Suijten wrote: > > > The strings passed in DT may possibly cause out-of-bounds register > > > accesses and should be validated before use. > > > > > > Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for WLED3") > > > > The first half of this patch actually fixes patch 1 from this patch set. > > It would be better to move that code there. > > It only helps guarding against a maximum of 3 leds for WLED3, while > using string_len instead of an unintentional sizeof(u32) (resulting in > a fixed size of 4) is a different issue requiring a separate patch to > fix. > > Would it help to reorder this patch before 1/10, and mention in patch > 1/10 (then 2/10) that, besides properly using string_len instead of > hardcoded 4 (which causes wrong reads from DT on top of this), it relies > on the previous patch to prevent against an array longer than 3 for > WLED3? Reordering is OK for me. Daniel. > > > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> > > > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org> > > > --- > > > drivers/video/backlight/qcom-wled.c | 14 ++++++++++++++ > > > 1 file changed, 14 insertions(+) > > > > > > diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c > > > index 29910e603c42..27e8949c7922 100644 > > > --- a/drivers/video/backlight/qcom-wled.c > > > +++ b/drivers/video/backlight/qcom-wled.c > > > @@ -1526,6 +1526,12 @@ static int wled_configure(struct wled *wled) > > > "qcom,enabled-strings", > > > sizeof(u32)); > > > if (string_len > 0) { > > > + if (string_len > wled->max_string_count) { > > > + dev_err(dev, "Cannot have more than %d strings\n", > > > + wled->max_string_count); > > > + return -EINVAL; > > > + } > > > + > > > rc = of_property_read_u32_array(dev->of_node, > > > "qcom,enabled-strings", > > > wled->cfg.enabled_strings, > > > @@ -1537,6 +1543,14 @@ static int wled_configure(struct wled *wled) > > > return -EINVAL; > > > } > > > > > > + for (i = 0; i < string_len; ++i) { > > > + if (wled->cfg.enabled_strings[i] >= wled->max_string_count) { > > > + dev_err(dev, "qcom,enabled-strings index %d at %d is out of bounds\n", > > > + wled->cfg.enabled_strings[i], i); > > > + return -EINVAL; > > > + } > > > + } > > > + > > > cfg->num_strings = string_len; > > > } > > > > > > -- > > > 2.33.0 > > >
diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c index 29910e603c42..27e8949c7922 100644 --- a/drivers/video/backlight/qcom-wled.c +++ b/drivers/video/backlight/qcom-wled.c @@ -1526,6 +1526,12 @@ static int wled_configure(struct wled *wled) "qcom,enabled-strings", sizeof(u32)); if (string_len > 0) { + if (string_len > wled->max_string_count) { + dev_err(dev, "Cannot have more than %d strings\n", + wled->max_string_count); + return -EINVAL; + } + rc = of_property_read_u32_array(dev->of_node, "qcom,enabled-strings", wled->cfg.enabled_strings, @@ -1537,6 +1543,14 @@ static int wled_configure(struct wled *wled) return -EINVAL; } + for (i = 0; i < string_len; ++i) { + if (wled->cfg.enabled_strings[i] >= wled->max_string_count) { + dev_err(dev, "qcom,enabled-strings index %d at %d is out of bounds\n", + wled->cfg.enabled_strings[i], i); + return -EINVAL; + } + } + cfg->num_strings = string_len; }
The strings passed in DT may possibly cause out-of-bounds register accesses and should be validated before use. Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for WLED3") Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org> --- drivers/video/backlight/qcom-wled.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)