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

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

[Suraj Sonawane]

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.

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)
-- 
2.34.1

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

[Peter Seiderer]

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)

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

[Suraj Sonawane]

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

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

[Alex Elder]

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@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)

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

[Suraj Sonawane]

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@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 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