| Message ID | 20250417232319.384094-1-andres.emb.sys@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | spi: offload: check ops and match pointers before use | expand |
On 4/17/25 6:23 PM, Andres Urian Florez wrote: > Before checking if one of the triggers matches, check if 'ops' and 'match' > exist Can you please explain in more detail why this change is needed? For example, show the code path where we could actually have null pointer de-reference here that would be fixed by this change. > > Signed-off-by: Andres Urian Florez <andres.emb.sys@gmail.com> > --- > drivers/spi/spi-offload.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/spi/spi-offload.c b/drivers/spi/spi-offload.c > index 6bad042fe437..fcb226887488 100644 > --- a/drivers/spi/spi-offload.c > +++ b/drivers/spi/spi-offload.c > @@ -173,7 +173,9 @@ static struct spi_offload_trigger > if (trigger->fwnode != args->fwnode) > continue; > > - match = trigger->ops->match(trigger, type, args->args, args->nargs); > + if (trigger->ops && trigger->ops->match) The check for trigger->ops != NULL here should not be necessary. The only place where trigger->ops = NULL is when the trigger is removed from the list and that operation is done with the spi_offload_triggers_lock held. The same lock is also currently held here, so it should not be possible for ops to be set to NULL here. In fact, there is this later in the same function: if (!trigger->ops) return ERR_PTR(-ENODEV); that could be removed (since we have shown that the condition can never be true). > + match = trigger->ops->match(trigger, type, args->args, args->nargs); > + > if (match) > break; > } If trigger->ops->match == NULL then the trigger could never be used because it would never be matched. So instead, I think it would be better to check for that in devm_spi_offload_trigger_register() and fail registration if it is missing. In other words, make match a required callback.
On Fri, Apr 18, 2025 at 9:20 AM David Lechner <dlechner@baylibre.com> wrote: > > On 4/17/25 6:23 PM, Andres Urian Florez wrote: > > Before checking if one of the triggers matches, check if 'ops' and 'match' > > exist > > Can you please explain in more detail why this change is needed? For example, > show the code path where we could actually have null pointer de-reference here > that would be fixed by this change. > > > > > Signed-off-by: Andres Urian Florez <andres.emb.sys@gmail.com> > > --- > > drivers/spi/spi-offload.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/spi/spi-offload.c b/drivers/spi/spi-offload.c > > index 6bad042fe437..fcb226887488 100644 > > --- a/drivers/spi/spi-offload.c > > +++ b/drivers/spi/spi-offload.c > > @@ -173,7 +173,9 @@ static struct spi_offload_trigger > > if (trigger->fwnode != args->fwnode) > > continue; > > > > - match = trigger->ops->match(trigger, type, args->args, args->nargs); > > + if (trigger->ops && trigger->ops->match) > > The check for trigger->ops != NULL here should not be necessary. The only place > where trigger->ops = NULL is when the trigger is removed from the list and that > operation is done with the spi_offload_triggers_lock held. The same lock is also > currently held here, so it should not be possible for ops to be set to NULL here. > > In fact, there is this later in the same function: > > if (!trigger->ops) > return ERR_PTR(-ENODEV); > > that could be removed (since we have shown that the condition can never be true). > > > > + match = trigger->ops->match(trigger, type, args->args, args->nargs); > > + > > if (match) > > break; > > } > > If trigger->ops->match == NULL then the trigger could never be used because it > would never be matched. So instead, I think it would be better to check for > that in devm_spi_offload_trigger_register() and fail registration if it is > missing. In other words, make match a required callback. Hi David. Thanks for your comments! I did not see the full picture and now it is clear to me that it is not required to check the trigger->ops in spi_offload_trigger_get(). Then I will create another patch to remove the trigger->ops check that you mentioned in spi_offload_trigger_get, and make match a required callback in devm_spi_offload_trigger_register() instead.
On 4/18/25 11:37 AM, Andres Urian wrote: > Hi David. > > Thanks for your comments! I did not see the full picture and now it is > clear to me that it is not required to check the trigger->ops in > spi_offload_trigger_get(). > > Then I will create another patch to remove the trigger->ops check that you > mentioned in spi_offload_trigger_get, and make match a required callback > in devm_spi_offload_trigger_register() instead. To save some future review, I would consider these two separate changes and put them in two separate patches.
diff --git a/drivers/spi/spi-offload.c b/drivers/spi/spi-offload.c index 6bad042fe437..fcb226887488 100644 --- a/drivers/spi/spi-offload.c +++ b/drivers/spi/spi-offload.c @@ -173,7 +173,9 @@ static struct spi_offload_trigger if (trigger->fwnode != args->fwnode) continue; - match = trigger->ops->match(trigger, type, args->args, args->nargs); + if (trigger->ops && trigger->ops->match) + match = trigger->ops->match(trigger, type, args->args, args->nargs); + if (match) break; }
Before checking if one of the triggers matches, check if 'ops' and 'match' exist Signed-off-by: Andres Urian Florez <andres.emb.sys@gmail.com> --- drivers/spi/spi-offload.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)