From: Denis Kenzior <denkenz@gmail.com>

The qrtr_ctrl_pkt structure is currently accessed without checking
if the received payload is large enough to hold the structure's fields.
Add a check to ensure the payload length is sufficient.

Signed-off-by: Denis Kenzior <denkenz@gmail.com>
Reviewed-by: Marcel Holtmann <marcel@holtmann.org>
Reviewed-by: Andy Gross <agross@kernel.org>
Signed-off-by: Mihai Moldovan <ionic@ionic.de>
Fixes: 0c2204a4ad71 ("net: qrtr: Migrate nameservice to kernel from userspace")

---
v5:
  - no changes
  - Link to v4: https://msgid.link/456d8dff226c88657c79f1dbadf0dcaba8b905ae.1753720934.git.ionic@ionic.de

v4:
  - no changes
  - Link to v3: https://msgid.link/a3bc13d1496404e96723a427086271107016bdd6.1753312999.git.ionic@ionic.de

v3:
  - add Fixes: tag
  - rebase against current master
  - Link to v2: https://msgid.link/866f309e9739d770dce7e8c648b562d37db1d8b5.1752947108.git.ionic@ionic.de

v2:
  - rebase against current master
  - use correct size of packet structure as per review comment
  - Link to v1: https://msgid.link/20241018181842.1368394-2-denkenz@gmail.com
---
 net/qrtr/ns.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c
index 3de9350cbf30..2bcfe539dc3e 100644
--- a/net/qrtr/ns.c
+++ b/net/qrtr/ns.c
@@ -619,6 +619,9 @@ static void qrtr_ns_worker(struct work_struct *work)
 			break;
 		}
 
+		if ((size_t)msglen < sizeof(*pkt))
+			break;
+
 		pkt = recv_buf;
 		cmd = le32_to_cpu(pkt->cmd);
 		if (cmd < ARRAY_SIZE(qrtr_ctrl_pkt_strings) &&
-- 
2.50.0

On Tue, 12 Aug 2025 03:35:27 +0200 Mihai Moldovan wrote:
> The qrtr_ctrl_pkt structure is currently accessed without checking
> if the received payload is large enough to hold the structure's fields.
> Add a check to ensure the payload length is sufficient.
> 
> Signed-off-by: Denis Kenzior <denkenz@gmail.com>
> Reviewed-by: Marcel Holtmann <marcel@holtmann.org>
> Reviewed-by: Andy Gross <agross@kernel.org>
> Signed-off-by: Mihai Moldovan <ionic@ionic.de>
> Fixes: 0c2204a4ad71 ("net: qrtr: Migrate nameservice to kernel from userspace")

If this is a fix it has to go to net, then once it reaches Linus's tree
the dependent patches should be reposted for net-next.

> diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c
> index 3de9350cbf30..2bcfe539dc3e 100644
> --- a/net/qrtr/ns.c
> +++ b/net/qrtr/ns.c
> @@ -619,6 +619,9 @@ static void qrtr_ns_worker(struct work_struct *work)
>  			break;
>  		}
>  
> +		if ((size_t)msglen < sizeof(*pkt))
> +			break;

why not continue?

* On 8/15/25 20:09, Jakub Kicinski wrote:
> If this is a fix it has to go to net, then once it reaches Linus's tree
> the dependent patches should be reposted for net-next.

Thanks.

Will split out the two commits and resend the resend of the series later on 
after the fixes have been merged.

Might take me a few weeks, I'm currently very limited on time and internet access.


>> diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c
>> index 3de9350cbf30..2bcfe539dc3e 100644
>> --- a/net/qrtr/ns.c
>> +++ b/net/qrtr/ns.c
>> @@ -619,6 +619,9 @@ static void qrtr_ns_worker(struct work_struct *work)
>>   			break;
>>   		}
>>   
>> +		if ((size_t)msglen < sizeof(*pkt))
>> +			break;
> 
> why not continue?

I don't really know and am not familiar with the QRTR protocol, but here's my 
best guess:

Since we're using non-blocking I/O, it doesn't seem to make sense to continue, 
because the next receive call would just break out anyway once it returns no 
data at all. Notice that we're also breaking out for -EAGAIN.

Also, if we somehow got a short read, and we're currently dropping the buffer we 
just read, any additional data after a subsequent receive would be garbage to us 
anyway. We'd probably have to keep the old buffer content around and concatenate 
it with data returned from a new receive call.



Mihai

On Fri, 22 Aug 2025 21:08:47 +0200 Mihai Moldovan wrote:
> >> diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c
> >> index 3de9350cbf30..2bcfe539dc3e 100644
> >> --- a/net/qrtr/ns.c
> >> +++ b/net/qrtr/ns.c
> >> @@ -619,6 +619,9 @@ static void qrtr_ns_worker(struct work_struct *work)
> >>   			break;
> >>   		}
> >>   
> >> +		if ((size_t)msglen < sizeof(*pkt))
> >> +			break;  
> > 
> > why not continue?  
> 
> I don't really know and am not familiar with the QRTR protocol, but here's my 
> best guess:
> 
> Since we're using non-blocking I/O, it doesn't seem to make sense to continue, 
> because the next receive call would just break out anyway once it returns no 
> data at all. Notice that we're also breaking out for -EAGAIN.
> 
> Also, if we somehow got a short read, and we're currently dropping the buffer we 
> just read, any additional data after a subsequent receive would be garbage to us 
> anyway. We'd probably have to keep the old buffer content around and concatenate 
> it with data returned from a new receive call.

Okay, I don't know this proto and driver either. Just reading the
existing code it seemed like it's only breaks if the socket itself
has an error. If the command is not recognized or garbage the loop
will at most print an error and carry on looping..