Re: [PATCH net] qrtr: make repeated lookups from one client idempotent

[Simon Horman <horms@kernel.org>]

On Mon, Oct 12, 2026 at 08:40:54AM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · Medium: 0 · Low: 0
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [High] The fix does not close the denial of service its commit message
>   describes.

This does seem to be a legitimate concern. But I wonder what can be done
about it. I expect some sort of non-trivial heuristic is required, but I'm
not sure what one that works would look like. Or even what the bar for
"works" would be.

> 
> --
> 
> qrtr: make repeated lookups from one client idempotent
> 
> This patch adds a check to ctrl_cmd_new_lookup(). If a NEW_LOOKUP
> request has the same node, port, service and instance as an existing
> lookup, that row is reused and no new one is allocated. The goal is to
> stop one unprivileged local socket from filling all 128 global lookup
> rows with duplicates and locking out other observers.
> 
> > diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c
> > index bcb090ee79d49..ae0dc2474e5e9 100644
> > --- a/net/qrtr/ns.c
> > +++ b/net/qrtr/ns.c
> > @@ -547,6 +547,14 @@ static int ctrl_cmd_new_lookup(struct sockaddr_qrtr *from,
> >  	if (from->sq_node != qrtr_ns.local_node)
> >  		return -EINVAL;
> >  
> > +	list_for_each_entry(lookup, &qrtr_ns.lookups, li) {
> > +		if (lookup->sq.sq_node == from->sq_node &&
> > +		    lookup->sq.sq_port == from->sq_port &&
> > +		    lookup->service == service &&
> > +		    lookup->instance == instance)
> > +			goto notify;
> > +	}
> > +
> >  	if (qrtr_ns.lookup_count >= QRTR_NS_MAX_LOOKUPS) {
> >  		pr_err_ratelimited("QRTR client node exceeds max lookup limit!\n");
> >  		return -ENOSPC;
> 
> [Severity: High]
> Does this close the denial of service described in the commit message?
> 
> The new loop only skips allocation when (sq_node, sq_port, service,
> instance) exactly matches an existing row. Any other tuple falls through
> to the unchanged global limit and takes a new row through list_add_tail()
> and qrtr_ns.lookup_count++.
> 
> The service and instance values come straight from the client packet in
> qrtr_ns_worker(), and nothing checks them:
> 
> 		case QRTR_TYPE_NEW_LOOKUP:
> 			ret = ctrl_cmd_new_lookup(&sq,
> 					 le32_to_cpu(pkt->server.service),
> 					 le32_to_cpu(pkt->server.instance));
> 
> Can a single unprivileged AF_QIPCRTR socket still fill all 128 rows?
> It could send NEW_LOOKUP for services 1..128, or for one service with
> instances 1..128. Requests spread across several sockets would also miss
> the exact-tuple check, because each autobound socket gets its own port.
> 
> Once qrtr_ns.lookup_count reaches QRTR_NS_MAX_LOOKUPS, an independent
> observer still gets -ENOSPC. The rows are only freed by DEL_LOOKUP or
> DEL_CLIENT from the owning port, so the attacker can hold them as long as
> the socket stays open.
> 
> The test in the commit message seems to cover only identical duplicates.
> 
> The root cause looks like the single global limit, which has no
> per-owner accounting:
> 
> #define QRTR_NS_MAX_LOOKUPS 128
> 
> Would per-owner accounting fit better than extending the exact-tuple
> dedup? Options include a per-port quota under a global ceiling, or
> per-uid or memcg based accounting.
> 
> Also, the global limit is hit by whichever client sends the next
> request. So the "QRTR client node exceeds max lookup limit!" message
> names the observer that was rejected, not the client that filled the
> table.
> 
> [ ... ]
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008082518.347350-1-tjdqudcks0424%40naver.com

-- 
pw-bot: changes-requested