On 14-02-2023 12:06, Teo Couprie Diaz wrote:
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) : \ (struct compat_cmsghdr __user *)NULL) #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 :)
The checkpatch character limit is 100 regardless of subsystem (see bdc48fa11e46f: "checkpatch/coding-style: deprecate 80-column warning"). 80-column limit is still preferred though.
Tudor
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; 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); @@ -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; msg->msg_controllen -= cmlen; return 0; @@ -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; 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); @@ -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); zc->msg_controllen = (__u64)cmsg_dummy.msg_controllen; zc->msg_flags = (__u32)cmsg_dummy.msg_flags;
linux-morello mailing list -- linux-morello@op-lists.linaro.org To unsubscribe send an email to linux-morello-leave@op-lists.linaro.org