Fix three packet-parsing and locking bugs in Phonet Pipe End Point
(`net/phonet/pep.c`):

1. In `pep_ctrlreq_error()`, `oph = pnp_hdr(oskb)` is dereferenced at
   `oph->pep_type` (`oph->data[0]`, offset 4 from `pnp_hdr(oskb)`)
   before verifying that `oph->data[0]` lies within the linear `oskb`
   data area. When called from `pep_do_rcv()`
   (`PN_PIPE_INVALID_HANDLE`), `oskb->data` still points to `oph` and
   only `pskb_may_pull(skb, sizeof(*hdr))` (4 bytes) was checked,
   causing `oph->pep_type` to read 1 byte past `oskb->tail` and echo it
   back to the peer in `PNS_PEP_CTRL_RESP`. Ensure `pskb_may_pull()`
   covers `(oph->data + 1) - oskb->data` bytes (and `sizeof(*hdr) + 1`
   in `pipe_do_rcv()`).
2. In `pep_sock_accept()`, `n_sb = hdr->data[3]` is read from the
   `PNS_PIPE_CONNECT_REQ` header, but `__skb_pull(skb, sizeof(*hdr) +
   4)` is omitted before the `pep_get_sb()` loop, causing `pep_get_sb()`
   to parse the 8-byte `pnpipehdr` and connect-request header as
   sub-blocks, and `PN_PIPE_SB_ALIGNED_DATA` reads `data[0]` without
   checking `len >= 1`.
3. In `pep_setsockopt()` (`PNPIPE_ENCAP`), `release_sock(sk)` is dropped
   around `gprs_attach(sk)`, and `pn->ifindex` is assigned afterwards
   without re-acquiring `lock_sock(sk)` or checking `SOCK_DEAD` /
   concurrent attachment.

Fixes: 9641458d3ec4 ("Phonet: Pipe End Point for Phonet Pipes protocol")
Assisted-by: LLM
Signed-off-by: Hui Peng <benquike@gmail.com>

---
 net/phonet/pep.c | 35 +++++++++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 8 deletions(-)

diff --git a/net/phonet/pep.c b/net/phonet/pep.c
index bd1cdd00edfa..5511770bb21c 100644
--- a/net/phonet/pep.c
+++ b/net/phonet/pep.c
@@ -192,15 +192,21 @@ static int pep_reject_conn(struct sock *sk, struct sk_buff *skb, u8 code,
 static int pep_ctrlreq_error(struct sock *sk, struct sk_buff *oskb, u8 code,
 				gfp_t priority)
 {
-	const struct pnpipehdr *oph = pnp_hdr(oskb);
+	const struct pnpipehdr *oph;
 	struct sk_buff *skb;
 	struct pnpipehdr *ph;
 	struct sockaddr_pn dst;
-	u8 data[4] = {
-		oph->pep_type, /* PEP type */
-		code, /* error code, at an unusual offset */
-		PAD, PAD,
-	};
+	u8 data[4];
+
+	oph = pnp_hdr(oskb);
+	if (!pskb_may_pull(oskb, (unsigned int)((oph->data + 1) - oskb->data)))
+		return -EINVAL;
+
+	oph = pnp_hdr(oskb);
+	data[0] = oph->pep_type; /* PEP type */
+	data[1] = code; /* error code, at an unusual offset */
+	data[2] = PAD;
+	data[3] = PAD;
 
 	skb = pep_alloc_skb(sk, data, 4, priority);
 	if (!skb)
@@ -377,6 +383,8 @@ static int pipe_do_rcv(struct sock *sk, struct sk_buff *skb)
 		break;
 
 	case PNS_PEP_CTRL_REQ:
+		if (!pskb_may_pull(skb, sizeof(*hdr) + 1))
+			break;
 		if (skb_queue_len(&pn->ctrlreq_queue) >= PNPIPE_CTRLREQ_MAX) {
 			sk_drops_inc(sk);
 			break;
@@ -823,6 +831,7 @@ static struct sock *pep_sock_accept(struct sock *sk,
 
 	/* Parse sub-blocks (options) */
 	n_sb = hdr->data[3];
+	__skb_pull(skb, sizeof(*hdr) + 4);
 	while (n_sb > 0) {
 		u8 type, buf[1], len = sizeof(buf);
 		const u8 *data = pep_get_sb(skb, &type, &len, buf);
@@ -836,6 +845,8 @@ static struct sock *pep_sock_accept(struct sock *sk,
 			peer_type = (peer_type & 0xff00) | data[0];
 			break;
 		case PN_PIPE_SB_ALIGNED_DATA:
+			if (len < 1)
+				goto drop;
 			aligned = data[0] != 0;
 			break;
 		}
@@ -1048,8 +1059,16 @@ static int pep_setsockopt(struct sock *sk, int level, int optname,
 			release_sock(sk);
 			err = gprs_attach(sk);
 			if (err > 0) {
-				pn->ifindex = err;
-				err = 0;
+				lock_sock(sk);
+				if (sock_flag(sk, SOCK_DEAD) || pn->ifindex) {
+					release_sock(sk);
+					gprs_detach(sk);
+					err = -EINVAL;
+				} else {
+					pn->ifindex = err;
+					err = 0;
+					release_sock(sk);
+				}
 			}
 		} else {
 			pn->ifindex = 0;

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 0 (5)

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).

- [Medium] The patch bundles three logically independent changes into one
  commit: (a) the 1-byte OOB-read hardening in pep_ctrlreq_error() /…
- [Medium] The changelog misidentifies the out-of-bounds read it fixes.

Pre-existing issues:
- [Low] In-tree sender and receiver of PNS_PEP_CONNECT_REQ disagree on
  the 4-byte payload layout that follows the pnpipehdr.
- [Low] The file-level locking-policy comment for struct pep_sock in
  net/phonet/pep.c (lines 33-37: 'pep_sock locking: - sk_state, hlist:…
- [Low] Documentation/networking/phonet.rst describes PNPIPE_HANDLE as 'a
  read-only integer value', but pep_setsockopt() accepts it: when…
- [Low] Documentation/networking/phonet.rst states 'The pipe protocol
  provides two socket options at the SOL_PNPIPE level' and then…
- [Low] The new writer-side locking in pep_setsockopt() does not remove
  the data race on pn->ifindex with getsockopt readers:…

--

phonet: pep: fix out-of-bounds reads in pep_ctrlreq_error() and pep_sock_accept()

This adds pskb_may_pull() coverage for the control-request path in
pep_ctrlreq_error() and pipe_do_rcv(), adds the missing __skb_pull() plus a
length check before the sub-block loop in pep_sock_accept(), and re-acquires
the socket lock around the pn->ifindex assignment in the PNPIPE_ENCAP case of
pep_setsockopt().  No Link: tags are present in the commit header.

A few questions below.

> diff --git a/net/phonet/pep.c b/net/phonet/pep.c
> index bd1cdd00edfa..5511770bb21c 100644
> --- a/net/phonet/pep.c
> +++ b/net/phonet/pep.c
> @@ -192,15 +192,21 @@ static int pep_reject_conn(struct sock *sk, struct sk_buff *skb, u8 code,
>  static int pep_ctrlreq_error(struct sock *sk, struct sk_buff *oskb, u8 code,
>  				gfp_t priority)
>  {
> -	const struct pnpipehdr *oph = pnp_hdr(oskb);
> +	const struct pnpipehdr *oph;
>  	struct sk_buff *skb;
>  	struct pnpipehdr *ph;
>  	struct sockaddr_pn dst;
> -	u8 data[4] = {
> -		oph->pep_type, /* PEP type */
> -		code, /* error code, at an unusual offset */
> -		PAD, PAD,
> -	};
> +	u8 data[4];
> +
> +	oph = pnp_hdr(oskb);
> +	if (!pskb_may_pull(oskb, (unsigned int)((oph->data + 1) - oskb->data)))
> +		return -EINVAL;
> +
> +	oph = pnp_hdr(oskb);
> +	data[0] = oph->pep_type; /* PEP type */
> +	data[1] = code; /* error code, at an unusual offset */
> +	data[2] = PAD;
> +	data[3] = PAD;

[Severity: Medium]
The new check looks correct, but does the changelog name the wrong read?  It
says:

   1. In `pep_ctrlreq_error()`, `oph = pnp_hdr(oskb)` is dereferenced at
      `oph->pep_type` (`oph->data[0]`, offset 4 from `pnp_hdr(oskb)`)

struct pnpipehdr in include/net/phonet/pep.h is made only of u8 members:

struct pnpipehdr {
	u8			utid;
	u8			message_id;
	u8			pipe_handle;
	union {
		u8		state_after_connect;
		...
		u8		pep_type;
		u8		data0;
	};
	u8			data[];
};

So pep_type is at offset 3, data[] begins at offset 4 and sizeof(*hdr) is 4.
The caller pep_do_rcv() already guarantees those four bytes:

	if (!pskb_may_pull(skb, sizeof(*hdr)))
		goto drop;

which means oph->pep_type was in bounds.  The read that was not covered is
the later one in the same function:

	ph->data0 = oph->data[0]; /* CTRL id */

That touches offset 4 and needs five linear bytes, which is exactly what the
new pskb_may_pull(oskb, (oph->data + 1) - oskb->data) provides.  Could the
changelog point at oph->data[0] instead of oph->pep_type, so a reader
checking the named expression does not conclude there is nothing to fix?

>  
>  	skb = pep_alloc_skb(sk, data, 4, priority);
>  	if (!skb)

[ ... ]

> @@ -823,6 +831,7 @@ static struct sock *pep_sock_accept(struct sock *sk,
>  
>  	/* Parse sub-blocks (options) */
>  	n_sb = hdr->data[3];
> +	__skb_pull(skb, sizeof(*hdr) + 4);

[Severity: Low]
This isn't a bug introduced by this patch, but the new __skb_pull() fixes the
sub-block offset according to the receiver's reading of the message, and the
in-tree sender disagrees with it.  pep_sock_connect() builds the connect
request payload as:

	u8 data[4] = { 0 /* sub-blocks */, PAD, PAD, PAD };

so the sub-block count goes at data[0] (header offset 4), while
pep_sock_accept() decodes peer_type from hdr->other_pep_type (also data[0])
and the count from hdr->data[3] (offset 7).

The connect-response pair is self-consistent: pep_accept_conn() emits
PAD, PAD, PAD, 2 and pep_connresp_rcv() reads hdr->data[3].

Nothing misbehaves today since the kernel always announces zero sub-blocks
and PAD is zero.  Is the request sender's layout worth a separate fix?

>  	while (n_sb > 0) {
>  		u8 type, buf[1], len = sizeof(buf);
>  		const u8 *data = pep_get_sb(skb, &type, &len, buf);

[ ... ]

> @@ -1048,8 +1059,16 @@ static int pep_setsockopt(struct sock *sk, int level, int optname,
>  			release_sock(sk);
>  			err = gprs_attach(sk);
>  			if (err > 0) {
> -				pn->ifindex = err;
> -				err = 0;
> +				lock_sock(sk);
> +				if (sock_flag(sk, SOCK_DEAD) || pn->ifindex) {
> +					release_sock(sk);
> +					gprs_detach(sk);
> +					err = -EINVAL;
> +				} else {

[Severity: Medium]
Item 3 of the changelog states:

   3. In `pep_setsockopt()` (`PNPIPE_ENCAP`), `release_sock(sk)` is dropped
      around `gprs_attach(sk)`, and `pn->ifindex` is assigned afterwards
      without re-acquiring `lock_sock(sk)`

The release_sock(sk) ahead of gprs_attach() is already present in the
baseline and this diff keeps it unchanged; only the lock_sock()
re-acquisition and the SOCK_DEAD / pn->ifindex recheck are new.  Could that
sentence be reworded?

Separately, this hunk changes what userspace sees: a
setsockopt(SOL_PNPIPE, PNPIPE_ENCAP, PNPIPE_ENCAP_IP) that previously
returned 0 can now return -EINVAL and, via gprs_detach(), unregister the
gprs netdev that gprs_attach() just created and drop the socket reference.

That is unrelated to the parsing changes in pep_ctrlreq_error() /
pipe_do_rcv() and in pep_sock_accept(), and all three share one Fixes: tag.
Would splitting this into one logical change per patch make the parsing
fixes easier to pick up for stable?

> +					pn->ifindex = err;
> +					err = 0;
> +					release_sock(sk);
> +				}

[Severity: Low]
This is a pre-existing issue and not introduced by this patch, but the store
to pn->ifindex is now marked while the readers still are not.
do_pep_getsockopt() reads it for two options:

	case PNPIPE_ENCAP:
		val = pn->ifindex ? PNPIPE_ENCAP_IP : PNPIPE_ENCAP_NONE;
		break;

	case PNPIPE_IFINDEX:
		val = pn->ifindex;
		break;

and neither pep_getsockopt() nor sock_common_getsockopt() takes the socket
lock on the way in, so KCSAN can still flag the pair.  Would converting
these accesses to READ_ONCE()/WRITE_ONCE() while here be reasonable?

>  			}
>  		} else {
>  			pn->ifindex = 0;

Two further notes that need no diff context:

[Severity: Low]
This is a pre-existing gap rather than something this patch introduced, but
the pep_sock locking policy comment near the top of net/phonet/pep.c reads:

 * pep_sock locking:
 *  - sk_state, hlist: sock lock needed
 *  - listener: read only
 *  - pipe_handle: read only

ifindex is not listed there, even though the fix in pep_setsockopt() relies
on it being read and written under lock_sock().  Could a line for ifindex be
added while touching this?

[Severity: Low]
This is pre-existing and untouched by the patch, but
Documentation/networking/phonet.rst describes PNPIPE_HANDLE as "is a
read-only integer value", while pep_setsockopt() accepts it:

	case PNPIPE_HANDLE:
		if ((sk->sk_state == TCP_CLOSE) &&
			(val >= 0) && (val < PN_PIPE_INVALID_HANDLE))
			pn->pipe_handle = val;

Should the documentation say it is writable while the socket is closed?

[Severity: Low]
Also pre-existing and not caused by this patch: the same file says "The pipe
protocol provides two socket options at the SOL_PNPIPE level" and then
documents three of them (PNPIPE_ENCAP, PNPIPE_IFINDEX, PNPIPE_HANDLE).
PNPIPE_INITSTATE is exported by include/uapi/linux/phonet.h and handled by
both pep_setsockopt() and do_pep_getsockopt(), but is not documented.  Could
the count and the missing option be fixed up?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260919212852.3240704-1-benquike%40gmail.com