Hi Kevin, just some nits and questions. Other than that it looks good, and so does the rest of the series !
On 10/02/2023 09:16, Kevin Brodsky wrote:
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 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 missed a number of cases, and some code using
missed -> miss Could it be better with comas around the "however", or maybe moved at the start of the sentence ? ("However, it did miss [...]")
msg_control inappropriately has also appeared in the meantime.
This commit is attempting to complete the split. Most issues are about msg_control being used when in fact a user pointer is stored in the union; msg_control_user is now used instead. An exception is made for null checks, as it should be safe to use msg_control unconditionally for that purpose.
Additionally, a special situation in cmsghdr_from_user_compat_to_kern() is addressed. There the input struct msghdr holds a user pointer (msg_control_user), but a kernel pointer is stored in msg_control when returning. msg_control_is_user is now updated accordingly.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com
This is for upstream, see notes in cover letter.
net/compat.c | 13 +++++++------ net/core/scm.c | 9 ++++++--- net/ipv4/tcp.c | 4 ++-- 3 files changed, 15 insertions(+), 11 deletions(-)
diff --git a/net/compat.c b/net/compat.c index f2fcf4204a1d..e10570f549a1 100644 --- a/net/compat.c +++ b/net/compat.c @@ -120,7 +120,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) : \#define CMSG_COMPAT_OK(ucmlen, ucmsg, mhdr) \ @@ -133,7 +133,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;
@@ -218,6 +218,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;
@@ -232,7 +233,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;
This line is quite long but checkpatch.pl doesn't complain. Does the net subsystem have a longer character limit ? Maybe I have just used 80 characters wrongly the whole time :)
Téo
struct compat_cmsghdr cmhdr; struct old_timeval32 ctv; struct old_timespec32 cts[3]; @@ -281,7 +282,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; }
@@ -296,7 +297,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;@@ -320,7 +321,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 5c356f0dee30..cb9dc17f1139 100644 --- a/net/core/scm.c +++ b/net/core/scm.c @@ -248,7 +248,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;@@ -297,7 +300,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;@@ -330,7 +333,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 17b213acffc8..8f5e605ea673 100644 --- a/net/ipv4/tcp.c +++ b/net/ipv4/tcp.c @@ -2163,7 +2163,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()
@@ -2174,7 +2174,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);