[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <c2e81a50-3716-4ee9-9ada-5d4d9287e564@gmail.com>
Date: Mon, 28 Oct 2024 18:13:30 +0530
From: Suraj Sonawane <surajsonawane0215@...il.com>
To: Alex Elder <elder@...e.org>, johan@...nel.org, elder@...nel.org,
gregkh@...uxfoundation.org
Cc: 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:30, Alex Elder wrote:
> On 10/27/24 2:53 AM, Suraj Sonawane 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)
>
> There is no need for this. This is a case where the code is
> doing something that is too involved for "smatch" to know
> things are OK.
>
> A unidirectional operation includes only a request message, but
> no response message.
>
> There are two cases:
> - Unidirectional
> - There is no response buffer
> - There will be no call to gb_operation_response_alloc(),
> because the operation is unidirectional.
> - The result gets set with the errno value. If there's
> an error (there shouldn't be), -EIO is returned.
> - We return 0 early, because it's a unidirectional operation.
> - Not unidirectional
> - If there is a response, we attempt to allocate one. If that
> fails, we return -ENOMEM early.
> - Otherwise there *is* a response (it was successfully allocated)
> - The result is set
> - It is not unidirectional, so we get a reference to the operation,
> add it to the active list (or skip to the end if not connected)
> - We record the result in the response header. This is the line in
> question, but we know the response pointer is good.
> - We send the response.
> - On error, we drop or references and return the error code.
>
> -Alex
>
>
>
>> 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.
>>
>> 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 Alex,
Thank you for your detailed explanation. I understand now that the
existing code already handles both unidirectional and non-unidirectional
cases properly, ensuring that operation->response is always allocated
when needed. It seems the Smatch tool flagged this as a potential issue
incorrectly.
I appreciate your insights, and I'll make sure to be more cautious about
such false positives from static analysis in the future.
Thanks again for your time.
Best,
Suraj
Powered by blists - more mailing lists