On Tue, Oct 05, 2021 at 01:44:35PM +0200, Marijn Suijten wrote:
On 2021-10-05 11:53:12, Daniel Thompson wrote:
On Tue, Oct 05, 2021 at 11:38:43AM +0100, Daniel Thompson wrote:
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 =Oops... looks like I missed the cfg->max_string_count here.
- else
/* Convert from enumerated to numeric form */cfg->num_strings = wled3_num_strings_values_fn(cfg->num_strings);PS the alternative option is not to treat num-strings as an enumerated value at all and just read it directly without using wled_values()...
I much prefer doing that instead of trying to wrangle enumeration parsing around integer values that are supposed to be used as-is. After all this variable is already named to set the `+ 1` override currently, and `qcom,enabled_strings` has "custom" handling as well. I'll extend the validation to ensure num_strings>=1 too.
Great.
In addition, and this needs some investigation on the dt-bindings side too, it might be beneficial to make both properties mutually exclusive. When specifying qcom,enabled_strings it makes little sense to also provide qcom,num_strings and we want the former to take precedence.
If we are designing a "fix" for that then my view is that if both are passed then num-strings should take precedence because it is an explicit statement about the number of strings where enabled_strings is implicit. In other words, if num-strings <= len(enabled_strings) then we should do what we are told, otherwise report error.
At that point one might ask why qcom,num_strings remains at all when DT can use qcom,enabled_strings instead. We will supposedly have to keep backwards compatibility with DTs in mind so none of this can be removed or made mutually exclusive from a driver standpoint, that all has to be done in dt-bindings yaml to be enforced on checked-in DTs.
So... perhaps I made a make offering a Reviewed-by: to a patch that allows len(enabled-strings) to have precedence. If anything currently uses enabled-strings then it *will* be 4 cells long and is relying on num-strings to ensure the right things happens ;-) .
We'd like that case to keep working so we must allow num-strings to have precedence. In other words, when you add the new code, please put it at the end of the function!
Daniel.
- Marijn