Hi,
Following up on Luca's series addressing ioctl-related GCC warnings, this series takes care of the remaining warnings reported by GCC (but not Clang). The patches are all straightforward, except patch 13 (see below).
Some notes:
- Patch 1-3 fix up small oversights in our own commits (see Fixes: tags).
- Patch 9-10 are about creating user pointers in cases where it is unavoidable, such cases are not concerning.
- Patch 11-12 and 14-18 are about creating user pointers because of uapi deficiencies (i.e. user pointers represented as __u64 in structs). They should therefore be viewed as a stopgap while waiting for a full solution (uapi change and compat handling); this is tracked by the following tickets:
* Patch 11: https://git.morello-project.org/morello/kernel/linux/-/issues/48 * Patch 12: https://git.morello-project.org/morello/kernel/linux/-/issues/47 * Patch 14: https://git.morello-project.org/morello/kernel/linux/-/issues/46 * Patch 15: https://git.morello-project.org/morello/kernel/linux/-/issues/50 * Patch 16: https://git.morello-project.org/morello/kernel/linux/-/issues/51 * Patch 17-18: https://git.morello-project.org/morello/kernel/linux/-/issues/52
- Patch 13 is upstreamable and is written in that spirit. Fixing up the msg_control/msg_control_user split required grepping all uses, as only a few misuses actually lead to warnings in PCuABI. Note that as an exceptional bending of the rules, this patch introduces a warning (both with Clang and GCC), which is impossible to avoid in a patch for upstream. The next patch (14) fixes the warnings so it should not be an issue in practice.
With these patches and Luca's, only a handful of genuine GCC warnings are left: - A few in fs/aio.c, tracked by https://git.morello-project.org/morello/kernel/linux/-/issues/49 - A few under kernel/bpf/, should be addressed by https://git.morello-project.org/morello/kernel/linux/-/issues/4
Other warnings are Morello GCC defects and have been reported.
Thanks, Kevin
Kevin Brodsky (18): arm64: signal: Avoid unnecessary capability arithmetic drivers/android/binder: Cast to (void *) when printing %p drivers/android/binder: Cast user_uintptr_t arg when printing math.h: Make round_{up,down} capability-friendly asm-generic/cacheflush.h: Use appropriate user pointer conversion kernel: signal: Use appropriate user pointer conversion perf/core: Use appropriate user pointer conversion compat: Fix shift warning in compat64 audit: Explicitly create user pointer coredump: Explicitly create user pointer kernel/futex: Explicitly create user pointer rseq: Explicitly create user pointers net: Finish up ->msg_control{,_user} split tcp: Explicitly create user pointers Input: evdev: Explicitly create user pointers mmc: block: Explicitly create user pointers drm: Explicitly create user pointers drm/panfrost: Explicitly create user pointers
arch/arm64/kernel/signal.c | 6 +++-- drivers/android/binder.c | 4 ++-- drivers/android/binder_alloc.c | 25 +++++++++++---------- drivers/gpu/drm/drm_atomic_uapi.c | 8 +++---- drivers/gpu/drm/drm_color_mgmt.c | 12 +++++----- drivers/gpu/drm/drm_connector.c | 8 +++---- drivers/gpu/drm/drm_crtc.c | 2 +- drivers/gpu/drm/drm_framebuffer.c | 2 +- drivers/gpu/drm/drm_lease.c | 4 ++-- drivers/gpu/drm/drm_mode_object.c | 4 ++-- drivers/gpu/drm/drm_plane.c | 2 +- drivers/gpu/drm/panfrost/panfrost_drv.c | 4 ++-- drivers/gpu/drm/panfrost/panfrost_perfcnt.c | 2 +- drivers/input/evdev.c | 4 ++-- drivers/mmc/core/block.c | 6 ++--- fs/coredump.c | 2 +- include/asm-generic/cacheflush.h | 6 ++--- include/linux/math.h | 5 +++++ kernel/auditsc.c | 2 +- kernel/compat.c | 4 ++-- kernel/events/core.c | 2 +- kernel/futex/waitwake.c | 2 +- kernel/rseq.c | 4 ++-- kernel/signal.c | 2 +- net/compat.c | 13 ++++++----- net/core/scm.c | 9 +++++--- net/ipv4/tcp.c | 8 +++---- 27 files changed, 82 insertions(+), 70 deletions(-)
sp_top is only used to compute the size to provide to access_ok(). It should therefore be just an address, not a capability (in PCuABI). GCC rightly warns about this situation, as subtracting two capabilities is ambiguous (it may be interpreted as a capability operation subtracting the address of the LHS with the address of the RHS, while here we expect a simple integer subtraction).
Fixes: ("linux/sched/signal.h: Modify the stack pointer to user_uintptr_t") Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- arch/arm64/kernel/signal.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c index b804457a36b2..80289e90fc66 100644 --- a/arch/arm64/kernel/signal.c +++ b/arch/arm64/kernel/signal.c @@ -1001,7 +1001,8 @@ static int setup_sigframe(struct rt_sigframe_user_layout *user, static int get_sigframe(struct rt_sigframe_user_layout *user, struct ksignal *ksig, struct pt_regs *regs) { - user_uintptr_t sp, sp_top; + user_uintptr_t sp; + ptraddr_t sp_top; int err;
init_user_layout(user); @@ -1009,7 +1010,8 @@ static int get_sigframe(struct rt_sigframe_user_layout *user, if (err) return err;
- sp = sp_top = sigsp(signal_sp(regs), ksig); + sp = sigsp(signal_sp(regs), ksig); + sp_top = (ptraddr_t)sp;
sp = round_down(sp - sizeof(struct frame_record), 16); user->next_frame = (struct frame_record __user *)sp;
On 10-02-2023 09:16, Kevin Brodsky wrote:
sp_top is only used to compute the size to provide to access_ok(). It should therefore be just an address, not a capability (in PCuABI). GCC rightly warns about this situation, as subtracting two capabilities is ambiguous (it may be interpreted as a capability operation subtracting the address of the LHS with the address of the RHS, while here we expect a simple integer subtraction).
Fixes: ("linux/sched/signal.h: Modify the stack pointer to user_uintptr_t") Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com
arch/arm64/kernel/signal.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c index b804457a36b2..80289e90fc66 100644 --- a/arch/arm64/kernel/signal.c +++ b/arch/arm64/kernel/signal.c
nit: I'm not sure on this, should we include linux/types.h explicitly for ptraddr_t?
Thanks, Tudor
@@ -1001,7 +1001,8 @@ static int setup_sigframe(struct rt_sigframe_user_layout *user, static int get_sigframe(struct rt_sigframe_user_layout *user, struct ksignal *ksig, struct pt_regs *regs) {
- user_uintptr_t sp, sp_top;
- user_uintptr_t sp;
- ptraddr_t sp_top; int err;
init_user_layout(user); @@ -1009,7 +1010,8 @@ static int get_sigframe(struct rt_sigframe_user_layout *user, if (err) return err;
- sp = sp_top = sigsp(signal_sp(regs), ksig);
- sp = sigsp(signal_sp(regs), ksig);
- sp_top = (ptraddr_t)sp;
sp = round_down(sp - sizeof(struct frame_record), 16); user->next_frame = (struct frame_record __user *)sp;
On 13/02/2023 11:30, Tudor Cretu wrote:
On 10-02-2023 09:16, Kevin Brodsky wrote:
sp_top is only used to compute the size to provide to access_ok(). It should therefore be just an address, not a capability (in PCuABI). GCC rightly warns about this situation, as subtracting two capabilities is ambiguous (it may be interpreted as a capability operation subtracting the address of the LHS with the address of the RHS, while here we expect a simple integer subtraction).
Fixes: ("linux/sched/signal.h: Modify the stack pointer to user_uintptr_t") Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com
arch/arm64/kernel/signal.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c index b804457a36b2..80289e90fc66 100644 --- a/arch/arm64/kernel/signal.c +++ b/arch/arm64/kernel/signal.c
nit: I'm not sure on this, should we include linux/types.h explicitly for ptraddr_t?
We could, but it's not essential, as we already include <linux/kernel.h> and it itself includes <linux/types.h>, which is something that I think we can reasonably rely on.
Kevin
Thanks, Tudor
@@ -1001,7 +1001,8 @@ static int setup_sigframe(struct rt_sigframe_user_layout *user, static int get_sigframe(struct rt_sigframe_user_layout *user, struct ksignal *ksig, struct pt_regs *regs) { - user_uintptr_t sp, sp_top; + user_uintptr_t sp; + ptraddr_t sp_top; int err; init_user_layout(user); @@ -1009,7 +1010,8 @@ static int get_sigframe(struct rt_sigframe_user_layout *user, if (err) return err; - sp = sp_top = sigsp(signal_sp(regs), ksig); + sp = sigsp(signal_sp(regs), ksig); + sp_top = (ptraddr_t)sp; sp = round_down(sp - sizeof(struct frame_record), 16); user->next_frame = (struct frame_record __user *)sp;
On 14-02-2023 08:38, Kevin Brodsky wrote:
On 13/02/2023 11:30, Tudor Cretu wrote:
On 10-02-2023 09:16, Kevin Brodsky wrote:
sp_top is only used to compute the size to provide to access_ok(). It should therefore be just an address, not a capability (in PCuABI). GCC rightly warns about this situation, as subtracting two capabilities is ambiguous (it may be interpreted as a capability operation subtracting the address of the LHS with the address of the RHS, while here we expect a simple integer subtraction).
Fixes: ("linux/sched/signal.h: Modify the stack pointer to user_uintptr_t") Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com
arch/arm64/kernel/signal.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c index b804457a36b2..80289e90fc66 100644 --- a/arch/arm64/kernel/signal.c +++ b/arch/arm64/kernel/signal.c
nit: I'm not sure on this, should we include linux/types.h explicitly for ptraddr_t?
We could, but it's not essential, as we already include <linux/kernel.h> and it itself includes <linux/types.h>, which is something that I think we can reasonably rely on.
Indeed. Thanks for the clarification!
Tudor
Kevin
Thanks, Tudor
@@ -1001,7 +1001,8 @@ static int setup_sigframe(struct rt_sigframe_user_layout *user, static int get_sigframe(struct rt_sigframe_user_layout *user, struct ksignal *ksig, struct pt_regs *regs) { - user_uintptr_t sp, sp_top; + user_uintptr_t sp; + ptraddr_t sp_top; int err; init_user_layout(user); @@ -1009,7 +1010,8 @@ static int get_sigframe(struct rt_sigframe_user_layout *user, if (err) return err; - sp = sp_top = sigsp(signal_sp(regs), ksig); + sp = sigsp(signal_sp(regs), ksig); + sp_top = (ptraddr_t)sp; sp = round_down(sp - sizeof(struct frame_record), 16); user->next_frame = (struct frame_record __user *)sp;
Commit "drivers/android/binder_alloc: Represent user addresses as integers" changed the type of many variables / members representing addresses from void * to unsigned long. It however failed to introduce the appropriate (void *) casts when printing these variables, which is the type %p expects. GCC warns about this sort of mismatch; add the casts to make it happy.
Fixes: ("drivers/android/binder_alloc: Represent user addresses as integers") Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- drivers/android/binder.c | 2 +- drivers/android/binder_alloc.c | 25 +++++++++++++------------ 2 files changed, 14 insertions(+), 13 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c index b9280b58c058..bccf30d0fa4f 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -5955,7 +5955,7 @@ static void print_binder_transaction_ilocked(struct seq_file *m, seq_printf(m, " node %d", buffer->target_node->debug_id); seq_printf(m, " size %zd:%zd data %pK\n", buffer->data_size, buffer->offsets_size, - buffer->user_data); + (void *)buffer->user_data); }
static void print_binder_work_ilocked(struct seq_file *m, diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c index 8f6190b490c5..44b094060f75 100644 --- a/drivers/android/binder_alloc.c +++ b/drivers/android/binder_alloc.c @@ -190,7 +190,8 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate,
binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC, "%d: %s pages %pK-%pK\n", alloc->pid, - allocate ? "allocate" : "free", start, end); + allocate ? "allocate" : "free", + (void *)start, (void *)end);
if (end <= start) return 0; @@ -250,7 +251,7 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate, __GFP_ZERO); if (!page->page_ptr) { pr_err("%d: binder_alloc_buf failed for page at %pK\n", - alloc->pid, page_addr); + alloc->pid, (void *)page_addr); goto err_alloc_page_failed; } page->alloc = alloc; @@ -594,8 +595,8 @@ static void binder_delete_free_buffer(struct binder_alloc *alloc, to_free = false; binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC, "%d: merge free, buffer %pK share page with %pK\n", - alloc->pid, buffer->user_data, - prev->user_data); + alloc->pid, (void *)buffer->user_data, + (void *)prev->user_data); }
if (!list_is_last(&buffer->entry, &alloc->buffers)) { @@ -605,24 +606,24 @@ static void binder_delete_free_buffer(struct binder_alloc *alloc, binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC, "%d: merge free, buffer %pK share page with %pK\n", alloc->pid, - buffer->user_data, - next->user_data); + (void *)buffer->user_data, + (void *)next->user_data); } }
if (PAGE_ALIGNED(buffer->user_data)) { binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC, "%d: merge free, buffer start %pK is page aligned\n", - alloc->pid, buffer->user_data); + alloc->pid, (void *)buffer->user_data); to_free = false; }
if (to_free) { binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC, "%d: merge free, buffer %pK do not share page with %pK or %pK\n", - alloc->pid, buffer->user_data, - prev->user_data, - next ? next->user_data : 0); + alloc->pid, (void *)buffer->user_data, + (void *)prev->user_data, + (void *)(next ? next->user_data : 0)); binder_update_page_range(alloc, 0, buffer_start_page(buffer), buffer_start_page(buffer) + PAGE_SIZE); } @@ -846,7 +847,7 @@ void binder_alloc_deferred_release(struct binder_alloc *alloc) page_addr = alloc->buffer + i * PAGE_SIZE; binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC, "%s: %d: page %d at %pK %s\n", - __func__, alloc->pid, i, page_addr, + __func__, alloc->pid, i, (void *)page_addr, on_lru ? "on lru" : "active"); __free_page(alloc->pages[i].page_ptr); page_count++; @@ -866,7 +867,7 @@ static void print_binder_buffer(struct seq_file *m, const char *prefix, struct binder_buffer *buffer) { seq_printf(m, "%s %d: %pK size %zd:%zd:%zd %s\n", - prefix, buffer->debug_id, buffer->user_data, + prefix, buffer->debug_id, (void *)buffer->user_data, buffer->data_size, buffer->offsets_size, buffer->extra_buffers_size, buffer->transaction ? "active" : "delivered");
%lx expects an (unsigned) long, but user_uintptr_t is a different type in PCuABI (uintcap_t). Cast arg to (unsigned long) to avoid a warning when building withg GCC.
Fixes: ("fs/ioctl: Modify 3rd argument of fops->unlocked_ioctl to user_uintptr_t") Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- drivers/android/binder.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c index bccf30d0fa4f..6ada02805d6a 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -5516,7 +5516,7 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, user_uintptr_t arg thread->looper_need_return = false; wait_event_interruptible(binder_user_error_wait, binder_stop_on_user_error < 2); if (ret && ret != -EINTR) - pr_info("%d:%d ioctl %x %lx returned %d\n", proc->pid, current->pid, cmd, arg, ret); + pr_info("%d:%d ioctl %x %lx returned %d\n", proc->pid, current->pid, cmd, (unsigned long)arg, ret); err_unlocked: trace_binder_ioctl_done(ret); return ret;
On 2/10/23 14:46, Kevin Brodsky wrote:
%lx expects an (unsigned) long, but user_uintptr_t is a different type in PCuABI (uintcap_t). Cast arg to (unsigned long) to avoid a warning when building withg GCC.
s/withg/with
Fixes: ("fs/ioctl: Modify 3rd argument of fops->unlocked_ioctl to user_uintptr_t") Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com
drivers/android/binder.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c index bccf30d0fa4f..6ada02805d6a 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -5516,7 +5516,7 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, user_uintptr_t arg thread->looper_need_return = false; wait_event_interruptible(binder_user_error_wait, binder_stop_on_user_error < 2); if (ret && ret != -EINTR)
pr_info("%d:%d ioctl %x %lx returned %d\n", proc->pid, current->pid, cmd, arg, ret);
pr_info("%d:%d ioctl %x %lx returned %d\n", proc->pid, current->pid, cmd, (unsigned long)arg, ret);
On 15/02/2023 08:07, Amit Daniel Kachhap wrote:
On 2/10/23 14:46, Kevin Brodsky wrote:
%lx expects an (unsigned) long, but user_uintptr_t is a different type in PCuABI (uintcap_t). Cast arg to (unsigned long) to avoid a warning when building withg GCC.
s/withg/with
Good catch! I thought I had run the spellchecker on the series but clearly not...
Kevin
Fixes: ("fs/ioctl: Modify 3rd argument of fops->unlocked_ioctl to user_uintptr_t") Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com
drivers/android/binder.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c index bccf30d0fa4f..6ada02805d6a 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -5516,7 +5516,7 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, user_uintptr_t arg thread->looper_need_return = false; wait_event_interruptible(binder_user_error_wait, binder_stop_on_user_error < 2); if (ret && ret != -EINTR) - pr_info("%d:%d ioctl %x %lx returned %d\n", proc->pid, current->pid, cmd, arg, ret); + pr_info("%d:%d ioctl %x %lx returned %d\n", proc->pid, current->pid, cmd, (unsigned long)arg, ret); err_unlocked: trace_binder_ioctl_done(ret); return ret;
round_{up,down} are occasionally used to align user_uintptr_t variables, that is capabilities in PCuABI. The way these macros are currently implemented is ambiguous when capabilities are involved, fortunately CHERI-aware compilers (and upstream Clang since 10.0) implement builtins that are able to align any type, including capabilities. Use these builtins if available.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- include/linux/math.h | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/include/linux/math.h b/include/linux/math.h index 439b8f0b9ebd..1e75ea17f264 100644 --- a/include/linux/math.h +++ b/include/linux/math.h @@ -6,6 +6,10 @@ #include <asm/div64.h> #include <uapi/linux/kernel.h>
+#if __has_builtin(__builtin_align_up) && __has_builtin(__builtin_align_down) +#define round_up(x, y) __builtin_align_up((x), (y)) +#define round_down(x, y) __builtin_align_down((x), (y)) +#else /* * This looks more complex than it should be. But we need to * get the type for the ~ right in round_down (it needs to be @@ -33,6 +37,7 @@ * To perform arbitrary rounding down, use rounddown() below. */ #define round_down(x, y) ((x) & ~__round_mask(x, y)) +#endif /* __has_builtin(__builtin_align_up) && __has_builtin(__builtin_align_down) */
#define DIV_ROUND_UP __KERNEL_DIV_ROUND_UP
instrument_copy_*() take a user pointer as src/dst, but in this case they are actually kernel pointers, as copy_{to,from}_user_page() always work with kernel pointers. We should therefore pass these kernel pointers as (invalid) user pointers, which is fine as memory tools only need them for recording/tracking purposes, not to dereference them.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- include/asm-generic/cacheflush.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/asm-generic/cacheflush.h b/include/asm-generic/cacheflush.h index f46258d1a080..a6617c221e00 100644 --- a/include/asm-generic/cacheflush.h +++ b/include/asm-generic/cacheflush.h @@ -107,7 +107,7 @@ static inline void flush_cache_vunmap(unsigned long start, unsigned long end) #ifndef copy_to_user_page #define copy_to_user_page(vma, page, vaddr, dst, src, len) \ do { \ - instrument_copy_to_user((void __user *)dst, src, len); \ + instrument_copy_to_user(as_user_ptr(dst), src, len); \ memcpy(dst, src, len); \ flush_icache_user_page(vma, page, vaddr, len); \ } while (0) @@ -117,10 +117,10 @@ static inline void flush_cache_vunmap(unsigned long start, unsigned long end) #ifndef copy_from_user_page #define copy_from_user_page(vma, page, vaddr, dst, src, len) \ do { \ - instrument_copy_from_user_before(dst, (void __user *)src, \ + instrument_copy_from_user_before(dst, as_user_ptr(src), \ len); \ memcpy(dst, src, len); \ - instrument_copy_from_user_after(dst, (void __user *)src, len, \ + instrument_copy_from_user_after(dst, as_user_ptr(src), len, \ 0); \ } while (0) #endif
si_call_addr is typed as a user pointer, but it really is just an address. Use as_user_ptr() to store the address as a user pointer, unmodified.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- kernel/signal.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/signal.c b/kernel/signal.c index d4158d626b45..6eb8dddf5946 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -1845,7 +1845,7 @@ int force_sig_seccomp(int syscall, int reason, bool force_coredump) clear_siginfo(&info); info.si_signo = SIGSYS; info.si_code = SYS_SECCOMP; - info.si_call_addr = (void __user *)KSTK_EIP(current); + info.si_call_addr = as_user_ptr(KSTK_EIP(current)); info.si_errno = reason; info.si_arch = syscall_get_arch(current); info.si_syscall = syscall;
send_sig_perf() and variants take a user pointer as first argument, matching the type of si_addr. However si_addr just represents an address, not a valid user pointer. It is therefore appropriate to pass the address as an (invalid) user pointer, using as_user_ptr().
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- kernel/events/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/events/core.c b/kernel/events/core.c index 37e013afb718..c362d35cdf40 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -6480,7 +6480,7 @@ static void perf_sigtrap(struct perf_event *event) if (current->flags & PF_EXITING) return;
- send_sig_perf((void __user *)event->pending_addr, + send_sig_perf(as_user_ptr(event->pending_addr), event->attr.type, event->attr.sig_data); }
On 2/10/23 14:46, Kevin Brodsky wrote:
send_sig_perf() and variants take a user pointer as first argument, matching the type of si_addr. However si_addr just represents an
May be the commit log can be like for better clarity, "However si_addr is just assigned an address here, not ..."
Amit
address, not a valid user pointer. It is therefore appropriate to pass the address as an (invalid) user pointer, using as_user_ptr().
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com
kernel/events/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/events/core.c b/kernel/events/core.c index 37e013afb718..c362d35cdf40 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -6480,7 +6480,7 @@ static void perf_sigtrap(struct perf_event *event) if (current->flags & PF_EXITING) return;
- send_sig_perf((void __user *)event->pending_addr,
- send_sig_perf(as_user_ptr(event->pending_addr), event->attr.type, event->attr.sig_data); }
On 15/02/2023 11:11, Amit Daniel Kachhap wrote:
On 2/10/23 14:46, Kevin Brodsky wrote:
send_sig_perf() and variants take a user pointer as first argument, matching the type of si_addr. However si_addr just represents an
May be the commit log can be like for better clarity, "However si_addr is just assigned an address here, not ..."
I really mean that si_addr represents an address. Its type is user pointer, but it never is a full pointer, we only ever set its address. See also the "Type of si_addr" note in the PCuABI spec [1].
Kevin
[1] https://git.morello-project.org/morello/kernel/linux/-/wikis/Morello-pure-ca...
Amit
address, not a valid user pointer. It is therefore appropriate to pass the address as an (invalid) user pointer, using as_user_ptr().
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com
kernel/events/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/events/core.c b/kernel/events/core.c index 37e013afb718..c362d35cdf40 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -6480,7 +6480,7 @@ static void perf_sigtrap(struct perf_event *event) if (current->flags & PF_EXITING) return; - send_sig_perf((void __user *)event->pending_addr, + send_sig_perf(as_user_ptr(event->pending_addr), event->attr.type, event->attr.sig_data); }
Commit "compat: make compat_get_bitmap work with 64-bit compat longs" made compat_{get,put}_bitmap() compat64-friendly by adding a separate path when compat_long_t is the same size as long.
Even though this condition can be evaluated at compile time and the inactive path eliminated completely, the compiler still parses both paths. As a result, in compat64, GCC complains about the shift in the 32-bit path, because BITS_PER_COMPAT_LONG is 64 in this case and it does not make sense to shift a (64-bit) integer by more than 63.
Avoid this warning by shifting by BITS_PER_LONG / 2 instead, which is equivalent as we already assert above that compat_long_t is half the size of long in this path.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- kernel/compat.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/kernel/compat.c b/kernel/compat.c index c3edaca046f9..01342b3781d6 100644 --- a/kernel/compat.c +++ b/kernel/compat.c @@ -215,7 +215,7 @@ long compat_get_bitmap(unsigned long *mask, const compat_ulong_t __user *umask, compat_ulong_t l1, l2; unsafe_get_user(l1, umask++, Efault); unsafe_get_user(l2, umask++, Efault); - *mask++ = ((unsigned long)l2 << BITS_PER_COMPAT_LONG) | l1; + *mask++ = ((unsigned long)l2 << (BITS_PER_LONG / 2)) | l1; nr_compat_longs -= 2; } if (nr_compat_longs) @@ -252,7 +252,7 @@ long compat_put_bitmap(compat_ulong_t __user *umask, unsigned long *mask, while (nr_compat_longs > 1) { unsigned long m = *mask++; unsafe_put_user((compat_ulong_t)m, umask++, Efault); - unsafe_put_user(m >> BITS_PER_COMPAT_LONG, umask++, Efault); + unsafe_put_user(m >> (BITS_PER_LONG / 2), umask++, Efault); nr_compat_longs -= 2; } if (nr_compat_longs)
audit_log_execve_info() is directly reading memory from a user mapping. The access is entirely driven by the kernel, not by userspace, so a user pointer has to be created manually from the address of the arguments region (mm->arg_start). Using uaddr_to_user_ptr_safe() is the appropriate way to create a user pointer in this case.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- kernel/auditsc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/auditsc.c b/kernel/auditsc.c index 9f8c05228d6d..a136fb44180c 100644 --- a/kernel/auditsc.c +++ b/kernel/auditsc.c @@ -1129,7 +1129,7 @@ static void audit_log_execve_info(struct audit_context *context, unsigned int arg; char *buf_head; char *buf; - const char __user *p = (const char __user *)current->mm->arg_start; + const char __user *p = uaddr_to_user_ptr_safe(current->mm->arg_start);
/* NOTE: this buffer needs to be large enough to hold all the non-arg * data we put in the audit record for this argument (see the
dump_vma_snapshot() is directly reading memory from a user mapping. The access is entirely driven by the kernel, not by userspace, so a user pointer has to be created manually from the computed address. Using uaddr_to_user_ptr_safe() is the appropriate way to create a user pointer in this case.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- fs/coredump.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/coredump.c b/fs/coredump.c index 7bad7785e8e6..90009027a8af 100644 --- a/fs/coredump.c +++ b/fs/coredump.c @@ -1182,7 +1182,7 @@ static bool dump_vma_snapshot(struct coredump_params *cprm) if (m->dump_size == DUMP_SIZE_MAYBE_ELFHDR_PLACEHOLDER) { char elfmag[SELFMAG];
- if (copy_from_user(elfmag, (void __user *)m->start, SELFMAG) || + if (copy_from_user(elfmag, uaddr_to_user_ptr_safe(m->start), SELFMAG) || memcmp(elfmag, ELFMAG, SELFMAG) != 0) { m->dump_size = 0; } else {
struct futex_waitv::uaddr is typed as __u64, although it represents a user pointer. For now let's create a user pointer manually using uaddr_to_user_ptr(), but eventually the struct should be modified to represent uaddr as __kernel_uintptr_t instead, allowing userspace to provide a whole user pointer instead of just an address.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- kernel/futex/waitwake.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/futex/waitwake.c b/kernel/futex/waitwake.c index ba01b9408203..1ab5640e7f84 100644 --- a/kernel/futex/waitwake.c +++ b/kernel/futex/waitwake.c @@ -433,7 +433,7 @@ static int futex_wait_multiple_setup(struct futex_vector *vs, int count, int *wo set_current_state(TASK_INTERRUPTIBLE|TASK_FREEZABLE);
for (i = 0; i < count; i++) { - u32 __user *uaddr = (u32 __user *)(unsigned long)vs[i].w.uaddr; + u32 __user *uaddr = uaddr_to_user_ptr(vs[i].w.uaddr); struct futex_q *q = &vs[i].q; u32 val = (u32)vs[i].w.val;
rseq structs represent all addresses as __u64. Some of them are in fact being used as pointers to access user memory. For now, let's create user pointers manually to make the access.
struct rseq::rseq_cs really represents a pointer (to a struct rseq_c) and its type should eventually be changed accordingly (__kernel_uintptr_t). The situation is less clear for struct rseq_cs::start_ip and struct rseq_cs::abort_ip as they are mostly addresses, but checks in PCuABI may still be desirable.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- kernel/rseq.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/kernel/rseq.c b/kernel/rseq.c index 9eebc6090c52..0afb44439a8d 100644 --- a/kernel/rseq.c +++ b/kernel/rseq.c @@ -141,7 +141,7 @@ static int rseq_get_rseq_cs(struct task_struct *t, struct rseq_cs *rseq_cs) } if (ptr >= TASK_SIZE) return -EINVAL; - urseq_cs = (struct rseq_cs __user *)(unsigned long)ptr; + urseq_cs = uaddr_to_user_ptr(ptr); if (copy_from_user(rseq_cs, urseq_cs, sizeof(*rseq_cs))) return -EFAULT;
@@ -157,7 +157,7 @@ static int rseq_get_rseq_cs(struct task_struct *t, struct rseq_cs *rseq_cs) if (rseq_cs->abort_ip - rseq_cs->start_ip < rseq_cs->post_commit_offset) return -EINVAL;
- usig = (u32 __user *)(unsigned long)(rseq_cs->abort_ip - sizeof(u32)); + usig = uaddr_to_user_ptr(rseq_cs->abort_ip - sizeof(u32)); ret = get_user(sig, usig); if (ret) return ret;
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 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; 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;
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);
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
On 14/02/2023 13:28, Tudor Cretu wrote:
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.
Good to know, thanks Tudor !
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
On 14/02/2023 13: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 !
Thanks for the review!
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
Good catch!
Could it be better with comas around the "however", or maybe moved at the start of the sentence ? ("However, it did miss [...]")
I think it's fairly common to use "however" this way (after the verb and without commas), especially in a short clause like this one.
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 :)
Tudor pointed out the change to checkpatch.pl, a bit more context is that there has never been a hard limit at 80 char, it's more of a guideline. In that case the line is already longer than 80 char, and remains within 100 with this diff (without impacting readability), so I think it's not an issue. Some subsystems are fairly strict with the 80 char limit, but this one clearly isn't :)
Kevin
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;
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> 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 ++--
Looking for additional places this might need updating, possibly worth looking at the following:
1. net/socket.c:____sys_recvmsg - a lot going on here, but I *think* it's fine? some weird pointer math
2. net/ipv6/ipv6_sockglue.c:do_ipv6_setsockopt - it's been updated for do_ipv6_getsockopt but not do_ipv6_setsockopt? - sockptr_t optval.is_kernel contains user/kernel status, looks like something similar to getsockopt is required
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; 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);
On 14/02/2023 13:16, Zachary Leaf wrote:
Looking for additional places this might need updating, possibly worth looking at the following:
- net/socket.c:____sys_recvmsg
- a lot going on here, but I *think* it's fine? some weird pointer math
I did notice this one too, but indeed it's fine as it's only subtracting addresses to calculate some size.
- net/ipv6/ipv6_sockglue.c:do_ipv6_setsockopt
- it's been updated for do_ipv6_getsockopt but not do_ipv6_setsockopt?
- sockptr_t optval.is_kernel contains user/kernel status, looks like
something similar to getsockopt is required
Agreed there's something fishy here, I didn't look at it closely enough. It does look like we need at least to initialise msg_control_is_user. Will give it some more thoughts.
Thanks for looking at all of the uses of msg_control again, I thought I had gone through them all but clearly not!
Kevin
On 14/02/2023 18:03, Kevin Brodsky wrote:
On 14/02/2023 13:16, Zachary Leaf wrote:
Looking for additional places this might need updating, possibly worth looking at the following:
- net/socket.c:____sys_recvmsg
- a lot going on here, but I *think* it's fine? some weird pointer math
I did notice this one too, but indeed it's fine as it's only subtracting addresses to calculate some size.
Right, looks fine to me this morning on a second look.
unsigned long cmsg_ptr = (unsigned long)msg_sys->msg_control; [...] sock_recvmsg(msg_sys) [...] __put_user((unsigned long)msg_sys->msg_control - cmsg_ptr, &msg_compat->msg_controllen);
sock_recvmsg() must update the ptr to msg_control by the size of the message, which makes sense in a socket/network context of filling up a buffer with multiple recv's, but otherwise seemed strange yesterday.
- net/ipv6/ipv6_sockglue.c:do_ipv6_setsockopt
- it's been updated for do_ipv6_getsockopt but not do_ipv6_setsockopt?
- sockptr_t optval.is_kernel contains user/kernel status, looks like
something similar to getsockopt is required
Agreed there's something fishy here, I didn't look at it closely enough. It does look like we need at least to initialise msg_control_is_user. Will give it some more thoughts.
Thanks for looking at all of the uses of msg_control again, I thought I had gone through them all but clearly not!
No problem - I didn't find anything else while going through.
Had a scan over the other patches as well so +1 on the series.
Thanks, Zach
Kevin
On 2/10/23 14:46, 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 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
Looks like even tcp_zc_finalize_rx_tstamp() is like above cmsghdr_from_user_compat_to_kern(Hides user pointer and then creates kernel pointer from it).
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 patch looks like a nice clean up candidate for mainline.
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; 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);
On 15/02/2023 14:35, Amit Daniel Kachhap wrote:
Additionally, a special situation in cmsghdr_from_user_compat_to_kern() is addressed. There the input
Looks like even tcp_zc_finalize_rx_tstamp() is like above cmsghdr_from_user_compat_to_kern(Hides user pointer and then creates kernel pointer from it).
This isn't really the same situation. In the zerocopy function, cmsg_dummy only ever holds a user pointer, and it was mistakenly being stored in msg_control instead of msg_control_user. cmsghdr_from_user_compat_to_kern() is special because on entry, msg_control_user is active (a user pointer is stored), while on exit msg_control is active (a kernel pointer is stored).
There is a separate issue with struct tcp_zerocopy_receive::msg_control being represented as unsigned long even though it always represents a user pointer, that's addressed in the next patch (and it is not something we're trying to upstream at this point).
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 patch looks like a nice clean up candidate for mainline.
I think so too, that's the intention behind the way it's written :)
Kevin
Multiple fields of struct tcp_zerocopy_receive (address, copybuf_address, msg_control) are typed as __u64 even though they represent user pointers. For now let's create user pointers manually using uaddr_to_user_ptr(), but eventually the struct should be modified to represent the fields as __kernel_uintptr_t instead, allowing userspace to provide whole user pointers instead of just addresses.
Also address a user pointer conversion issue where we need to explicitly extract the address of struct msghdr::msg_control_user to store it in struct tcp_zerocopy_receive::msg_control (as it currently only represents an address).
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- net/ipv4/tcp.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c index 8f5e605ea673..f5b69c347846 100644 --- a/net/ipv4/tcp.c +++ b/net/ipv4/tcp.c @@ -2000,7 +2000,7 @@ static int receive_fallback_to_copy(struct sock *sk, if (copy_address != zc->copybuf_address) return -EINVAL;
- err = import_single_range(READ, (void __user *)copy_address, + err = import_single_range(READ, uaddr_to_user_ptr(copy_address), inq, &iov, &msg.msg_iter); if (err) return err; @@ -2034,7 +2034,7 @@ static int tcp_copy_straggler_data(struct tcp_zerocopy_receive *zc, if (copy_address != zc->copybuf_address) return -EINVAL;
- err = import_single_range(READ, (void __user *)copy_address, + err = import_single_range(READ, uaddr_to_user_ptr(copy_address), copylen, &iov, &msg.msg_iter); if (err) return err; @@ -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_user = (void __user *)msg_control_addr; + cmsg_dummy.msg_control_user = uaddr_to_user_ptr(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_user); + user_ptr_addr(cmsg_dummy.msg_control_user); zc->msg_controllen = (__u64)cmsg_dummy.msg_controllen; zc->msg_flags = (__u32)cmsg_dummy.msg_flags;
struct input_mask::codes_ptr is typed as __u64, although it represents a user pointer. For now let's create a user pointer manually using uaddr_to_user_ptr(), but eventually the struct should be modified to represent uaddr as __kernel_uintptr_t instead, allowing userspace to provide a whole user pointer instead of just an address.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- drivers/input/evdev.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c index 066dd1d8cfe4..9abb58dc57b0 100644 --- a/drivers/input/evdev.c +++ b/drivers/input/evdev.c @@ -1101,7 +1101,7 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd, if (copy_from_user(&mask, p, sizeof(mask))) return -EFAULT;
- codes_ptr = (void __user *)(unsigned long)mask.codes_ptr; + codes_ptr = uaddr_to_user_ptr(mask.codes_ptr); return evdev_get_mask(client, mask.type, codes_ptr, mask.codes_size, compat_mode); @@ -1113,7 +1113,7 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd, if (copy_from_user(&mask, p, sizeof(mask))) return -EFAULT;
- codes_ptr = (const void __user *)(unsigned long)mask.codes_ptr; + codes_ptr = uaddr_to_user_ptr(mask.codes_ptr); return evdev_set_mask(client, mask.type, codes_ptr, mask.codes_size, compat_mode);
struct mmc_ioc_cmd::data_ptr is typed as __u64, although it represents a user pointer. For now let's create a user pointer manually using uaddr_to_user_ptr(), but eventually the struct should be modified to represent data_ptr as __kernel_uintptr_t instead, allowing userspace to provide a whole user pointer instead of just an address.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- drivers/mmc/core/block.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index 4a5d0e0283f9..8a05493f3691 100644 --- a/drivers/mmc/core/block.c +++ b/drivers/mmc/core/block.c @@ -430,8 +430,8 @@ static struct mmc_blk_ioc_data *mmc_blk_ioctl_copy_from_user( return idata; }
- idata->buf = memdup_user((void __user *)(unsigned long) - idata->ic.data_ptr, idata->buf_bytes); + idata->buf = memdup_user(uaddr_to_user_ptr(idata->ic.data_ptr), + idata->buf_bytes); if (IS_ERR(idata->buf)) { err = PTR_ERR(idata->buf); goto idata_err; @@ -455,7 +455,7 @@ static int mmc_blk_ioctl_copy_to_user(struct mmc_ioc_cmd __user *ic_ptr, return -EFAULT;
if (!idata->ic.write_flag) { - if (copy_to_user((void __user *)(unsigned long)ic->data_ptr, + if (copy_to_user(uaddr_to_user_ptr(ic->data_ptr), idata->buf, idata->buf_bytes)) return -EFAULT; }
Various DRM uapi structs represent user pointers as __u64, which is not PCuABI-friendly. For now let's create a user pointer manually using uaddr_to_user_ptr(), but eventually the structs should be modified to represent the user pointer fields as __kernel_uintptr_t instead, allowing userspace to provide a whole user pointer instead of just an address.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- drivers/gpu/drm/drm_atomic_uapi.c | 8 ++++---- drivers/gpu/drm/drm_color_mgmt.c | 12 ++++++------ drivers/gpu/drm/drm_connector.c | 8 ++++---- drivers/gpu/drm/drm_crtc.c | 2 +- drivers/gpu/drm/drm_framebuffer.c | 2 +- drivers/gpu/drm/drm_lease.c | 4 ++-- drivers/gpu/drm/drm_mode_object.c | 4 ++-- drivers/gpu/drm/drm_plane.c | 2 +- 8 files changed, 21 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index 79730fa1dd8e..772540313d48 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -1282,10 +1282,10 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { struct drm_mode_atomic *arg = data; - uint32_t __user *objs_ptr = (uint32_t __user *)(unsigned long)(arg->objs_ptr); - uint32_t __user *count_props_ptr = (uint32_t __user *)(unsigned long)(arg->count_props_ptr); - uint32_t __user *props_ptr = (uint32_t __user *)(unsigned long)(arg->props_ptr); - uint64_t __user *prop_values_ptr = (uint64_t __user *)(unsigned long)(arg->prop_values_ptr); + uint32_t __user *objs_ptr = uaddr_to_user_ptr(arg->objs_ptr); + uint32_t __user *count_props_ptr = uaddr_to_user_ptr(arg->count_props_ptr); + uint32_t __user *props_ptr = uaddr_to_user_ptr(arg->props_ptr); + uint64_t __user *prop_values_ptr = uaddr_to_user_ptr(arg->prop_values_ptr); unsigned int copied_objs, copied_props; struct drm_atomic_state *state; struct drm_modeset_acquire_ctx ctx; diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c index d021497841b8..eb065ba37abd 100644 --- a/drivers/gpu/drm/drm_color_mgmt.c +++ b/drivers/gpu/drm/drm_color_mgmt.c @@ -384,19 +384,19 @@ int drm_mode_gamma_set_ioctl(struct drm_device *dev,
size = crtc_lut->gamma_size * (sizeof(uint16_t)); r_base = crtc->gamma_store; - if (copy_from_user(r_base, (void __user *)(unsigned long)crtc_lut->red, size)) { + if (copy_from_user(r_base, uaddr_to_user_ptr(crtc_lut->red), size)) { ret = -EFAULT; goto out; }
g_base = r_base + size; - if (copy_from_user(g_base, (void __user *)(unsigned long)crtc_lut->green, size)) { + if (copy_from_user(g_base, uaddr_to_user_ptr(crtc_lut->green), size)) { ret = -EFAULT; goto out; }
b_base = g_base + size; - if (copy_from_user(b_base, (void __user *)(unsigned long)crtc_lut->blue, size)) { + if (copy_from_user(b_base, uaddr_to_user_ptr(crtc_lut->blue), size)) { ret = -EFAULT; goto out; } @@ -448,19 +448,19 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev, drm_modeset_lock(&crtc->mutex, NULL); size = crtc_lut->gamma_size * (sizeof(uint16_t)); r_base = crtc->gamma_store; - if (copy_to_user((void __user *)(unsigned long)crtc_lut->red, r_base, size)) { + if (copy_to_user(uaddr_to_user_ptr(crtc_lut->red), r_base, size)) { ret = -EFAULT; goto out; }
g_base = r_base + size; - if (copy_to_user((void __user *)(unsigned long)crtc_lut->green, g_base, size)) { + if (copy_to_user(uaddr_to_user_ptr(crtc_lut->green), g_base, size)) { ret = -EFAULT; goto out; }
b_base = g_base + size; - if (copy_to_user((void __user *)(unsigned long)crtc_lut->blue, b_base, size)) { + if (copy_to_user(uaddr_to_user_ptr(crtc_lut->blue), b_base, size)) { ret = -EFAULT; goto out; } diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index 61c29ce74b03..3fce01825728 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -2679,7 +2679,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
if ((out_resp->count_encoders >= encoders_count) && encoders_count) { copied = 0; - encoder_ptr = (uint32_t __user *)(unsigned long)(out_resp->encoders_ptr); + encoder_ptr = uaddr_to_user_ptr(out_resp->encoders_ptr);
drm_connector_for_each_possible_encoder(connector, encoder) { if (put_user(encoder->base.id, encoder_ptr + copied)) { @@ -2730,7 +2730,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, */ if ((out_resp->count_modes >= mode_count) && mode_count) { copied = 0; - mode_ptr = (struct drm_mode_modeinfo __user *)(unsigned long)out_resp->modes_ptr; + mode_ptr = uaddr_to_user_ptr(out_resp->modes_ptr); list_for_each_entry(mode, &connector->modes, head) { if (!mode->expose_to_userspace) continue; @@ -2782,8 +2782,8 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, * properties reflect the latest status. */ ret = drm_mode_object_get_properties(&connector->base, file_priv->atomic, - (uint32_t __user *)(unsigned long)(out_resp->props_ptr), - (uint64_t __user *)(unsigned long)(out_resp->prop_values_ptr), + uaddr_to_user_ptr(out_resp->props_ptr), + uaddr_to_user_ptr(out_resp->prop_values_ptr), &out_resp->count_props); drm_modeset_unlock(&dev->mode_config.connection_mutex);
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index df9bf3c9206e..2d598d52ee70 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -853,7 +853,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
for (i = 0; i < crtc_req->count_connectors; i++) { connector_set[i] = NULL; - set_connectors_ptr = (uint32_t __user *)(unsigned long)crtc_req->set_connectors_ptr; + set_connectors_ptr = uaddr_to_user_ptr(crtc_req->set_connectors_ptr); if (get_user(out_id, &set_connectors_ptr[i])) { ret = -EFAULT; goto out; diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c index 2dd97473ca10..394896984d47 100644 --- a/drivers/gpu/drm/drm_framebuffer.c +++ b/drivers/gpu/drm/drm_framebuffer.c @@ -711,7 +711,7 @@ int drm_mode_dirtyfb_ioctl(struct drm_device *dev, return -ENOENT;
num_clips = r->num_clips; - clips_ptr = (struct drm_clip_rect __user *)(unsigned long)r->clips_ptr; + clips_ptr = uaddr_to_user_ptr(r->clips_ptr);
if (!num_clips != !clips_ptr) { ret = -EINVAL; diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c index d72c2fac0ff1..bc9b6cfab2b8 100644 --- a/drivers/gpu/drm/drm_lease.c +++ b/drivers/gpu/drm/drm_lease.c @@ -587,7 +587,7 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev, void *data, struct drm_file *lessor_priv) { struct drm_mode_list_lessees *arg = data; - __u32 __user *lessee_ids = (__u32 __user *) (uintptr_t) (arg->lessees_ptr); + __u32 __user *lessee_ids = uaddr_to_user_ptr(arg->lessees_ptr); __u32 count_lessees = arg->count_lessees; struct drm_master *lessor, *lessee; int count; @@ -634,7 +634,7 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev, void *data, struct drm_file *lessee_priv) { struct drm_mode_get_lease *arg = data; - __u32 __user *object_ids = (__u32 __user *) (uintptr_t) (arg->objects_ptr); + __u32 __user *object_ids = uaddr_to_user_ptr(arg->objects_ptr); __u32 count_objects = arg->count_objects; struct drm_master *lessee; struct idr *object_idr; diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c index ba1608effc0f..041903363a3d 100644 --- a/drivers/gpu/drm/drm_mode_object.c +++ b/drivers/gpu/drm/drm_mode_object.c @@ -454,8 +454,8 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data, }
ret = drm_mode_object_get_properties(obj, file_priv->atomic, - (uint32_t __user *)(unsigned long)(arg->props_ptr), - (uint64_t __user *)(unsigned long)(arg->prop_values_ptr), + uaddr_to_user_ptr(arg->props_ptr), + uaddr_to_user_ptr(arg->prop_values_ptr), &arg->count_props);
out_unref: diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index 33357629a7f5..b32530db64a0 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -727,7 +727,7 @@ int drm_mode_getplane(struct drm_device *dev, void *data, */ if (plane->format_count && (plane_resp->count_format_types >= plane->format_count)) { - format_ptr = (uint32_t __user *)(unsigned long)plane_resp->format_type_ptr; + format_ptr = uaddr_to_user_ptr(plane_resp->format_type_ptr); if (copy_to_user(format_ptr, plane->format_types, sizeof(uint32_t) * plane->format_count)) {
Various Panfrost uapi structs represent user pointers as __u64, which is not PCuABI-friendly. For now let's create a user pointer manually using uaddr_to_user_ptr(), but eventually the structs should be modified to represent the user pointer fields as __kernel_uintptr_t instead, allowing userspace to provide a whole user pointer instead of just an address.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- drivers/gpu/drm/panfrost/panfrost_drv.c | 4 ++-- drivers/gpu/drm/panfrost/panfrost_perfcnt.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c index 2fa5afe21288..9f79b024a840 100644 --- a/drivers/gpu/drm/panfrost/panfrost_drv.c +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c @@ -139,7 +139,7 @@ panfrost_lookup_bos(struct drm_device *dev, return 0;
ret = drm_gem_objects_lookup(file_priv, - (void __user *)(uintptr_t)args->bo_handles, + uaddr_to_user_ptr(args->bo_handles), job->bo_count, &job->bos); if (ret) return ret; @@ -203,7 +203,7 @@ panfrost_copy_in_sync(struct drm_device *dev, }
if (copy_from_user(handles, - (void __user *)(uintptr_t)args->in_syncs, + uaddr_to_user_ptr(args->in_syncs), in_fence_count * sizeof(u32))) { ret = -EFAULT; DRM_DEBUG("Failed to copy in syncobj handles\n"); diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c index bc0df93f7f21..d917c8733813 100644 --- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c +++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c @@ -240,7 +240,7 @@ int panfrost_ioctl_perfcnt_dump(struct drm_device *dev, void *data, struct panfrost_device *pfdev = dev->dev_private; struct panfrost_perfcnt *perfcnt = pfdev->perfcnt; struct drm_panfrost_perfcnt_dump *req = data; - void __user *user_ptr = (void __user *)(uintptr_t)req->buf_ptr; + void __user *user_ptr = uaddr_to_user_ptr(req->buf_ptr); int ret;
ret = panfrost_unstable_ioctl_check();
On 10/02/2023 10:16, Kevin Brodsky wrote:
Hi,
Following up on Luca's series addressing ioctl-related GCC warnings, this series takes care of the remaining warnings reported by GCC (but not Clang). The patches are all straightforward, except patch 13 (see below).
Some notes:
Patch 1-3 fix up small oversights in our own commits (see Fixes: tags).
Patch 9-10 are about creating user pointers in cases where it is unavoidable, such cases are not concerning.
Patch 11-12 and 14-18 are about creating user pointers because of uapi deficiencies (i.e. user pointers represented as __u64 in structs). They should therefore be viewed as a stopgap while waiting for a full solution (uapi change and compat handling); this is tracked by the following tickets:
- Patch 11: https://git.morello-project.org/morello/kernel/linux/-/issues/48
- Patch 12: https://git.morello-project.org/morello/kernel/linux/-/issues/47
- Patch 14: https://git.morello-project.org/morello/kernel/linux/-/issues/46
- Patch 15: https://git.morello-project.org/morello/kernel/linux/-/issues/50
- Patch 16: https://git.morello-project.org/morello/kernel/linux/-/issues/51
- Patch 17-18: https://git.morello-project.org/morello/kernel/linux/-/issues/52
Patch 13 is upstreamable and is written in that spirit. Fixing up the msg_control/msg_control_user split required grepping all uses, as only a few misuses actually lead to warnings in PCuABI. Note that as an exceptional bending of the rules, this patch introduces a warning (both with Clang and GCC), which is impossible to avoid in a patch for upstream. The next patch (14) fixes the warnings so it should not be an issue in practice.
With these patches and Luca's, only a handful of genuine GCC warnings are left:
- A few in fs/aio.c, tracked by https://git.morello-project.org/morello/kernel/linux/-/issues/49
- A few under kernel/bpf/, should be addressed by https://git.morello-project.org/morello/kernel/linux/-/issues/4
Other warnings are Morello GCC defects and have been reported.
Sorry, forgot to mention the review branch:
https://git.morello-project.org/kbrodsky-arm/linux/-/commits/morello/gcc_fix...
Kevin
Thanks, Kevin
Kevin Brodsky (18): arm64: signal: Avoid unnecessary capability arithmetic drivers/android/binder: Cast to (void *) when printing %p drivers/android/binder: Cast user_uintptr_t arg when printing math.h: Make round_{up,down} capability-friendly asm-generic/cacheflush.h: Use appropriate user pointer conversion kernel: signal: Use appropriate user pointer conversion perf/core: Use appropriate user pointer conversion compat: Fix shift warning in compat64 audit: Explicitly create user pointer coredump: Explicitly create user pointer kernel/futex: Explicitly create user pointer rseq: Explicitly create user pointers net: Finish up ->msg_control{,_user} split tcp: Explicitly create user pointers Input: evdev: Explicitly create user pointers mmc: block: Explicitly create user pointers drm: Explicitly create user pointers drm/panfrost: Explicitly create user pointers
arch/arm64/kernel/signal.c | 6 +++-- drivers/android/binder.c | 4 ++-- drivers/android/binder_alloc.c | 25 +++++++++++---------- drivers/gpu/drm/drm_atomic_uapi.c | 8 +++---- drivers/gpu/drm/drm_color_mgmt.c | 12 +++++----- drivers/gpu/drm/drm_connector.c | 8 +++---- drivers/gpu/drm/drm_crtc.c | 2 +- drivers/gpu/drm/drm_framebuffer.c | 2 +- drivers/gpu/drm/drm_lease.c | 4 ++-- drivers/gpu/drm/drm_mode_object.c | 4 ++-- drivers/gpu/drm/drm_plane.c | 2 +- drivers/gpu/drm/panfrost/panfrost_drv.c | 4 ++-- drivers/gpu/drm/panfrost/panfrost_perfcnt.c | 2 +- drivers/input/evdev.c | 4 ++-- drivers/mmc/core/block.c | 6 ++--- fs/coredump.c | 2 +- include/asm-generic/cacheflush.h | 6 ++--- include/linux/math.h | 5 +++++ kernel/auditsc.c | 2 +- kernel/compat.c | 4 ++-- kernel/events/core.c | 2 +- kernel/futex/waitwake.c | 2 +- kernel/rseq.c | 4 ++-- kernel/signal.c | 2 +- net/compat.c | 13 ++++++----- net/core/scm.c | 9 +++++--- net/ipv4/tcp.c | 8 +++---- 27 files changed, 82 insertions(+), 70 deletions(-)
On 10-02-2023 09:16, Kevin Brodsky wrote:
Hi,
Following up on Luca's series addressing ioctl-related GCC warnings, this series takes care of the remaining warnings reported by GCC (but not Clang). The patches are all straightforward, except patch 13 (see below).
Some notes:
Patch 1-3 fix up small oversights in our own commits (see Fixes: tags).
Patch 9-10 are about creating user pointers in cases where it is unavoidable, such cases are not concerning.
Patch 11-12 and 14-18 are about creating user pointers because of uapi deficiencies (i.e. user pointers represented as __u64 in structs). They should therefore be viewed as a stopgap while waiting for a full solution (uapi change and compat handling); this is tracked by the following tickets:
- Patch 11: https://git.morello-project.org/morello/kernel/linux/-/issues/48
- Patch 12: https://git.morello-project.org/morello/kernel/linux/-/issues/47
- Patch 14: https://git.morello-project.org/morello/kernel/linux/-/issues/46
- Patch 15: https://git.morello-project.org/morello/kernel/linux/-/issues/50
- Patch 16: https://git.morello-project.org/morello/kernel/linux/-/issues/51
- Patch 17-18: https://git.morello-project.org/morello/kernel/linux/-/issues/52
Patch 13 is upstreamable and is written in that spirit. Fixing up the msg_control/msg_control_user split required grepping all uses, as only a few misuses actually lead to warnings in PCuABI. Note that as an exceptional bending of the rules, this patch introduces a warning (both with Clang and GCC), which is impossible to avoid in a patch for upstream. The next patch (14) fixes the warnings so it should not be an issue in practice.
Well done for this, happy to see our team has opportunities to upstream!
With these patches and Luca's, only a handful of genuine GCC warnings are left:
- A few in fs/aio.c, tracked by https://git.morello-project.org/morello/kernel/linux/-/issues/49
- A few under kernel/bpf/, should be addressed by https://git.morello-project.org/morello/kernel/linux/-/issues/4
Other warnings are Morello GCC defects and have been reported.
One minor question in Patch 1, otherwise, everything looks good to me!
Thanks, Tudor
Thanks, Kevin
Kevin Brodsky (18): arm64: signal: Avoid unnecessary capability arithmetic drivers/android/binder: Cast to (void *) when printing %p drivers/android/binder: Cast user_uintptr_t arg when printing math.h: Make round_{up,down} capability-friendly asm-generic/cacheflush.h: Use appropriate user pointer conversion kernel: signal: Use appropriate user pointer conversion perf/core: Use appropriate user pointer conversion compat: Fix shift warning in compat64 audit: Explicitly create user pointer coredump: Explicitly create user pointer kernel/futex: Explicitly create user pointer rseq: Explicitly create user pointers net: Finish up ->msg_control{,_user} split tcp: Explicitly create user pointers Input: evdev: Explicitly create user pointers mmc: block: Explicitly create user pointers drm: Explicitly create user pointers drm/panfrost: Explicitly create user pointers
arch/arm64/kernel/signal.c | 6 +++-- drivers/android/binder.c | 4 ++-- drivers/android/binder_alloc.c | 25 +++++++++++---------- drivers/gpu/drm/drm_atomic_uapi.c | 8 +++---- drivers/gpu/drm/drm_color_mgmt.c | 12 +++++----- drivers/gpu/drm/drm_connector.c | 8 +++---- drivers/gpu/drm/drm_crtc.c | 2 +- drivers/gpu/drm/drm_framebuffer.c | 2 +- drivers/gpu/drm/drm_lease.c | 4 ++-- drivers/gpu/drm/drm_mode_object.c | 4 ++-- drivers/gpu/drm/drm_plane.c | 2 +- drivers/gpu/drm/panfrost/panfrost_drv.c | 4 ++-- drivers/gpu/drm/panfrost/panfrost_perfcnt.c | 2 +- drivers/input/evdev.c | 4 ++-- drivers/mmc/core/block.c | 6 ++--- fs/coredump.c | 2 +- include/asm-generic/cacheflush.h | 6 ++--- include/linux/math.h | 5 +++++ kernel/auditsc.c | 2 +- kernel/compat.c | 4 ++-- kernel/events/core.c | 2 +- kernel/futex/waitwake.c | 2 +- kernel/rseq.c | 4 ++-- kernel/signal.c | 2 +- net/compat.c | 13 ++++++----- net/core/scm.c | 9 +++++--- net/ipv4/tcp.c | 8 +++---- 27 files changed, 82 insertions(+), 70 deletions(-)
I had a scan over this - nothing jumped out to me as bad. The drm changes are things i was expecting to turn up once we actually start beating gfx in anger in purecap, so unsurprising there.
so +1 from me I guess as nothing jumps out to me to grumble much at. :)
Nice work!
On 2/10/23 09:16, Kevin Brodsky wrote:
Hi,
Following up on Luca's series addressing ioctl-related GCC warnings, this series takes care of the remaining warnings reported by GCC (but not Clang). The patches are all straightforward, except patch 13 (see below).
Some notes:
Patch 1-3 fix up small oversights in our own commits (see Fixes: tags).
Patch 9-10 are about creating user pointers in cases where it is unavoidable, such cases are not concerning.
Patch 11-12 and 14-18 are about creating user pointers because of uapi deficiencies (i.e. user pointers represented as __u64 in structs). They should therefore be viewed as a stopgap while waiting for a full solution (uapi change and compat handling); this is tracked by the following tickets:
- Patch 11: https://git.morello-project.org/morello/kernel/linux/-/issues/48
- Patch 12: https://git.morello-project.org/morello/kernel/linux/-/issues/47
- Patch 14: https://git.morello-project.org/morello/kernel/linux/-/issues/46
- Patch 15: https://git.morello-project.org/morello/kernel/linux/-/issues/50
- Patch 16: https://git.morello-project.org/morello/kernel/linux/-/issues/51
- Patch 17-18: https://git.morello-project.org/morello/kernel/linux/-/issues/52
Patch 13 is upstreamable and is written in that spirit. Fixing up the msg_control/msg_control_user split required grepping all uses, as only a few misuses actually lead to warnings in PCuABI. Note that as an exceptional bending of the rules, this patch introduces a warning (both with Clang and GCC), which is impossible to avoid in a patch for upstream. The next patch (14) fixes the warnings so it should not be an issue in practice.
With these patches and Luca's, only a handful of genuine GCC warnings are left:
- A few in fs/aio.c, tracked by https://git.morello-project.org/morello/kernel/linux/-/issues/49
- A few under kernel/bpf/, should be addressed by https://git.morello-project.org/morello/kernel/linux/-/issues/4
Other warnings are Morello GCC defects and have been reported.
Thanks, Kevin
Kevin Brodsky (18): arm64: signal: Avoid unnecessary capability arithmetic drivers/android/binder: Cast to (void *) when printing %p drivers/android/binder: Cast user_uintptr_t arg when printing math.h: Make round_{up,down} capability-friendly asm-generic/cacheflush.h: Use appropriate user pointer conversion kernel: signal: Use appropriate user pointer conversion perf/core: Use appropriate user pointer conversion compat: Fix shift warning in compat64 audit: Explicitly create user pointer coredump: Explicitly create user pointer kernel/futex: Explicitly create user pointer rseq: Explicitly create user pointers net: Finish up ->msg_control{,_user} split tcp: Explicitly create user pointers Input: evdev: Explicitly create user pointers mmc: block: Explicitly create user pointers drm: Explicitly create user pointers drm/panfrost: Explicitly create user pointers
arch/arm64/kernel/signal.c | 6 +++-- drivers/android/binder.c | 4 ++-- drivers/android/binder_alloc.c | 25 +++++++++++---------- drivers/gpu/drm/drm_atomic_uapi.c | 8 +++---- drivers/gpu/drm/drm_color_mgmt.c | 12 +++++----- drivers/gpu/drm/drm_connector.c | 8 +++---- drivers/gpu/drm/drm_crtc.c | 2 +- drivers/gpu/drm/drm_framebuffer.c | 2 +- drivers/gpu/drm/drm_lease.c | 4 ++-- drivers/gpu/drm/drm_mode_object.c | 4 ++-- drivers/gpu/drm/drm_plane.c | 2 +- drivers/gpu/drm/panfrost/panfrost_drv.c | 4 ++-- drivers/gpu/drm/panfrost/panfrost_perfcnt.c | 2 +- drivers/input/evdev.c | 4 ++-- drivers/mmc/core/block.c | 6 ++--- fs/coredump.c | 2 +- include/asm-generic/cacheflush.h | 6 ++--- include/linux/math.h | 5 +++++ kernel/auditsc.c | 2 +- kernel/compat.c | 4 ++-- kernel/events/core.c | 2 +- kernel/futex/waitwake.c | 2 +- kernel/rseq.c | 4 ++-- kernel/signal.c | 2 +- net/compat.c | 13 ++++++----- net/core/scm.c | 9 +++++--- net/ipv4/tcp.c | 8 +++---- 27 files changed, 82 insertions(+), 70 deletions(-)
On 2/10/23 14:46, Kevin Brodsky wrote:
Hi,
Following up on Luca's series addressing ioctl-related GCC warnings, this series takes care of the remaining warnings reported by GCC (but not Clang). The patches are all straightforward, except patch 13 (see below).
Some notes:
Patch 1-3 fix up small oversights in our own commits (see Fixes: tags).
Patch 9-10 are about creating user pointers in cases where it is unavoidable, such cases are not concerning.
Patch 11-12 and 14-18 are about creating user pointers because of uapi deficiencies (i.e. user pointers represented as __u64 in structs). They should therefore be viewed as a stopgap while waiting for a full solution (uapi change and compat handling); this is tracked by the following tickets:
- Patch 11: https://git.morello-project.org/morello/kernel/linux/-/issues/48
- Patch 12: https://git.morello-project.org/morello/kernel/linux/-/issues/47
- Patch 14: https://git.morello-project.org/morello/kernel/linux/-/issues/46
- Patch 15: https://git.morello-project.org/morello/kernel/linux/-/issues/50
- Patch 16: https://git.morello-project.org/morello/kernel/linux/-/issues/51
- Patch 17-18: https://git.morello-project.org/morello/kernel/linux/-/issues/52
Patch 13 is upstreamable and is written in that spirit. Fixing up the msg_control/msg_control_user split required grepping all uses, as only a few misuses actually lead to warnings in PCuABI. Note that as an exceptional bending of the rules, this patch introduces a warning (both with Clang and GCC), which is impossible to avoid in a patch for upstream. The next patch (14) fixes the warnings so it should not be an issue in practice.
With these patches and Luca's, only a handful of genuine GCC warnings are left:
- A few in fs/aio.c, tracked by https://git.morello-project.org/morello/kernel/linux/-/issues/49
- A few under kernel/bpf/, should be addressed by https://git.morello-project.org/morello/kernel/linux/-/issues/4
Other warnings are Morello GCC defects and have been reported.
I looked at all the patches and with few nits and they look good overall.
Amit
Thanks, Kevin
Kevin Brodsky (18): arm64: signal: Avoid unnecessary capability arithmetic drivers/android/binder: Cast to (void *) when printing %p drivers/android/binder: Cast user_uintptr_t arg when printing math.h: Make round_{up,down} capability-friendly asm-generic/cacheflush.h: Use appropriate user pointer conversion kernel: signal: Use appropriate user pointer conversion perf/core: Use appropriate user pointer conversion compat: Fix shift warning in compat64 audit: Explicitly create user pointer coredump: Explicitly create user pointer kernel/futex: Explicitly create user pointer rseq: Explicitly create user pointers net: Finish up ->msg_control{,_user} split tcp: Explicitly create user pointers Input: evdev: Explicitly create user pointers mmc: block: Explicitly create user pointers drm: Explicitly create user pointers drm/panfrost: Explicitly create user pointers
arch/arm64/kernel/signal.c | 6 +++-- drivers/android/binder.c | 4 ++-- drivers/android/binder_alloc.c | 25 +++++++++++---------- drivers/gpu/drm/drm_atomic_uapi.c | 8 +++---- drivers/gpu/drm/drm_color_mgmt.c | 12 +++++----- drivers/gpu/drm/drm_connector.c | 8 +++---- drivers/gpu/drm/drm_crtc.c | 2 +- drivers/gpu/drm/drm_framebuffer.c | 2 +- drivers/gpu/drm/drm_lease.c | 4 ++-- drivers/gpu/drm/drm_mode_object.c | 4 ++-- drivers/gpu/drm/drm_plane.c | 2 +- drivers/gpu/drm/panfrost/panfrost_drv.c | 4 ++-- drivers/gpu/drm/panfrost/panfrost_perfcnt.c | 2 +- drivers/input/evdev.c | 4 ++-- drivers/mmc/core/block.c | 6 ++--- fs/coredump.c | 2 +- include/asm-generic/cacheflush.h | 6 ++--- include/linux/math.h | 5 +++++ kernel/auditsc.c | 2 +- kernel/compat.c | 4 ++-- kernel/events/core.c | 2 +- kernel/futex/waitwake.c | 2 +- kernel/rseq.c | 4 ++-- kernel/signal.c | 2 +- net/compat.c | 13 ++++++----- net/core/scm.c | 9 +++++--- net/ipv4/tcp.c | 8 +++---- 27 files changed, 82 insertions(+), 70 deletions(-)
linux-morello@op-lists.linaro.org
-
Amit Daniel Kachhap -
Carsten Haitzler -
Kevin Brodsky -
Teo Couprie Diaz -
Tudor Cretu -
Zachary Leaf