[PATCH net-next] net: ncsi: check for netlink-driven responses before requiring a handler

[PATCH net-next] net: ncsi: check for netlink-driven responses before requiring a handler

[Jeremy Kerr]

Currently, the NCSI response path will look up an opcode-specific
handler for all incoming response messages. However, we may be receiving
a response from a netlink-generated request, which may not have a
corresponding in-kernel handler for that request opcode. In that case,
we'll drop the response because we didn't find a opcode-specific
handler.

Perform the lookup for the pending request (and hence for
NETLINK_DRIVEN) before requiring an in-kernel handler, and defer the
requirement for a corresponding kernel request until we know it's a
kernel-driven command.

Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
---
 net/ncsi/ncsi-rsp.c | 61 ++++++++++++++++++++++++-----------------------------
 1 file changed, 27 insertions(+), 34 deletions(-)

diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
index e28be33bdf2c487c0fbfe3a1b4de6f52c8f923cc..882767c138895eba5bd7e413b7edb6480d5807bf 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c
@@ -1205,12 +1205,6 @@ int ncsi_rcv_rsp(struct sk_buff *skb, struct net_device *dev,
 		}
 	}
 
-	if (!nrh) {
-		netdev_err(nd->dev, "Received unrecognized packet (0x%x)\n",
-			   hdr->type);
-		return -ENOENT;
-	}
-
 	/* Associate with the request */
 	spin_lock_irqsave(&ndp->lock, flags);
 	nr = &ndp->requests[hdr->id];
@@ -1228,43 +1222,42 @@ int ncsi_rcv_rsp(struct sk_buff *skb, struct net_device *dev,
 
 	/* Validate the packet */
 	spin_unlock_irqrestore(&ndp->lock, flags);
-	payload = nrh->payload;
+	payload = nrh ? nrh->payload : -1;
 	if (payload < 0)
 		payload = ntohs(hdr->length);
+
 	ret = ncsi_validate_rsp_pkt(nr, payload);
-	if (ret) {
+	if (ret)
 		netdev_warn(ndp->ndev.dev,
 			    "NCSI: 'bad' packet ignored for type 0x%x\n",
 			    hdr->type);
 
-		if (nr->flags == NCSI_REQ_FLAG_NETLINK_DRIVEN) {
-			if (ret == -EPERM)
-				goto out_netlink;
-			else
-				ncsi_send_netlink_err(ndp->ndev.dev,
-						      nr->snd_seq,
-						      nr->snd_portid,
-						      &nr->nlhdr,
-						      ret);
-		}
-		goto out;
-	}
-
-	/* Process the packet */
-	ret = nrh->handler(nr);
-	if (ret)
-		netdev_err(ndp->ndev.dev,
-			   "NCSI: Handler for packet type 0x%x returned %d\n",
-			   hdr->type, ret);
-
-out_netlink:
 	if (nr->flags == NCSI_REQ_FLAG_NETLINK_DRIVEN) {
-		ret = ncsi_rsp_handler_netlink(nr);
-		if (ret) {
-			netdev_err(ndp->ndev.dev,
-				   "NCSI: Netlink handler for packet type 0x%x returned %d\n",
-				   hdr->type, ret);
+		/* netlink driven: forward response to netlink socket */
+		if (!ret || ret == -EPERM)
+			ncsi_rsp_handler_netlink(nr);
+		else
+			ncsi_send_netlink_err(ndp->ndev.dev,
+					      nr->snd_seq,
+					      nr->snd_portid,
+					      &nr->nlhdr,
+					      ret);
+	} else if (nrh) {
+		/* not netlink driven: process the packet in the kernel. We
+		 * need to have a handler for this
+		 */
+		if (!ret) {
+			ret = nrh->handler(nr);
+			if (ret)
+				netdev_err(ndp->ndev.dev,
+					   "NCSI: Handler for packet type 0x%x returned %d\n",
+					   hdr->type, ret);
 		}
+
+	} else {
+		netdev_err(nd->dev, "Received unrecognized packet (0x%x)\n",
+			   hdr->type);
+		ret = -ENOENT;
 	}
 
 out:

---
base-commit: 03fc07a24735e0be8646563913abf5f5cb71ad19
change-id: 20241028-ncsi-arb-opcode-a346ddb88862

Best regards,
-- 
Jeremy Kerr <jk@codeconstruct.com.au>

Re: [PATCH net-next] net: ncsi: check for netlink-driven responses before requiring a handler

[Simon Horman]

On Mon, Oct 28, 2024 at 03:08:34PM +0800, Jeremy Kerr wrote:
> Currently, the NCSI response path will look up an opcode-specific
> handler for all incoming response messages. However, we may be receiving
> a response from a netlink-generated request, which may not have a
> corresponding in-kernel handler for that request opcode. In that case,
> we'll drop the response because we didn't find a opcode-specific
> handler.
> 
> Perform the lookup for the pending request (and hence for
> NETLINK_DRIVEN) before requiring an in-kernel handler, and defer the
> requirement for a corresponding kernel request until we know it's a
> kernel-driven command.
> 
> Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>

Hi Jeremy,

Not strictly related to this patch, but I do wonder if the log messages
should be rate limited, which doesn't seem to be the case at this time.

Regardless, this looks good to me.

Reviewed-by: Simon Horman <horms@kernel.org>

...

Re: [PATCH net-next] net: ncsi: check for netlink-driven responses before requiring a handler

[Jakub Kicinski]

On Mon, 28 Oct 2024 15:08:34 +0800 Jeremy Kerr wrote:
> Subject: [PATCH net-next] net: ncsi: check for netlink-driven responses before requiring a handler

> Currently, the NCSI response path will look up an opcode-specific
> handler for all incoming response messages. However, we may be receiving
> a response from a netlink-generated request, which may not have a
> corresponding in-kernel handler for that request opcode. In that case,
> we'll drop the response because we didn't find a opcode-specific
> handler.

This makes it sound like the code is written this way unintentionally,
which I doubt. A better description of the patch would be "allow
userspace to issue commands unknown to the kernel". And then it'd be
great to get some examples of commands you'd like to issue..

> Perform the lookup for the pending request (and hence for
> NETLINK_DRIVEN) before requiring an in-kernel handler, and defer the
> requirement for a corresponding kernel request until we know it's a
> kernel-driven command.

As for the code - delaying handling ret != 0 makes me worried that
someone will insert code in between and clobber it. Can you split
the handling so that all the ret != 0 (or EPERM for netlink)
are still handled in the if (ret) {} ?
-- 
pw-bot: cr

Re: [PATCH net-next] net: ncsi: check for netlink-driven responses before requiring a handler

[Jeremy Kerr]

Hi Jakub,

> > Currently, the NCSI response path will look up an opcode-specific
> > handler for all incoming response messages. However, we may be
> > receiving
> > a response from a netlink-generated request, which may not have a
> > corresponding in-kernel handler for that request opcode. In that
> > case,
> > we'll drop the response because we didn't find a opcode-specific
> > handler.
> 
> This makes it sound like the code is written this way
> unintentionally, which I doubt.

This seems like an oversight in the response path, failing on a missing
in-kernel handler even if we don't later use it. If we were
intentionally enforcing only known commands, then we'd also be checking
on the request side.

But yes, I can certainly word this in terms of what this change now
enables, and provide examples.

> > Perform the lookup for the pending request (and hence for
> > NETLINK_DRIVEN) before requiring an in-kernel handler, and defer
> > the
> > requirement for a corresponding kernel request until we know it's a
> > kernel-driven command.
> 
> As for the code - delaying handling ret != 0 makes me worried that
> someone will insert code in between and clobber it. Can you split
> the handling so that all the ret != 0 (or EPERM for netlink)
> are still handled in the if (ret) {} ?

Can do! The -EPERM case can probably be simplified a little too, as it
just indicates we didn't get a success response from the remote NCSI
device.

Will work on a v2 soon.

Cheers,


Jeremy