Re: [PATCH] greybus: Fix null pointer dereference in gb_operation_response_send()

[Peter Seiderer <ps.report@gmx.net>]

Hello Suraj,

On Sun, 27 Oct 2024 13:23:04 +0530, Suraj Sonawane <surajsonawane0215@gmail.com> wrote:

> Fix an issue detected by the Smatch static tool:
> drivers/greybus/operation.c:852 gb_operation_response_send() error:
> we previously assumed 'operation->response' could be null (see line 829)
>
> The issue occurs because 'operation->response' may be null if the
> response allocation fails at line 829. However, the code tries to
> access 'operation->response->header' at line 852 without checking if
> it was successfully allocated. This can cause a crash if 'response'
> is null.
>
> To fix this, add a check to ensure 'operation->response' is not null
> before accessing its header. If the response is null, log an error
> message and return -ENOMEM to stop further processing, preventing
> any crashes or undefined behavior.

False warning (?) as the complete code is as follows:

 823 static int gb_operation_response_send(struct gb_operation *operation,
 824                                       int errno)
 825 {
 826         struct gb_connection *connection = operation->connection;
 827         int ret;
 828
 829         if (!operation->response &&
 830             !gb_operation_is_unidirectional(operation)) {
 831                 if (!gb_operation_response_alloc(operation, 0, GFP_KERNEL))
 832                         return -ENOMEM;
 833         }
 834
 835         /* Record the result */
 836         if (!gb_operation_result_set(operation, errno)) {
 837                 dev_err(&connection->hd->dev, "request result already set\n     ");
 838                 return -EIO;    /* Shouldn't happen */
 839         }
 840
 841         /* Sender of request does not care about response. */
 842         if (gb_operation_is_unidirectional(operation))
 843                 return 0;
 844
 845         /* Reference will be dropped when message has been sent. */
 846         gb_operation_get(operation);
 847         ret = gb_operation_get_active(operation);
 848         if (ret)
 849                 goto err_put;
 850
 851         /* Fill in the response header and send it */
 852         operation->response->header->result = gb_operation_errno_map(errno)     ;
 853
 854         ret = gb_message_send(operation->response, GFP_KERNEL);
 855         if (ret)
 856                 goto err_put_active;
 857
 858         return 0;
 859
 860 err_put_active:
 861         gb_operation_put_active(operation);
 862 err_put:
 863         gb_operation_put(operation);
 864
 865         return ret;
 866 }

Lines 829-833 make sure that in case of '!gb_operation_is_unidirectional()'
'operation->response' is allocated (in case of failure early return with
'return -ENOMEM')

Lines 842-843 do an early return in case of 'gb_operation_is_unidirectional()'

So no chance to reach line 852 without 'operation->response' is allocated...

Regards,
Peter

>
> Signed-off-by: Suraj Sonawane <surajsonawane0215@gmail.com>
> ---
>  drivers/greybus/operation.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/greybus/operation.c b/drivers/greybus/operation.c
> index 8459e9bc0..521899fbc 100644
> --- a/drivers/greybus/operation.c
> +++ b/drivers/greybus/operation.c
> @@ -849,7 +849,13 @@ static int gb_operation_response_send(struct gb_operation *operation,
>  		goto err_put;
>
>  	/* Fill in the response header and send it */
> -	operation->response->header->result = gb_operation_errno_map(errno);
> +	if (operation->response) {
> +		operation->response->header->result = gb_operation_errno_map(errno);
> +	} else {
> +		dev_err(&connection->hd->dev, "failed to allocate response\n");
> +		ret = -ENOMEM;
> +		goto err_put_active;
> +	}
>
>  	ret = gb_message_send(operation->response, GFP_KERNEL);
>  	if (ret)