Re: [PATCH] greybus: Fix null pointer dereference in gb_operation_response_send()
From: Suraj Sonawane
Date: Mon Oct 28 2024 - 08:39:34 EST
On 27/10/24 19:27, Peter Seiderer wrote:
Hello Suraj,
On Sun, 27 Oct 2024 13:23:04 +0530, Suraj Sonawane <surajsonawane0215@xxxxxxxxx> 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@xxxxxxxxx>
---
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