Re: [PATCH 2/2] net: qrtr: ns: Limit the maximum lookups per socket

[Simon Horman]

On Fri, Mar 27, 2026 at 03:47:13PM +0530, Manivannan Sadhasivam wrote:
> On Fri, Mar 27, 2026 at 10:07:09AM +0000, Simon Horman wrote:
> > On Wed, Mar 25, 2026 at 04:14:15PM +0530, Manivannan Sadhasivam wrote:
> > > Current code does no bound checking on the number of lookups a client can
> > > perform per socket. Though the code restricts the lookups to local clients,
> > > there is still a possibility of a malicious local client sending a flood of
> > > NEW_LOOKUP messages over the same socket.
> > > 
> > > Fix this issue by limiting the maximum number of lookups to 64 per socket.
> > > Note that, limit of 64 is chosen based on the current platform
> > > requirements. If requirement changes in the future, this limit can be
> > > increased.
> > > 
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > Fixes: 0c2204a4ad71 ("net: qrtr: Migrate nameservice to kernel from userspace")
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxxxxxxxx>
> > > ---
> > >  net/qrtr/ns.c | 18 ++++++++++++++++--
> > >  1 file changed, 16 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c
> > > index fb4e8a2d370d..707fde809939 100644
> > > --- a/net/qrtr/ns.c
> > > +++ b/net/qrtr/ns.c
> > > @@ -70,10 +70,11 @@ struct qrtr_node {
> > >  	u32 server_count;
> > >  };
> > >  
> > > -/* Max server limit is chosen based on the current platform requirements. If the
> > > - * requirement changes in the future, this value can be increased.
> > > +/* Max server, lookup limits are chosen based on the current platform requirements.
> > > + * If the requirement changes in the future, these values can be increased.
> > >   */
> > >  #define QRTR_NS_MAX_SERVERS 256
> > > +#define QRTR_NS_MAX_LOOKUPS 64
> > >  
> > >  static struct qrtr_node *node_get(unsigned int node_id)
> > >  {
> > > @@ -545,11 +546,24 @@ static int ctrl_cmd_new_lookup(struct sockaddr_qrtr *from,
> > >  	struct qrtr_node *node;
> > >  	unsigned long node_idx;
> > >  	unsigned long srv_idx;
> > > +	u8 count = 0;
> > >  
> > >  	/* Accept only local observers */
> > >  	if (from->sq_node != qrtr_ns.local_node)
> > >  		return -EINVAL;
> > >  
> > > +	/* Make sure the client performs only maximum allowed lookups */
> > > +	list_for_each_entry(lookup, &qrtr_ns.lookups, li) {
> > > +		if (lookup->sq.sq_node == from->sq_node &&
> > > +		    lookup->sq.sq_port == from->sq_port)
> > > +			count++;
> > 
> > This feels like it could get quite expensive.
> > If many lookups are added, it feels like it may be O(n^2).
> > 
> 
> Lookups are not something that'll happen very often. A client only registers
> for the lookup once per service that it depends on. That shouldn't be too
> much. And then once lookup is registered, it will be used throughout the
> lifetime of the client.
> 
> So there is no overhead associated with this check.

Thanks, and sorry for the delay: I was taking a holiday.

I'm still slightly concerned that this may prove to be a problem.
But we can leave that concern sit in a corner by itself for now.

For this patch feel free to add,

Reviewed-by: Simon Horman <horms@xxxxxxxxxx>

I notice that this series has been marked as changes requested in patchwork.
So, unless it's being handled by some other means that I can't see at this
moment, I think it would be best to repost this series.