drivers/greybus/operation.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
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.1On 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)
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
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)
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
© 2016 - 2026 Red Hat, Inc.