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

[Suraj Sonawane <surajsonawane0215@gmail.com>]

On 27/10/24 19:27, Peter Seiderer wrote:
> 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)
> 
Hello Peter,

Thank you for the feedback. I understand your point about the existing 
checks ensuring operation->response is allocated before line 852. It 
seems this might have been a false positive from the static analysis tool.

Once again, thank you for your time and consideration.

Best,
Suraj