Re: Coverity: __do_sys_pidfd_send_signal(): UNINIT
On 02/13, Tycho Andersen wrote:
>
> I think this is a false positive, we have:
Agreed,
> That said, a default case wouldn't hurt, and we should fix the first
> comment anyways, since now we have extensions.
>
> I'm happy to send a patch or maybe it's better for Christian to fix it
> in-tree.
I leave this to you and Christian, whatever you prefer. But perhaps we
can simplify these checks? Something like below.
Oleg.
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -3876,10 +3876,6 @@ static struct pid *pidfd_to_pid(const struct file *file)
return tgid_pidfd_to_pid(file);
}
-#define PIDFD_SEND_SIGNAL_FLAGS\
- (PIDFD_SIGNAL_THREAD | PIDFD_SIGNAL_THREAD_GROUP | \
-PIDFD_SIGNAL_PROCESS_GROUP)
-
/**
* sys_pidfd_send_signal - Signal a process through a pidfd
* @pidfd: file descriptor of the process
@@ -3903,13 +3899,21 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
kernel_siginfo_t kinfo;
enum pid_type type;
- /* Enforce flags be set to 0 until we add an extension. */
- if (flags & ~PIDFD_SEND_SIGNAL_FLAGS)
- return -EINVAL;
-
- /* Ensure that only a single signal scope determining flag is set. */
- if (hweight32(flags & PIDFD_SEND_SIGNAL_FLAGS) > 1)
- return -EINVAL;
+ switch (flags) {
+ case 0:
+ /* but see the PIDFD_THREAD check below */
+ type = PIDTYPE_TGID;
+ break;
+ case PIDFD_SIGNAL_THREAD:
+ type = PIDTYPE_PID;
+ break;
+ case PIDFD_SIGNAL_THREAD_GROUP:
+ type = PIDTYPE_TGID;
+ break;
+ case PIDFD_SIGNAL_PROCESS_GROUP:
+ type = PIDTYPE_PGID;
+ break;
+ }
f = fdget(pidfd);
if (!f.file)
@@ -3926,24 +3930,8 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
if (!access_pidfd_pidns(pid))
goto err;
- switch (flags) {
- case 0:
- /* Infer scope from the type of pidfd. */
- if (f.file->f_flags & PIDFD_THREAD)
- type = PIDTYPE_PID;
- else
- type = PIDTYPE_TGID;
- break;
- case PIDFD_SIGNAL_THREAD:
+ if (!flags && (f.file->f_flags & PIDFD_THREAD))
type = PIDTYPE_PID;
- break;
- case PIDFD_SIGNAL_THREAD_GROUP:
- type = PIDTYPE_TGID;
- break;
- case PIDFD_SIGNAL_PROCESS_GROUP:
- type = PIDTYPE_PGID;
- break;
- }
if (info) {
ret = copy_siginfo_from_user_any(&kinfo, info);
Re: Coverity: __do_sys_pidfd_send_signal(): UNINIT
On 02/14, Oleg Nesterov wrote:
>
> On 02/13, Tycho Andersen wrote:
> >
> > I think this is a false positive, we have:
>
> Agreed,
>
> > That said, a default case wouldn't hurt, and we should fix the first
> > comment anyways, since now we have extensions.
> >
> > I'm happy to send a patch or maybe it's better for Christian to fix it
> > in-tree.
>
> I leave this to you and Christian, whatever you prefer. But perhaps we
> can simplify these checks? Something like below.
forgot about -EINVAL ...
Oleg.
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -3876,10 +3876,6 @@ static struct pid *pidfd_to_pid(const struct file *file)
return tgid_pidfd_to_pid(file);
}
-#define PIDFD_SEND_SIGNAL_FLAGS\
- (PIDFD_SIGNAL_THREAD | PIDFD_SIGNAL_THREAD_GROUP | \
-PIDFD_SIGNAL_PROCESS_GROUP)
-
/**
* sys_pidfd_send_signal - Signal a process through a pidfd
* @pidfd: file descriptor of the process
@@ -3903,13 +3899,23 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
kernel_siginfo_t kinfo;
enum pid_type type;
- /* Enforce flags be set to 0 until we add an extension. */
- if (flags & ~PIDFD_SEND_SIGNAL_FLAGS)
- return -EINVAL;
-
- /* Ensure that only a single signal scope determining flag is set. */
- if (hweight32(flags & PIDFD_SEND_SIGNAL_FLAGS) > 1)
+ switch (flags) {
+ case 0:
+ /* but see the PIDFD_THREAD check below */
+ type = PIDTYPE_TGID;
+ break;
+ case PIDFD_SIGNAL_THREAD:
+ type = PIDTYPE_PID;
+ break;
+ case PIDFD_SIGNAL_THREAD_GROUP:
+ type = PIDTYPE_TGID;
+ break;
+ case PIDFD_SIGNAL_PROCESS_GROUP:
+ type = PIDTYPE_PGID;
+ break;
+ default:
return -EINVAL;
+ }
f = fdget(pidfd);
if (!f.file)
@@ -3926,24 +3932,8 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
if (!access_pidfd_pidns(pid))
goto err;
- switch (flags) {
- case 0:
- /* Infer scope from the type of pidfd. */
- if (f.file->f_flags & PIDFD_THREAD)
- type = PIDTYPE_PID;
- else
- type = PIDTYPE_TGID;
- break;
- case PIDFD_SIGNAL_THREAD:
+ if (!flags && (f.file->f_flags & PIDFD_THREAD))
type = PIDTYPE_PID;
- break;
- case PIDFD_SIGNAL_THREAD_GROUP:
- type = PIDTYPE_TGID;
- break;
- case PIDFD_SIGNAL_PROCESS_GROUP:
- type = PIDTYPE_PGID;
- break;
- }
if (info) {
ret = copy_siginfo_from_user_any(&kinfo, info);
Re: Coverity: __do_sys_pidfd_send_signal(): UNINIT
Hi Tycho,
let me repeat just in case, I am fine either way, whatever you and
Christian prefer. In particular, I agree in advance if you decide
to not change the current code, it is correct even if it can fool
the tools.
That said,
On 02/14, Tycho Andersen wrote:
>
> On Wed, Feb 14, 2024 at 10:06:41AM +0100, Oleg Nesterov wrote:
> >
> > - /* Ensure that only a single signal scope determining flag is set. */
> > - if (hweight32(flags & PIDFD_SEND_SIGNAL_FLAGS) > 1)
> > + switch (flags) {
> > + case 0:
> > + /* but see the PIDFD_THREAD check below */
>
> Why not put that bit inline?
Not sure I understand what does "inline" mean... but let me reply
anyway.
We want to check the "flags" argument at the start, we do not want to
delay the "case 0:" check until we have f.file (so that we can check
f.file->f_flags).
but perhaps this is another case when I misunderstand you.
> But I guess the hweight and flags mask
> are intended to be future proofness for flags that don't fit into this
> switch.
Yes I see, but
> That said, your patch reads better than the way it is in the
> tree and is what I was thinking.
this was my point.
And if we add more flags, we will need to update the "switch" stmt anyway.
But again, I won't insist. This is cosmetic afer all.
Oleg.
Re: Coverity: __do_sys_pidfd_send_signal(): UNINIT
On 02/14, Tycho Andersen wrote: > > On Wed, Feb 14, 2024 at 06:55:55PM +0100, Oleg Nesterov wrote: > > > > We want to check the "flags" argument at the start, we do not want to > > delay the "case 0:" check until we have f.file (so that we can check > > f.file->f_flags). > > Fair point. I was thinking delaying it would make it simpler, but then > you have to free the file and it's less fast in the EINVAL case. plus we do not want to return, say, -EBADF if the "flags" argument is wrong. > I also don't have a strong opinion here. Neither me. Oleg.
Re: [RFC PATCH v1 1/1] exec: seal system mappings
On 10/04, jef...@chromium.org wrote:
>
> It is important to note that the CHECKPOINT_RESTORE feature (CRIU) may
> alter the mapping of vdso, vvar, and sigpage during restore
> operations. Consequently, this feature cannot be universally enabled
> across all systems.
Can't review.
But as for uprobes, I'd prefer a simpler patch which doesn't need the new
CONFIG_ and/or kernel boot options, something like the patch below.
And I don't really like the fact that this patch changes the behaviour
of the "generic" _install_special_mapping() helper, but I won't argue.
Oleg.
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -430,6 +430,8 @@ extern unsigned int kobjsize(const void *objp);
#ifdef CONFIG_64BIT
/* VM is sealed, in vm_flags */
#define VM_SEALED _BITUL(63)
+#else
+#define VM_SEALED 0
#endif
/* Bits set in the VMA until the stack is in its final location */
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 40ecab0971ff..388373c11593 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1510,7 +1510,7 @@ static int xol_add_vma(struct mm_struct *mm, struct
xol_area *area)
}
vma = _install_special_mapping(mm, area->vaddr, PAGE_SIZE,
- VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO,
+ VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO|VM_SEALED,
&xol_mapping);
if (IS_ERR(vma)) {
ret = PTR_ERR(vma);
Re: [RFC PATCH v1 1/1] exec: seal system mappings
Sorry for the noise, forgot to mention...
On 10/04, jef...@chromium.org wrote:
>
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -1535,6 +1535,15 @@
> Permit 'security.evm' to be updated regardless of
> current integrity status.
>
> + exec.seal_system_mappings = [KNL]
> + Format: { never | always }
> + Seal system mappings: vdso, vvar, sigpage, uprobes,
> + vsyscall.
> + This overwrites KCONFIG CONFIG_SEAL_SYSTEM_MAPPINGS_*
> + - 'never': never seal system mappings.
> + - 'always': always seal system mappings.
> + If not specified or invalid, default is the KCONFIG
> value.
perhaps the documentation should also mention that this new parameter has
no effect if CONFIG_64BIT=n.
Oleg.
Re: [RFC PATCH v2 1/1] exec: seal system mappings
On 10/16, Jeff Xu wrote:
>
> On Wed, Oct 16, 2024 at 6:10 PM Liam R. Howlett
> wrote:
> >
> > > + exec.seal_system_mappings = [KNL]
> > > + Format: { never | always }
> > > + Seal system mappings: vdso, vvar, sigpage, uprobes,
> > > + vsyscall.
> > > + This overwrites KCONFIG
> > > CONFIG_SEAL_SYSTEM_MAPPINGS_*
> > > + - 'never': never seal system mappings.
> >
> > Not true, uprobes are sealed when 'never' is selected.
> >
> Thanks. I forgot to uprobes from the description in Kconfig and
> kernel-parameters.txt, will update.
Jeff, I am sorry for confusion.
No need to make uprobes "special" and complicate the logic/documentation.
I just meant that, unlike vdso, it is always safe/good to mseal the "[uprobes]"
vma, regardless of config/boot options.
Please do what you think is right, I am fine either way.
Oleg.
Re: [PATCH v8 5/7] mseal sysmap: uprobe mapping
On 03/03, jef...@chromium.org wrote: > > @@ -1683,7 +1683,8 @@ static int xol_add_vma(struct mm_struct *mm, struct > xol_area *area) > } > > vma = _install_special_mapping(mm, area->vaddr, PAGE_SIZE, > - VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO, > + VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO| > + VM_SEALED_SYSMAP, > &xol_mapping); Reviewed-by: Oleg Nesterov
Re: [PATCH v7 6/7] mseal, system mappings: uprobe mapping
On 02/26, Oleg Nesterov wrote: > > On 02/24, jef...@chromium.org wrote: > > > > Unlike other system mappings, the uprobe mapping is not > > established during program startup. However, its lifetime is the same > > as the process's lifetime. It could be sealed from creation. > > Agreed, VM_SEALED should be always for the "[uprobes]" vma, regardless > of config options. > > ACK, > > but can't we do > > #ifdef CONFIG_64BIT > /* VM is sealed, in vm_flags */ > #define VM_SEALED _BITUL(63) > + #else > + #define VM_SEALED 0 > #endif > > and then simply > > vma = _install_special_mapping(mm, area->vaddr, PAGE_SIZE, > - VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO, > + VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO|VM_SEALED, > > ? > > But I am fine either way, feel free to ignore. Yes, but either way, why your patch adds "unsigned long vm_flags" ? OK, perhaps it makes sense for readability, but vm_flags = VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO; vm_flags |= VM_SEALED_SYSMAP; looks a bit strange, why not vm_flags = VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO|VM_SEALED_SYSMAP; ? Oleg.
Re: [PATCH v7 6/7] mseal, system mappings: uprobe mapping
On 02/26, Lorenzo Stoakes wrote: > > On Wed, Feb 26, 2025 at 05:26:04PM +0100, Oleg Nesterov wrote: > > On 02/24, jef...@chromium.org wrote: > > > > > > Unlike other system mappings, the uprobe mapping is not > > > established during program startup. However, its lifetime is the same > > > as the process's lifetime. It could be sealed from creation. > > > > Agreed, VM_SEALED should be always for the "[uprobes]" vma, regardless > > of config options. > > If you think this ought to be the case generally, then perhaps we should > drop this patch from the commit and just do this separately as a > permanent-on thing, if you are sure this is fine + want it? See below... > An aside - we _definitely_ cannot allow this -system mapping stuff- to be > enabled without a config option, This is clear. But as for uprobes in particular I do think that VM_SEALED is always fine. Do we really want it? I dunno. If a task unmaps its "[uprobes]" vma it will crash when it hits the uprobes bp next time. Unless the probed insn can be emulated and it is not ret-probe. Do we really care? Again, I don't know. Should this change come as a separate patch? I don't understand why it should but I am fine either way. In short. please do what you think is right, VM_SEALED can't hurt uprobes ;) > > #ifdef CONFIG_64BIT > > /* VM is sealed, in vm_flags */ > > #define VM_SEALED _BITUL(63) > > + #else > > + #define VM_SEALED 0 > > #endif > > This has been raised a few times. Jeff objects to this OK, > > and then simply > > > > vma = _install_special_mapping(mm, area->vaddr, PAGE_SIZE, > > - VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO, > > + VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO|VM_SEALED, > > > > ? > > Nah you'd have to do: > > > > vma = _install_special_mapping(mm, area->vaddr, PAGE_SIZE, > VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO > #ifdef CONFIG_64BIT > VM_SEALED > #endif > , Why??? With the proposed change above VM_SEALED == 0 if !CONFIG_64BIT. Oleg.
Re: [PATCH v7 6/7] mseal, system mappings: uprobe mapping
On 02/26, Lorenzo Stoakes wrote: > > Like I said, Jeff opposes the change. I disagree with him, and agree with you, > because this is very silly. > > But I don't want to hold up this series with that discussion (this is for his > sake...) Neither me, so lets go with VM_SEALED_SYSMAP. My only objection is that vm_flags = VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO; vm_flags |= VM_SEALED_SYSMAP; looks unnecessarily confusing to me, vm_flags = VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO|VM_SEALED_SYSMAP; or just vma = _install_special_mapping(..., VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO|VM_SEALED_SYSMAP, ... looks more readable. But this is cosmetic/subjective, so I won't argue/insist. > Jeff - perhaps drop this and let's return to it in a follow up so this series > isn't held up? Up to you and Jeff. But this patch looks "natural" to me in this series. Oleg.
Re: [PATCH v7 6/7] mseal, system mappings: uprobe mapping
On 02/24, jef...@chromium.org wrote: > > Unlike other system mappings, the uprobe mapping is not > established during program startup. However, its lifetime is the same > as the process's lifetime. It could be sealed from creation. Agreed, VM_SEALED should be always for the "[uprobes]" vma, regardless of config options. ACK, but can't we do #ifdef CONFIG_64BIT /* VM is sealed, in vm_flags */ #define VM_SEALED _BITUL(63) + #else + #define VM_SEALED 0 #endif and then simply vma = _install_special_mapping(mm, area->vaddr, PAGE_SIZE, - VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO, + VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO|VM_SEALED, ? But I am fine either way, feel free to ignore. Oleg.
