lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<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

Powered by Openwall GNU/*/Linux Powered by OpenVZ