Skip to contentSkip to conversation

MCTP sockaddr padding check/initialisation fixup

PATCH Applied Eugene Syromiatnikov 4 participants 7 messages Nov 3, 2021 – Nov 5, 2021

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

v2 cover MCTP sockaddr padding check/initialisation fixup

Hello.

This pair of patches introduces checks for padding fields of struct sockaddr_mctp/sockaddr_mctp_ext to ease their re-use for possible extensions in the future; as well as zeroing of these fields in the respective sockaddr filling routines. While the first commit is definitely an ABI breakage, it is proposed in hopes that the change is made soon enough (the interface appeared only in Linux 5.15) to avoid affecting any existing user space.

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

--
v2:
- Fixed the style of logical continuations, as noted by Jakub Kicinski.
- Changed "Complements:" to "Fixes:" in commit messages, as it seems that it is the only commit message tag checkpatch.pl recognises.

v1: https://lore.kernel.org/netdev/[email protected]/

Signature · 1 line
2.1.4
#1 lore raw

PATCH v2 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.

Fixes 5a20dd46b8b84593 ("mctp: Be explicit about struct sockaddr_mctp padding")
---
 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
#2 lore raw

PATCH v2 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.

Fixes 99ce45d5e7dbde39 ("mctp: Implement extended addressing")
---
 net/mctp/af_mctp.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)
net/mctp/af_mctp.c +10 -1
index bc88159..871cf62 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
maintainer
#3 lore raw
In reply to Eugene Syromiatnikov
Quoted 7 lines · On Wed, 3 Nov 2021 20:09:36 +0100 Eugene Syromiatnikov wrote:

This pair of patches introduces checks for padding fields of struct sockaddr_mctp/sockaddr_mctp_ext to ease their re-use for possible extensions in the future; as well as zeroing of these fields in the respective sockaddr filling routines. While the first commit is definitely an ABI breakage, it is proposed in hopes that the change is made soon enough (the interface appeared only in Linux 5.15) to avoid affecting any existing user space.

Seems reasonable, Jeremy can you send an ack?

maintainer
#4 lore raw

Hi Jakub,

Quoted 11 lines
Quoted 9 lines

This pair of patches introduces checks for padding fields of struct sockaddr_mctp/sockaddr_mctp_ext to ease their re-use for possible extensions in the future;  as well as zeroing of these fields in the respective sockaddr filling routines.  While the first commit
is definitely an ABI breakage, it is proposed in hopes that the change
is made soon enough (the interface appeared only in Linux 5.15) to avoid affecting any existing user space.

Seems reasonable, Jeremy can you send an ack?

Yep, will do ASAP - I'm planning also send references to the commits ton the userspace side, so that we have a record of where everything lines up on the updated ABI.

I'll have that done later today.

Cheers,

Jeremy

maintainer Acked-by on the v2 series
#5 lore raw
In reply to Eugene Syromiatnikov

Hi Eugene,

Quoted 3 lines

While the first commit is definitely an ABI breakage, it is proposed in hopes that the change is made soon enough (the interface appeared only in Linux 5.15) to avoid affecting any existing user space.

Of the two applications that currently use AF_MCTP:

- one is already zeroing the sockaddr_mctp
- the other has a fix for two of the potential sendmsg() & bind()
calls: https://github.com/CodeConstruct/mctp/commit/072bafe7

Given we have a confined set of applications (and users), and they're both now compatible with this change:

For both patches.

Cheers,

Jeremy

#6 lore raw
In reply to Eugene Syromiatnikov
This series was applied to netdev/net.git (master) by Jakub Kicinski <[email protected]>:

Hello:

This series was applied to netdev/net.git (master) by Jakub Kicinski <[email protected]>:

Quoted 11 lines · On Wed, 3 Nov 2021 20:09:36 +0100 you wrote:

Hello.

This pair of patches introduces checks for padding fields of struct sockaddr_mctp/sockaddr_mctp_ext to ease their re-use for possible extensions in the future; as well as zeroing of these fields in the respective sockaddr filling routines. While the first commit is definitely an ABI breakage, it is proposed in hopes that the change is made soon enough (the interface appeared only in Linux 5.15) to avoid affecting any existing user space.

[...]

Here is the summary with links:
- [net-next,v2,1/2] mctp: handle the struct sockaddr_mctp padding fields

    https://git.kernel.org/netdev/net/c/1e4b50f06d97
  - [net-next,v2,2/2] mctp: handle the struct sockaddr_mctp_ext padding field
    https://git.kernel.org/netdev/net/c/e9ea574ec1c2

You are awesome, thank you!

Signature · 2 lines
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html