Re: [PATCH v2 1/4] seccomp: Add sysctl to display available actions

On Tue, Feb 7, 2017 at 4:25 PM, Tyler Hicks <tyhicks(a)canonical.com&gt; wrote: > On 02/07/2017 06:03 PM, Kees Cook wrote: >> On Thu, Feb 2, 2017 at 9:37 PM, Tyler Hicks <tyhicks(a)canonical.com&gt; wrote: >>> This patch creates a read-only sysctl containing an ordered list of >>> seccomp actions that the kernel supports. The ordering, from left to >>> right, is the lowest action value (kill) to the highest action value >>> (allow). Currently, a read of the sysctl file would return "kill trap >>> errno trace allow". The contents of this sysctl file can be useful for >>> userspace code as well as the system administrator. >>> >>> The path to the sysctl is: >>> >>> /proc/sys/kernel/seccomp/actions_avail >>> >>> libseccomp and other userspace code can easily determine which actions >>> the current kernel supports. The set of actions supported by the current >>> kernel may be different than the set of action macros found in kernel >>> headers that were installed where the userspace code was built. >> >> This is certainly good: having a discoverable way to detect filter >> capabilities. I do wonder if it'd still be easier to just expose the >> max_log sysctl as a numeric value, since the SECCOMP_RET_* values are >> all part of uapi, so we can't escape their values... > > I was very torn on whether to use a numeric or string representation > here. The reason I decided on string representation is because I think > these sysctls are mostly aimed for admins and numeric representations > wouldn't be easy to use. I considered added a utility to libseccomp but, > since the kernel code to do a string representation was so simple, I > went with doing it in the kernel. Yeah, I think I like it just because it gives a way to discover the UAPI "level"... I will think more about this. For v3, let's keep the string stuff. > Another possibility is exposing the SECCOMP_RET_*_NAME macros as part of > the uapi. I like keeping the UAPI minimal. ;) >>> +static int __init seccomp_sysctl_init(void) >>> +{ >>> + struct ctl_table_header *hdr; >>> + >>> + hdr = register_sysctl_paths(seccomp_sysctl_path, seccomp_sysctl_table); >>> + kmemleak_not_leak(hdr); >> >> Will kmemleak complain about this if hdr is saved to a global (or not >> saved at all)? Also, something should be reported in the failure >> case... > > I have to admit to blindly following the example set by sysctl_init() in > kernel/sysctl.c. I can test what kmemleak will/won't complain about and > report back (tomorrow at the earliest). Cool, no rush. I'm backlogged on reviews anyway. :)