[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <7906ba56-e476-4836-ad33-2504b2f635e1@gmail.com>
Date: Mon, 28 Oct 2024 18:09:18 +0530
From: Suraj Sonawane <surajsonawane0215@...il.com>
To: Peter Seiderer <ps.report@....net>
Cc: johan@...nel.org, elder@...nel.org, gregkh@...uxfoundation.org,
greybus-dev@...ts.linaro.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] greybus: Fix null pointer dereference in
gb_operation_response_send()
On 27/10/24 19:27, Peter Seiderer wrote:
> Hello Suraj,
>
> On Sun, 27 Oct 2024 13:23:04 +0530, Suraj Sonawane <surajsonawane0215@...il.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@...il.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
Powered by blists - more mailing lists