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)
_______________________________________________
greybus-dev mailing list -- greybus-dev@xxxxxxxxxxxxxxxx
To unsubscribe send an email to greybus-dev-leave@xxxxxxxxxxxxxxxx
|