MCTP sockaddr padding check/initialisation fixup

PATCH Superseded Eugene Syromiatnikov 3 participants 6 messages Nov 1, 2021 – Nov 3, 2021

Subsystems (3)

Networking Network drivers Management component transport protocol (MCTP)

lore

Share & tools

lore

Eugene Syromiatnikov

author

2021-11-01 17:54 UTC #0 lore raw

cover MCTP sockaddr padding check/initialisation fixup

Hello.

Padding/reserved fields necessitate appropriate checks in order to be usable in the future.

Eugene Syromiatnikov (2):
mctp: handle the struct sockaddr_mctp padding fields
mctp: handle the struct sockaddr_mctp_ext padding field

 net/mctp/af_mctp.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

Signature · 1 line

2.1.4

Eugene Syromiatnikov

author

2021-11-01 17:54 UTC #1 lore raw

PATCH 1/2 mctp: handle the struct sockaddr_mctp padding fields

In order to have the padding fields actually usable in the future, there have to be checks that user space doesn't supply non-zero garbage there. It is also worth setting these padding fields to zero, unless it is known that they have been already zeroed.

Cc: [email protected] # v5.15
Complements: 5a20dd46b8b84593 ("mctp: Be explicit about struct sockaddr_mctp padding")

Signed-off-by Eugene Syromiatnikov <[email protected]>

---
 net/mctp/af_mctp.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

net/mctp/af_mctp.c +13 -0

index d344b02..bc88159 100644
@@ -33,6 +33,12 @@ static int mctp_release(struct socket *sock)
 	return 0;
 }
 
+/* Generic sockaddr checks, padding checks only so far */
+static bool mctp_sockaddr_is_ok(const struct sockaddr_mctp *addr)
+{
+	return !addr->__smctp_pad0 && !addr->__smctp_pad1;
+}
+
 static int mctp_bind(struct socket *sock, struct sockaddr *addr, int addrlen)
 {
 	struct sock *sk = sock->sk;

@@ -52,6 +58,9 @@ static int mctp_bind(struct socket *sock, struct sockaddr *addr, int addrlen)
 	/* it's a valid sockaddr for MCTP, cast and do protocol checks */
 	smctp = (struct sockaddr_mctp *)addr;
 
+	if (!mctp_sockaddr_is_ok(smctp))
+		return -EINVAL;
+
 	lock_sock(sk);
 
 	/* TODO: allow rebind */

@@ -87,6 +96,8 @@ static int mctp_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
 			return -EINVAL;
 		if (addr->smctp_family != AF_MCTP)
 			return -EINVAL;
+		if (!mctp_sockaddr_is_ok(addr))
+			return -EINVAL;
 		if (addr->smctp_tag & ~(MCTP_TAG_MASK | MCTP_TAG_OWNER))
 			return -EINVAL;

@@ -198,11 +209,13 @@ static int mctp_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
 
 		addr = msg->msg_name;
 		addr->smctp_family = AF_MCTP;
+		addr->__smctp_pad0 = 0;
 		addr->smctp_network = cb->net;
 		addr->smctp_addr.s_addr = hdr->src;
 		addr->smctp_type = type;
 		addr->smctp_tag = hdr->flags_seq_tag &
 					(MCTP_HDR_TAG_MASK | MCTP_HDR_FLAG_TO);
+		addr->__smctp_pad1 = 0;
 		msg->msg_namelen = sizeof(*addr);
 
 		if (msk->addr_ext) {

Signature · 1 line

2.1.4

Eugene Syromiatnikov

author

2021-11-01 17:55 UTC #2 lore raw

PATCH 2/2 mctp: handle the struct sockaddr_mctp_ext padding field

In reply to Eugene Syromiatnikov

struct sockaddr_mctp_ext.__smctp_paddin0 has to be checked for being set to zero, otherwise it cannot be utilised in the future.

Complements: 99ce45d5e7dbde39 ("mctp: Implement extended addressing")

Signed-off-by Eugene Syromiatnikov <[email protected]>

---
 net/mctp/af_mctp.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

net/mctp/af_mctp.c +10 -1

index bc88159..6cd1308 100644
@@ -39,6 +39,13 @@ static bool mctp_sockaddr_is_ok(const struct sockaddr_mctp *addr)
 	return !addr->__smctp_pad0 && !addr->__smctp_pad1;
 }
 
+static bool mctp_sockaddr_ext_is_ok(const struct sockaddr_mctp_ext *addr)
+{
+	return !addr->__smctp_pad0[0]
+	       && !addr->__smctp_pad0[1]
+	       && !addr->__smctp_pad0[2];
+}
+
 static int mctp_bind(struct socket *sock, struct sockaddr *addr, int addrlen)
 {
 	struct sock *sk = sock->sk;

@@ -135,7 +142,8 @@ static int mctp_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
 		DECLARE_SOCKADDR(struct sockaddr_mctp_ext *,
 				 extaddr, msg->msg_name);
 
-		if (extaddr->smctp_halen > sizeof(cb->haddr)) {
+		if (!mctp_sockaddr_ext_is_ok(extaddr)
+		    || extaddr->smctp_halen > sizeof(cb->haddr)) {
 			rc = -EINVAL;
 			goto err_free;
 		}

@@ -224,6 +232,7 @@ static int mctp_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
 			msg->msg_namelen = sizeof(*ae);
 			ae->smctp_ifindex = cb->ifindex;
 			ae->smctp_halen = cb->halen;
+			memset(ae->__smctp_pad0, 0x0, sizeof(ae->__smctp_pad0));
 			memset(ae->smctp_haddr, 0x0, sizeof(ae->smctp_haddr));
 			memcpy(ae->smctp_haddr, cb->haddr, cb->halen);
 		}

Signature · 1 line

2.1.4

Jakub Kicinski

maintainer

2021-11-03 00:55 UTC #3 lore raw

Re: [PATCH net-next 2/2] mctp: handle the struct sockaddr_mctp_ext padding field

net/mctp/af_mctp.c static bool mctp_sockaddr_is_ok(const struct sockaddr_mctp *addr) · line 46

Quoted 5 lines · On Mon, 1 Nov 2021 18:54:53 +0100 Eugene Syromiatnikov wrote:

net/mctp/af_mctp.c +5 -0

+static bool mctp_sockaddr_ext_is_ok(const struct sockaddr_mctp_ext *addr)
+{
+	return !addr->__smctp_pad0[0]
+	       && !addr->__smctp_pad0[1]
+	       && !addr->__smctp_pad0[2];

&& at the end of the previous line please. Checkpatch will point those out to you.

Quoted 1 line

+}

Jeremy Kerr

maintainer

2021-11-02 01:57 UTC #4 lore raw

In reply to Eugene Syromiatnikov

Hi Eugene,

Quoted 2 lines

Padding/reserved fields necessitate appropriate checks in order to be usable in the future.

We don't have a foreseeable need for extra fields here; so this is a bit hypothetical at the moment. However, I guess there may be something that comes up in future - was there something you have in mind?

The requirements for the padding bytes to be zero on sendmsg() will break the ABI for applications that are using the interface on 5.15; there's a small, contained set of those at the moment though, so I'm OK to handle the updates if this patch is accepted, but we'd need to make a call on that soon.

Setting the pad bytes to zero on recvmsg() is a good plan though, I'm happy for that change to go in regardless.

Cheers,

Jeremy

Eugene Syromiatnikov

author

2021-11-02 08:50 UTC #5 lore raw

Quoted 8 lines · Jeremy Kerr

Hi Eugene,
Quoted 2 lines
Padding/reserved fields necessitate appropriate checks in order to be usable in the future.
We don't have a foreseeable need for extra fields here; so this is a bit hypothetical at the moment. However, I guess there may be something that comes up in future - was there something you have in mind?

Not really, but reality suggests that many interfaces tend to extend over time (including socket addresses, see flags in sockaddr_vm as an example), so future-proofing padding allows extending into it with minimal implementation complication, comparing to other approaches.

Quoted 5 lines

The requirements for the padding bytes to be zero on sendmsg() will break the ABI for applications that are using the interface on 5.15; there's a small, contained set of those at the moment though, so I'm OK to handle the updates if this patch is accepted, but we'd need to make a call on that soon.

Yeah, I regret I have not caught it earlier.

Quoted 2 lines

Setting the pad bytes to zero on recvmsg() is a good plan though, I'm happy for that change to go in regardless.

I can split it out in case there is hesitance with regards to applying padding checks.