[PATCH v2 0/3] net: Finish up ->msg_control{,_user} split

[PATCH v2 0/3] net: Finish up ->msg_control{,_user} split

[Kevin Brodsky]

Hi,

Commit 1f466e1f15cf ("net: cleanly handle kernel vs user buffers for
->msg_control") introduced the msg_control_user and
msg_control_is_user fields in struct msghdr, to ensure that user
pointers are represented as such. It also took care of converting most
users of struct msghdr::msg_control where user pointers are involved. It
did however miss a number of cases, and some code using msg_control
inappropriately has also appeared in the meantime.

This series is attempting to complete the split, by eliminating the
remaining cases where msg_control is used when in fact a user
pointer is stored in the union (patch 1).

It also addresses a couple of issues with msg_control_is_user: one where
it is not updated as it should (patch 2), and one where it is not
initialised (patch 3).

v1..v2:
* Split out the msg_control_is_user fixes into separate patches.

v1: https://lore.kernel.org/all/20230411122625.3902339-1-kevin.brodsky@arm.com/

Thanks,
Kevin

Cc: Christoph Hellwig <hch@lst.de>
Cc: Eric Dumazet <edumazet@google.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Jakub Kicinski <kuba@kernel.org>

Kevin Brodsky (3):
  net: Ensure ->msg_control_user is used for user buffers
  net/compat: Update msg_control_is_user when setting a kernel pointer
  net/ipv6: Initialise msg_control_is_user

 net/compat.c             | 13 +++++++------
 net/core/scm.c           |  9 ++++++---
 net/ipv4/tcp.c           |  4 ++--
 net/ipv6/ipv6_sockglue.c |  1 +
 4 files changed, 16 insertions(+), 11 deletions(-)

-- 
2.38.1

[PATCH v2 1/3] net: Ensure ->msg_control_user is used for user buffers

[Kevin Brodsky]

Since commit 1f466e1f15cf ("net: cleanly handle kernel vs user
buffers for ->msg_control"), pointers to user buffers should be
stored in struct msghdr::msg_control_user, instead of the
msg_control field.  Most users of msg_control have already been
converted (where user buffers are involved), but not all of them.

This patch attempts to address the remaining cases. An exception is
made for null checks, as it should be safe to use msg_control
unconditionally for that purpose.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Eric Dumazet <edumazet@google.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Kevin Brodsky <kevin.brodsky@arm.com>
---
 net/compat.c   | 12 ++++++------
 net/core/scm.c |  9 ++++++---
 net/ipv4/tcp.c |  4 ++--
 3 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/net/compat.c b/net/compat.c
index 161b7bea1f62..000a2e054d4c 100644
--- a/net/compat.c
+++ b/net/compat.c
@@ -113,7 +113,7 @@ int get_compat_msghdr(struct msghdr *kmsg,
 
 #define CMSG_COMPAT_FIRSTHDR(msg)			\
 	(((msg)->msg_controllen) >= sizeof(struct compat_cmsghdr) ?	\
-	 (struct compat_cmsghdr __user *)((msg)->msg_control) :		\
+	 (struct compat_cmsghdr __user *)((msg)->msg_control_user) :	\
 	 (struct compat_cmsghdr __user *)NULL)
 
 #define CMSG_COMPAT_OK(ucmlen, ucmsg, mhdr) \
@@ -126,7 +126,7 @@ static inline struct compat_cmsghdr __user *cmsg_compat_nxthdr(struct msghdr *ms
 		struct compat_cmsghdr __user *cmsg, int cmsg_len)
 {
 	char __user *ptr = (char __user *)cmsg + CMSG_COMPAT_ALIGN(cmsg_len);
-	if ((unsigned long)(ptr + 1 - (char __user *)msg->msg_control) >
+	if ((unsigned long)(ptr + 1 - (char __user *)msg->msg_control_user) >
 			msg->msg_controllen)
 		return NULL;
 	return (struct compat_cmsghdr __user *)ptr;
@@ -225,7 +225,7 @@ int cmsghdr_from_user_compat_to_kern(struct msghdr *kmsg, struct sock *sk,
 
 int put_cmsg_compat(struct msghdr *kmsg, int level, int type, int len, void *data)
 {
-	struct compat_cmsghdr __user *cm = (struct compat_cmsghdr __user *) kmsg->msg_control;
+	struct compat_cmsghdr __user *cm = (struct compat_cmsghdr __user *) kmsg->msg_control_user;
 	struct compat_cmsghdr cmhdr;
 	struct old_timeval32 ctv;
 	struct old_timespec32 cts[3];
@@ -274,7 +274,7 @@ int put_cmsg_compat(struct msghdr *kmsg, int level, int type, int len, void *dat
 	cmlen = CMSG_COMPAT_SPACE(len);
 	if (kmsg->msg_controllen < cmlen)
 		cmlen = kmsg->msg_controllen;
-	kmsg->msg_control += cmlen;
+	kmsg->msg_control_user += cmlen;
 	kmsg->msg_controllen -= cmlen;
 	return 0;
 }
@@ -289,7 +289,7 @@ static int scm_max_fds_compat(struct msghdr *msg)
 void scm_detach_fds_compat(struct msghdr *msg, struct scm_cookie *scm)
 {
 	struct compat_cmsghdr __user *cm =
-		(struct compat_cmsghdr __user *)msg->msg_control;
+		(struct compat_cmsghdr __user *)msg->msg_control_user;
 	unsigned int o_flags = (msg->msg_flags & MSG_CMSG_CLOEXEC) ? O_CLOEXEC : 0;
 	int fdmax = min_t(int, scm_max_fds_compat(msg), scm->fp->count);
 	int __user *cmsg_data = CMSG_COMPAT_DATA(cm);
@@ -313,7 +313,7 @@ void scm_detach_fds_compat(struct msghdr *msg, struct scm_cookie *scm)
 			cmlen = CMSG_COMPAT_SPACE(i * sizeof(int));
 			if (msg->msg_controllen < cmlen)
 				cmlen = msg->msg_controllen;
-			msg->msg_control += cmlen;
+			msg->msg_control_user += cmlen;
 			msg->msg_controllen -= cmlen;
 		}
 	}
diff --git a/net/core/scm.c b/net/core/scm.c
index acb7d776fa6e..3cd7dd377e53 100644
--- a/net/core/scm.c
+++ b/net/core/scm.c
@@ -250,7 +250,10 @@ int put_cmsg(struct msghdr * msg, int level, int type, int len, void *data)
 	}
 
 	cmlen = min(CMSG_SPACE(len), msg->msg_controllen);
-	msg->msg_control += cmlen;
+	if (msg->msg_control_is_user)
+		msg->msg_control_user += cmlen;
+	else
+		msg->msg_control += cmlen;
 	msg->msg_controllen -= cmlen;
 	return 0;
 
@@ -299,7 +302,7 @@ static int scm_max_fds(struct msghdr *msg)
 void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm)
 {
 	struct cmsghdr __user *cm =
-		(__force struct cmsghdr __user *)msg->msg_control;
+		(__force struct cmsghdr __user *)msg->msg_control_user;
 	unsigned int o_flags = (msg->msg_flags & MSG_CMSG_CLOEXEC) ? O_CLOEXEC : 0;
 	int fdmax = min_t(int, scm_max_fds(msg), scm->fp->count);
 	int __user *cmsg_data = CMSG_USER_DATA(cm);
@@ -332,7 +335,7 @@ void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm)
 			cmlen = CMSG_SPACE(i * sizeof(int));
 			if (msg->msg_controllen < cmlen)
 				cmlen = msg->msg_controllen;
-			msg->msg_control += cmlen;
+			msg->msg_control_user += cmlen;
 			msg->msg_controllen -= cmlen;
 		}
 	}
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 288693981b00..6fa7a3fa9abd 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -2164,7 +2164,7 @@ static void tcp_zc_finalize_rx_tstamp(struct sock *sk,
 	struct msghdr cmsg_dummy;
 
 	msg_control_addr = (unsigned long)zc->msg_control;
-	cmsg_dummy.msg_control = (void *)msg_control_addr;
+	cmsg_dummy.msg_control_user = (void __user *)msg_control_addr;
 	cmsg_dummy.msg_controllen =
 		(__kernel_size_t)zc->msg_controllen;
 	cmsg_dummy.msg_flags = in_compat_syscall()
@@ -2175,7 +2175,7 @@ static void tcp_zc_finalize_rx_tstamp(struct sock *sk,
 	    zc->msg_controllen == cmsg_dummy.msg_controllen) {
 		tcp_recv_timestamp(&cmsg_dummy, sk, tss);
 		zc->msg_control = (__u64)
-			((uintptr_t)cmsg_dummy.msg_control);
+			((uintptr_t)cmsg_dummy.msg_control_user);
 		zc->msg_controllen =
 			(__u64)cmsg_dummy.msg_controllen;
 		zc->msg_flags = (__u32)cmsg_dummy.msg_flags;
-- 
2.38.1

Re: [PATCH v2 1/3] net: Ensure ->msg_control_user is used for user buffers

[Christoph Hellwig]

On Thu, Apr 13, 2023 at 12:47:03PM +0100, Kevin Brodsky wrote:
> Since commit 1f466e1f15cf ("net: cleanly handle kernel vs user
> buffers for ->msg_control"), pointers to user buffers should be
> stored in struct msghdr::msg_control_user, instead of the
> msg_control field.  Most users of msg_control have already been
> converted (where user buffers are involved), but not all of them.
> 
> This patch attempts to address the remaining cases. An exception is
> made for null checks, as it should be safe to use msg_control
> unconditionally for that purpose.

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

although I would have expected this at the end of the series.  Given
that the patches don't overlap it shouldn't really matter in the end,
though.

[PATCH v2 2/3] net/compat: Update msg_control_is_user when setting a kernel pointer

[Kevin Brodsky]

cmsghdr_from_user_compat_to_kern() is an unusual case w.r.t. how
the kmsg->msg_control* fields are used. The input struct msghdr
holds a pointer to a user buffer, i.e. ksmg->msg_control_user is
active. However, upon success, a kernel pointer is stored in
kmsg->msg_control. kmsg->msg_control_is_user should therefore be
updated accordingly.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Eric Dumazet <edumazet@google.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Kevin Brodsky <kevin.brodsky@arm.com>
---
 net/compat.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/compat.c b/net/compat.c
index 000a2e054d4c..6564720f32b7 100644
--- a/net/compat.c
+++ b/net/compat.c
@@ -211,6 +211,7 @@ int cmsghdr_from_user_compat_to_kern(struct msghdr *kmsg, struct sock *sk,
 		goto Einval;
 
 	/* Ok, looks like we made it.  Hook it up and return success. */
+	kmsg->msg_control_is_user = false;
 	kmsg->msg_control = kcmsg_base;
 	kmsg->msg_controllen = kcmlen;
 	return 0;
-- 
2.38.1

Re: [PATCH v2 2/3] net/compat: Update msg_control_is_user when setting a kernel pointer

[Christoph Hellwig]

On Thu, Apr 13, 2023 at 12:47:04PM +0100, Kevin Brodsky wrote:
> cmsghdr_from_user_compat_to_kern() is an unusual case w.r.t. how
> the kmsg->msg_control* fields are used. The input struct msghdr
> holds a pointer to a user buffer, i.e. ksmg->msg_control_user is
> active. However, upon success, a kernel pointer is stored in
> kmsg->msg_control. kmsg->msg_control_is_user should therefore be
> updated accordingly.

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH v2 3/3] net/ipv6: Initialise msg_control_is_user

[Kevin Brodsky]

do_ipv6_setsockopt() makes use of struct msghdr::msg_control in the
IPV6_2292PKTOPTIONS case. Make sure to initialise
msg_control_is_user accordingly.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Eric Dumazet <edumazet@google.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Kevin Brodsky <kevin.brodsky@arm.com>
---
 net/ipv6/ipv6_sockglue.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
index 2917dd8d198c..ae818ff46224 100644
--- a/net/ipv6/ipv6_sockglue.c
+++ b/net/ipv6/ipv6_sockglue.c
@@ -716,6 +716,7 @@ int do_ipv6_setsockopt(struct sock *sk, int level, int optname,
 			goto done;
 
 		msg.msg_controllen = optlen;
+		msg.msg_control_is_user = false;
 		msg.msg_control = (void *)(opt+1);
 		ipc6.opt = opt;
 
-- 
2.38.1

Re: [PATCH v2 3/3] net/ipv6: Initialise msg_control_is_user

[Christoph Hellwig]

On Thu, Apr 13, 2023 at 12:47:05PM +0100, Kevin Brodsky wrote:
> do_ipv6_setsockopt() makes use of struct msghdr::msg_control in the
> IPV6_2292PKTOPTIONS case. Make sure to initialise
> msg_control_is_user accordingly.

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 0/3] net: Finish up ->msg_control{,_user} split

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by David S. Miller <davem@davemloft.net>:

On Thu, 13 Apr 2023 12:47:02 +0100 you wrote:
> Hi,
> 
> Commit 1f466e1f15cf ("net: cleanly handle kernel vs user buffers for
> ->msg_control") introduced the msg_control_user and
> msg_control_is_user fields in struct msghdr, to ensure that user
> pointers are represented as such. It also took care of converting most
> users of struct msghdr::msg_control where user pointers are involved. It
> did however miss a number of cases, and some code using msg_control
> inappropriately has also appeared in the meantime.
> 
> [...]

Here is the summary with links:
  - [v2,1/3] net: Ensure ->msg_control_user is used for user buffers
    https://git.kernel.org/netdev/net-next/c/c39ef2130491
  - [v2,2/3] net/compat: Update msg_control_is_user when setting a kernel pointer
    https://git.kernel.org/netdev/net-next/c/60daf8d40b80
  - [v2,3/3] net/ipv6: Initialise msg_control_is_user
    https://git.kernel.org/netdev/net-next/c/b6d85cf5bd14

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html