[PATCH] spi: offload: check ops and match pointers before use

[PATCH] spi: offload: check ops and match pointers before use

[Andres Urian Florez]

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(-)

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;
 	}
-- 
2.43.0

Re: [PATCH] spi: offload: check ops and match pointers before use

[David Lechner]

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.

Re: [PATCH] spi: offload: check ops and match pointers before use

[Andres Urian]

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.

Re: [PATCH] spi: offload: check ops and match pointers before use

[David Lechner]

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.