| Message ID | 20250120134452.GA21275@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | seccomp: remove the 'sd' argument from __secure_computing() | expand |
On Mon, Jan 20, 2025 at 02:44:52PM +0100, Oleg Nesterov wrote: > Depending on CONFIG_HAVE_ARCH_SECCOMP_FILTER, __secure_computing(NULL) > will crash or not, this is not consistent/safe. Right now this never happens because there are no callers. > Fortunately, if CONFIG_HAVE_ARCH_SECCOMP_FILTER=n, __secure_computing() > has no callers, these architectures use secure_computing_strict(). As you say here. > Also, after the previous change __secure_computing(sd) is always called > with sd == NULL, so it is clear that we can remove the code which makes > no sense. However, after this change, if someone were to *add* a caller, it would bypass strict mode. Instead of "return 0", it seems like it'd be better to remove the function entirely (and maybe add a comment about calling secure_computing_strict() directly)? > > Signed-off-by: Oleg Nesterov <oleg@redhat.com> > --- > include/linux/seccomp.h | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h > index e45531455d3b..e01dfe57a884 100644 > --- a/include/linux/seccomp.h > +++ b/include/linux/seccomp.h > @@ -32,11 +32,7 @@ static inline int secure_computing(void) > } > #else > extern void secure_computing_strict(int this_syscall); > -static inline int __secure_computing(const struct seccomp_data *sd) > -{ > - secure_computing_strict(sd->nr); > - return 0; > -} > +static inline int __secure_computing(const struct seccomp_data *sd) { return 0; } > #endif > > extern long prctl_get_seccomp(void); > -- > 2.25.1.362.g51ebf55 >
On 01/20, Kees Cook wrote: > > On Mon, Jan 20, 2025 at 02:44:52PM +0100, Oleg Nesterov wrote: > > Depending on CONFIG_HAVE_ARCH_SECCOMP_FILTER, __secure_computing(NULL) > > will crash or not, this is not consistent/safe. > > Right now this never happens because there are no callers. > > > Fortunately, if CONFIG_HAVE_ARCH_SECCOMP_FILTER=n, __secure_computing() > > has no callers, these architectures use secure_computing_strict(). > > As you say here. > > > Also, after the previous change __secure_computing(sd) is always called > > with sd == NULL, so it is clear that we can remove the code which makes > > no sense. > > However, after this change, if someone were to *add* a caller, it would > bypass strict mode. OK, thanks, I agree this is not consistent, even if I think that !CONFIG_HAVE_ARCH_SECCOMP_FILTER arches should not add a new caller. > Instead of "return 0", it seems like it'd be better > to remove the function entirely (and maybe add a comment about calling > secure_computing_strict() directly)? This means that __secure_computing() will be defined even if !CONFIG_SECCOMP, but it won't be defined if CONFIG_SECCOMP && !CONFIG_HAVE_ARCH_SECCOMP_FILTER. How about __secure_computing() { return secure_computing_strict(syscall_get_nr(...)); } in the "#ifndef CONFIG_HAVE_ARCH_SECCOMP_FILTER" section near secure_computing_strict() in kernel/seccomp.c ? Oleg.
diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h index e45531455d3b..e01dfe57a884 100644 --- a/include/linux/seccomp.h +++ b/include/linux/seccomp.h @@ -32,11 +32,7 @@ static inline int secure_computing(void) } #else extern void secure_computing_strict(int this_syscall); -static inline int __secure_computing(const struct seccomp_data *sd) -{ - secure_computing_strict(sd->nr); - return 0; -} +static inline int __secure_computing(const struct seccomp_data *sd) { return 0; } #endif extern long prctl_get_seccomp(void);
Depending on CONFIG_HAVE_ARCH_SECCOMP_FILTER, __secure_computing(NULL) will crash or not, this is not consistent/safe. Fortunately, if CONFIG_HAVE_ARCH_SECCOMP_FILTER=n, __secure_computing() has no callers, these architectures use secure_computing_strict(). Also, after the previous change __secure_computing(sd) is always called with sd == NULL, so it is clear that we can remove the code which makes no sense. Signed-off-by: Oleg Nesterov <oleg@redhat.com> --- include/linux/seccomp.h | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)