* Re: [PATCH V31 25/25] debugfs: Disable open() when kernel is locked down
From: Andy Lutomirski @ 2019-03-27 4:29 UTC (permalink / raw)
To: Greg KH
Cc: Andy Lutomirski, Matthew Garrett, James Morris, LSM List, LKML,
David Howells, Linux API, Matthew Garrett
In-Reply-To: <20190327003057.GA27311@kroah.com>
On Tue, Mar 26, 2019 at 5:31 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Tue, Mar 26, 2019 at 12:20:24PM -0700, Andy Lutomirski wrote:
> > On Tue, Mar 26, 2019 at 11:28 AM Matthew Garrett
> > <matthewgarrett@google.com> wrote:
> > >
> > > From: Matthew Garrett <mjg59@google.com>
> > >
> > > debugfs has not been meaningfully audited in terms of ensuring that
> > > userland cannot trample over the kernel. At Greg's request, disable
> > > access to it entirely when the kernel is locked down. This is done at
> > > open() time rather than init time as the kernel lockdown status may be
> > > made stricter at runtime.
> >
> > Ugh. Some of those files are very useful. Could this perhaps still
> > allow O_RDONLY if we're in INTEGRITY mode?
>
> Useful for what? Debugging, sure, but for "normal operation", no kernel
> functionality should ever require debugfs. If it does, that's a bug and
> should be fixed.
>
I semi-regularly read files in debugfs to diagnose things, and I think
it would be good for this to work on distro kernels.
^ permalink raw reply
* Re: [PATCH V31 25/25] debugfs: Disable open() when kernel is locked down
From: Greg KH @ 2019-03-27 2:35 UTC (permalink / raw)
To: Matthew Garrett
Cc: James Morris, LSM List, Linux Kernel Mailing List, David Howells,
Linux API, Andy Lutomirski
In-Reply-To: <CACdnJuvBJmCRnzT=4qh9cmNkRNHi3Xfha5xqUqYu8cgCRrtsqQ@mail.gmail.com>
On Tue, Mar 26, 2019 at 07:06:36PM -0700, Matthew Garrett wrote:
> On Tue, Mar 26, 2019 at 5:31 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Tue, Mar 26, 2019 at 11:27:41AM -0700, Matthew Garrett wrote:
> > > From: Matthew Garrett <mjg59@google.com>
> > >
> > > debugfs has not been meaningfully audited in terms of ensuring that
> > > userland cannot trample over the kernel. At Greg's request, disable
> > > access to it entirely when the kernel is locked down. This is done at
> > > open() time rather than init time as the kernel lockdown status may be
> > > made stricter at runtime.
>
> (snip)
>
> > Why allow all this, why not just abort the registering of the filesystem
> > with the vfs core so it can't even be mounted?
>
> As mentioned in the commit message, because the lockdown state can be
> made stricter at runtime - blocking at mount time would be
> inconsistent if the machine is locked down afterwards. We could
> potentially assert that it's the admin's responsibility to ensure that
> debugfs isn't mounted at the point of policy being made stricter?
Ugh, I can not read, sorry, neverind.
The patch is fine as-is.
greg k-h
^ permalink raw reply
* Re: [PATCH V31 25/25] debugfs: Disable open() when kernel is locked down
From: Matthew Garrett @ 2019-03-27 2:06 UTC (permalink / raw)
To: Greg KH
Cc: James Morris, LSM List, Linux Kernel Mailing List, David Howells,
Linux API, Andy Lutomirski
In-Reply-To: <20190327003130.GB27311@kroah.com>
On Tue, Mar 26, 2019 at 5:31 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> On Tue, Mar 26, 2019 at 11:27:41AM -0700, Matthew Garrett wrote:
> > From: Matthew Garrett <mjg59@google.com>
> >
> > debugfs has not been meaningfully audited in terms of ensuring that
> > userland cannot trample over the kernel. At Greg's request, disable
> > access to it entirely when the kernel is locked down. This is done at
> > open() time rather than init time as the kernel lockdown status may be
> > made stricter at runtime.
(snip)
> Why allow all this, why not just abort the registering of the filesystem
> with the vfs core so it can't even be mounted?
As mentioned in the commit message, because the lockdown state can be
made stricter at runtime - blocking at mount time would be
inconsistent if the machine is locked down afterwards. We could
potentially assert that it's the admin's responsibility to ensure that
debugfs isn't mounted at the point of policy being made stricter?
^ permalink raw reply
* Re: [PATCH V31 25/25] debugfs: Disable open() when kernel is locked down
From: Greg KH @ 2019-03-27 0:31 UTC (permalink / raw)
To: Matthew Garrett
Cc: jmorris, linux-security-module, linux-kernel, dhowells, linux-api,
luto, Matthew Garrett
In-Reply-To: <20190326182742.16950-26-matthewgarrett@google.com>
On Tue, Mar 26, 2019 at 11:27:41AM -0700, Matthew Garrett wrote:
> From: Matthew Garrett <mjg59@google.com>
>
> debugfs has not been meaningfully audited in terms of ensuring that
> userland cannot trample over the kernel. At Greg's request, disable
> access to it entirely when the kernel is locked down. This is done at
> open() time rather than init time as the kernel lockdown status may be
> made stricter at runtime.
>
> Signed-off-by: Matthew Garrett <mjg59@google.com>
> Cc: gregkh@linuxfoundation.org
> ---
> fs/debugfs/file.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
> index 4fce1da7db23..9ae12ef29ba0 100644
> --- a/fs/debugfs/file.c
> +++ b/fs/debugfs/file.c
> @@ -142,6 +142,9 @@ static int open_proxy_open(struct inode *inode, struct file *filp)
> const struct file_operations *real_fops = NULL;
> int r;
>
> + if (kernel_is_locked_down("debugfs", LOCKDOWN_INTEGRITY))
> + return -EPERM;
Why allow all this, why not just abort the registering of the filesystem
with the vfs core so it can't even be mounted?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH V31 25/25] debugfs: Disable open() when kernel is locked down
From: Greg KH @ 2019-03-27 0:30 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Matthew Garrett, James Morris, LSM List, LKML, David Howells,
Linux API, Matthew Garrett
In-Reply-To: <CALCETrXDF2c2-gnOyNTnEn-uPAZp0AaSB_PB8e2y8jf7zo=Cyg@mail.gmail.com>
On Tue, Mar 26, 2019 at 12:20:24PM -0700, Andy Lutomirski wrote:
> On Tue, Mar 26, 2019 at 11:28 AM Matthew Garrett
> <matthewgarrett@google.com> wrote:
> >
> > From: Matthew Garrett <mjg59@google.com>
> >
> > debugfs has not been meaningfully audited in terms of ensuring that
> > userland cannot trample over the kernel. At Greg's request, disable
> > access to it entirely when the kernel is locked down. This is done at
> > open() time rather than init time as the kernel lockdown status may be
> > made stricter at runtime.
>
> Ugh. Some of those files are very useful. Could this perhaps still
> allow O_RDONLY if we're in INTEGRITY mode?
Useful for what? Debugging, sure, but for "normal operation", no kernel
functionality should ever require debugfs. If it does, that's a bug and
should be fixed.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 03/17] fpga: dfl: fme: support 512bit data width PR
From: Scott Wood @ 2019-03-26 21:22 UTC (permalink / raw)
To: Alan Tull
Cc: Wu Hao, Moritz Fischer, linux-fpga, linux-kernel, linux-api,
Ananda Ravuri, Xu Yilun
In-Reply-To: <CANk1AXSBHAudsLfNkSK1OuE6R9bogo6jW9bYmmZdvweTt3eCiA@mail.gmail.com>
On Tue, 2019-03-26 at 14:33 -0500, Alan Tull wrote:
> On Mon, Mar 25, 2019 at 5:58 PM Scott Wood <swood@redhat.com> wrote:
>
> Hi Scott,
>
> > On Mon, 2019-03-25 at 17:53 -0500, Scott Wood wrote:
> > > On Mon, 2019-03-25 at 11:07 +0800, Wu Hao wrote:
> > > > +#if defined(CONFIG_X86) && defined(CONFIG_AS_AVX512)
> > > > +
> > > > +#include <asm/fpu/api.h>
> > > > +
> > > > +static inline void copy512(void *src, void __iomem *dst)
> > > > +{
> > > > + kernel_fpu_begin();
> > > > +
> > > > + asm volatile("vmovdqu64 (%0), %%zmm0;"
> > > > + "vmovntdq %%zmm0, (%1);"
> > > > + :
> > > > + : "r"(src), "r"(dst));
> > > > +
> > > > + kernel_fpu_end();
> > > > +}
> > >
> > > Shouldn't there be some sort of check that AVX512 is actually
> > > supported
> > > on the running system?
> > >
> > > Also, src should be const, and the asm statement should have a memory
> > > clobber.
> > >
> > > > +#else
> > > > +static inline void copy512(void *src, void __iomem *dst)
> > > > +{
> > > > + WARN_ON_ONCE(1);
> > > > +}
> > > > +#endif
> > >
> > > Likewise, this will be called if a revision 2 device is used on non-
> > > x86
> > > (or on x86 with an old binutils). The driver should fall back to 32-
> > > bit
> > > in such cases.
> >
> > Sorry, I missed the comment about revision 2 only being on integrated
> > devices -- but will that always be the case? Seems worthwhile to check
> > for
> > AVX512 support anyway. And there's still the possibility of being built
> > with an old binutils such that CONFIG_AS_AVX512 is not set, or running
> > on a
> > kernel where avx512 was disabled via a boot option.
>
> The code checks for CONFIG_AS_AVX512 above.
That just indicates that binutils supports it. Plus, the code does not
check for CONFIG_AS_AVX512 when deciding whether to set pr_datawidth to 64
(and thus call copy512), so you'll get a WARN_ON rather than falling back to
32-bit.
> What boot option are you referring to?
clearcpuid=304
-Scott
^ permalink raw reply
* Re: [PATCH V31 10/25] PCI: Lock down BAR access when the kernel is locked down
From: Alex Williamson @ 2019-03-26 21:19 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Matthew Garrett, James Morris, LSM List, LKML, David Howells,
Linux API, Matthew Garrett, Matthew Garrett, Bjorn Helgaas,
linux-pci
In-Reply-To: <CALCETrW43Syz-deM-fGUCbWfA8tcsoCJJ5XqjFhpg+d0sNYDRA@mail.gmail.com>
On Tue, 26 Mar 2019 13:55:39 -0700
Andy Lutomirski <luto@kernel.org> wrote:
> On Tue, Mar 26, 2019 at 11:28 AM Matthew Garrett
> <matthewgarrett@google.com> wrote:
> >
> > From: Matthew Garrett <mjg59@srcf.ucam.org>
> >
> > Any hardware that can potentially generate DMA has to be locked down in
> > order to avoid it being possible for an attacker to modify kernel code,
> > allowing them to circumvent disabled module loading or module signing.
> > Default to paranoid - in future we can potentially relax this for
> > sufficiently IOMMU-isolated devices.
>
> Does this break vfio?
No, vfio provides its own interface to pci config space. Thanks,
Alex
^ permalink raw reply
* Re: [PATCH V31 11/25] x86: Lock down IO port access when the kernel is locked down
From: Andy Lutomirski @ 2019-03-26 20:56 UTC (permalink / raw)
To: Matthew Garrett
Cc: James Morris, LSM List, LKML, David Howells, Linux API,
Andrew Lutomirski, Matthew Garrett, Matthew Garrett, X86 ML
In-Reply-To: <20190326182742.16950-12-matthewgarrett@google.com>
On Tue, Mar 26, 2019 at 11:28 AM Matthew Garrett
<matthewgarrett@google.com> wrote:
>
> From: Matthew Garrett <mjg59@srcf.ucam.org>
>
> IO port access would permit users to gain access to PCI configuration
> registers, which in turn (on a lot of hardware) give access to MMIO
> register space. This would potentially permit root to trigger arbitrary
> DMA, so lock it down by default.
>
> This also implicitly locks down the KDADDIO, KDDELIO, KDENABIO and
> KDDISABIO console ioctls.
>
Reviewed-by: Andy Lutomirski <luto@kernel.org>
^ permalink raw reply
* Re: [PATCH V31 10/25] PCI: Lock down BAR access when the kernel is locked down
From: Andy Lutomirski @ 2019-03-26 20:55 UTC (permalink / raw)
To: Matthew Garrett
Cc: James Morris, LSM List, LKML, David Howells, Linux API,
Andrew Lutomirski, Matthew Garrett, Matthew Garrett,
Bjorn Helgaas, linux-pci
In-Reply-To: <20190326182742.16950-11-matthewgarrett@google.com>
On Tue, Mar 26, 2019 at 11:28 AM Matthew Garrett
<matthewgarrett@google.com> wrote:
>
> From: Matthew Garrett <mjg59@srcf.ucam.org>
>
> Any hardware that can potentially generate DMA has to be locked down in
> order to avoid it being possible for an attacker to modify kernel code,
> allowing them to circumvent disabled module loading or module signing.
> Default to paranoid - in future we can potentially relax this for
> sufficiently IOMMU-isolated devices.
Does this break vfio?
--Andy
^ permalink raw reply
* Re: [PATCH 23/27] bpf: Restrict kernel image access functions when the kernel is locked down
From: Matthew Garrett @ 2019-03-26 20:19 UTC (permalink / raw)
To: James Morris
Cc: Andy Lutomirski, Stephen Hemminger, Linux API, LSM List, LKML,
David Howells, Alexei Starovoitov, Network Development,
Chun-Yi Lee, Daniel Borkmann, Kees Cook, Will Drewry
In-Reply-To: <alpine.LRH.2.21.1903270523240.5235@namei.org>
On Tue, Mar 26, 2019 at 11:57 AM James Morris <jmorris@namei.org> wrote:
> - Assign an ID to each lockdown point
> - Implement a policy mechanism where each ID is mapped to 0 or 1
> - Allow this policy to be specified statically or dynamically
One of the problems with this approach is what the default behaviour
should be when a new feature is added. If an admin fails to notice
that there's now a new policy element, they run the risk of kernel
integrity being compromised via the new feature even if the rest of
the kernel is locked down.
^ permalink raw reply
* Re: [PATCH v1 2/4] pid: add pidctl()
From: Christian Brauner @ 2019-03-26 19:57 UTC (permalink / raw)
To: Joel Fernandes
Cc: jannh, khlebnikov, luto, dhowells, serge, ebiederm, linux-api,
linux-kernel, arnd, keescook, adobriyan, tglx, mtk.manpages,
bl0pbl33p, ldv, akpm, oleg, nagarathnam.muthusamy, cyphar, viro,
dancol
In-Reply-To: <20190326195151.GD114492@google.com>
On Tue, Mar 26, 2019 at 03:51:51PM -0400, Joel Fernandes wrote:
> On Tue, Mar 26, 2019 at 08:19:43PM +0100, Christian Brauner wrote:
> > On Tue, Mar 26, 2019 at 02:10:12PM -0400, Joel Fernandes wrote:
> > > On Tue, Mar 26, 2019 at 06:22:33PM +0100, Christian Brauner wrote:
> > > > On Tue, Mar 26, 2019 at 01:06:01PM -0400, Joel Fernandes wrote:
> > > > > On Tue, Mar 26, 2019 at 04:55:11PM +0100, Christian Brauner wrote:
> > > > > > The pidctl() syscalls builds on, extends, and improves translate_pid() [4].
> > > > > > I quote Konstantins original patchset first that has already been acked and
> > > > > > picked up by Eric before and whose functionality is preserved in this
> > > > > > syscall:
> > > > > >
> > > > > > "Each process have different pids, one for each pid namespace it belongs.
> > > > > > When interaction happens within single pid-ns translation isn't required.
> > > > > > More complicated scenarios needs special handling.
> > > > > >
> > > > > > For example:
> > > > > > - reading pid-files or logs written inside container with pid namespace
> > > > > > - writing logs with internal pids outside container for pushing them into
> > > > > > - attaching with ptrace to tasks from different pid namespace
> > > > > >
> > > > > > Generally speaking, any cross pid-ns API with pids needs translation.
> > > > > >
> > > > > > Currently there are several interfaces that could be used here:
> > > > > >
> > > > > > Pid namespaces are identified by device and inode of /proc/[pid]/ns/pid.
> > > > > >
> > > > > > Pids for nested pid namespaces are shown in file /proc/[pid]/status.
> > > > > > In some cases pid translation could be easily done using this information.
> > > > > > Backward translation requires scanning all tasks and becomes really
> > > > > > complicated for deeper namespace nesting.
> > > > > >
> > > > > > Unix socket automatically translates pid attached to SCM_CREDENTIALS.
> > > > > > This requires CAP_SYS_ADMIN for sending arbitrary pids and entering
> > > > > > into pid namespace, this expose process and could be insecure."
> > > > > >
> > > > > > The original patchset allowed two distinct operations implicitly:
> > > > > > - discovering whether pid namespaces (pidns) have a parent-child
> > > > > > relationship
> > > > > > - translating a pid from a source pidns into a target pidns
> > > > > >
> > > > > > Both tasks are accomplished in the original patchset by passing a pid
> > > > > > along. If the pid argument is passed as 1 the relationship between two pid
> > > > > > namespaces can be discovered.
> > > > > > The syscall will gain a lot clearer syntax and will be easier to use for
> > > > > > userspace if the task it is asked to perform is passed through a
> > > > > > command argument. Additionally, it allows us to remove an intrinsic race
> > > > > > caused by using the pid argument as a way to discover the relationship
> > > > > > between pid namespaces.
> > > > > > This patch introduces three commands:
> > > > > >
> > > > > > /* PIDCMD_QUERY_PID */
> > > > > > PIDCMD_QUERY_PID allows to translate a pid between pid namespaces.
> > > > > > Given a source pid namespace fd return the pid of the process in the target
> > > > > > namespace:
> > > > >
> > > > > Could we call this PIDCMD_TRANSLATE_PID please? QUERY is confusing/ambigious
> > > > > IMO (see below).
> > > >
> > > > Yes, doesn't matter to me too much what we call it.
> > >
> > > Ok.
> > >
> > > > > > 1. pidctl(PIDCMD_QUERY_PID, pid, source_fd, -1, 0)
> > > > > > - retrieve pidns identified by source_fd
> > > > > > - retrieve struct pid identifed by pid in pidns identified by source_fd
> > > > > > - retrieve callers pidns
> > > > > > - return pid in callers pidns
> > > > > >
> > > > > > 2. pidctl(PIDCMD_QUERY_PID, pid, -1, target_fd, 0)
> > > > > > - retrieve callers pidns
> > > > > > - retrieve struct pid identifed by pid in callers pidns
> > > > > > - retrieve pidns identified by target_fd
> > > > > > - return pid in pidns identified by target_fd
> > > > > >
> > > > > > 3. pidctl(PIDCMD_QUERY_PID, 1, source_fd, -1, 0)
> > > > > > - retrieve pidns identified by source_fd
> > > > > > - retrieve struct pid identifed by init task in pidns identified by source_fd
> > > > > > - retrieve callers pidns
> > > > > > - return pid of init task of pidns identified by source_fd in callers pidns
> > > > > >
> > > > > > 4. pidctl(PIDCMD_QUERY_PID, pid, source_fd, target_fd, 0)
> > > > > > - retrieve pidns identified by source_fd
> > > > > > - retrieve struct pid identifed by pid in pidns identified by source_fd
> > > > > > - retrieve pidns identified by target_fd
> > > > > > - check whether struct pid can be found in pidns identified by target_fd
> > > > > > - return pid in pidns identified by target_fd
> > > > > >
> > > > > > /* PIDCMD_QUERY_PIDNS */
> > > > > > PIDCMD_QUERY_PIDNS allows to determine the relationship between pid
> > > > > > namespaces.
> > > > > > In the original version of the pachset passing pid as 1 would allow to
> > > > > > deterimine the relationship between the pid namespaces. This is inherhently
> > > > > > racy. If pid 1 inside a pid namespace has died it would report false
> > > > > > negatives. For example, if pid 1 inside of the target pid namespace already
> > > > > > died, it would report that the target pid namespace cannot be reached from
> > > > > > the source pid namespace because it couldn't find the pid inside of the
> > > > > > target pid namespace and thus falsely report to the user that the two pid
> > > > > > namespaces are not related. This problem is simple to avoid. In the new
> > > > > > version we simply walk the list of ancestors and check whether the
> > > > > > namespace are related to each other. By doing it this way we can reliably
> > > > > > report what the relationship between two pid namespace file descriptors
> > > > > > looks like.
> > > > > >
> > > > > > 1. pidctl(PIDCMD_QUERY_PIDNS, 0, ns_fd1, ns_fd1, 0) == 0
> > > > > > - pidns_of(ns_fd1) and pidns_of(ns_fd2) are unrelated to each other
> > > > > >
> > > > > > 2. pidctl(PIDCMD_QUERY_PIDNS, 0, ns_fd1, ns_fd2, 0) == 1
> > > > > > - pidns_of(ns_fd1) == pidns_of(ns_fd2)
> > > > > >
> > > > > > 3. pidctl(PIDCMD_QUERY_PIDNS, 0, ns_fd1, ns_fd2, 0) == 2
> > > > > > - pidns_of(ns_fd1) is ancestor of pidns_of(ns_fd2)
> > > > > >
> > > > > > 4. pidctl(PIDCMD_QUERY_PIDNS, 0, ns_fd1, ns_fd2, 0) == 3
> > > > > > - pidns_of(ns_fd2) is ancestor of pidns_of(ns_fd1)
> > > > >
> > > > > Why not call this PIDCMD_COMPARE_PIDNS, since a comparison is what you're doing.
> > > >
> > > > Same.
> > >
> > > Ok, glad we agree on it.
> > >
> > > > >
> > > > > Again QUERY is ambigious here. Above you called QUERY to translate something,
> > > > > now you're calling QUERY to mean compare something. I suggest to be explicit
> > > > > about the operation PIDCMD_<OPERATION>_<OPERAND-TYPE>.
> > > > >
> > > > > Arguably, 2 syscalls for this is cleaner:
> > > > > pid_compare_namespaces(ns_fd1, ns_fd2);
> > > > > pid_translate(pid, ns_fd1, nds_fd2);
> > > >
> > > > I don't think we want to send out pid_compare_namespaces() as a separate
> > > > syscall. If that's the consensus I'd rather just drop this functionality
> > > > completely.
> > >
> > > I mean, if pid-namespace fd comparison functionality is not needed in
> > > userspace, why add it all. I suggest to drop it if not needed and can be
> > > considered for addition later when needed.
> > >
> > > Then you're just left with GET_PID and TRANSLATE_PID which we know we do need.
> > >
> > > > > > These two commands - PIDCMD_QUERY_PID and PIDCMD_QUERY_PIDNS - cover and
> > > > > > improve the functionality expressed implicitly in translate_pid() before.
> > > > > >
> > > > > > /* PIDCMD_GET_PIDFD */
> > > > >
> > > > > And this can be a third syscall:
> > > > > pidfd_translate(pid, ns_fd1, ns_fd2).
> > > >
> > > > Sigh, yes. But I honestly want other people other than us to come out
> > > > and say "yes, please send a PR to Linus with three separate syscalls for
> > > > very closely related functionality". There's a difference between "this
> > > > is how we would ideally like to do it" and "this is what is common
> > > > practice and will likely get accepted". But I'm really not opposed to it
> > > > per se.
> > >
> > > Ok.
> > >
> > > > > I am actually supportive of Daniel's view that by combining too many
> > > > > arguments into a single syscall, becomes confusing and sometimes some
> > > > > arguments have to be forced to 0 in the single shoe-horned syscall. Like you
> > > >
> > > > There's a difference between an ioctl() and say seccomp() which this is
> > > > close to:
> > > > int seccomp(unsigned int operation, unsigned int flags, void *args);
> > > > The point is that the functionality is closely related not just randomly
> > > > unrelated stuff. But as I said I'm more than willing to compromise.
> > >
> > > Sounds great, yeah whatever makes sense.
> >
> > One option is to do the following which removes the cmd argument all
> > together in favor of a flag GET_PIDFD:
> >
> > +SYSCALL_DEFINE4(pidfd_translate, pid_t, pid, int, source, int, target,
> > + unsigned int, flags)
> > +{
>
> Yes that's also fine. But would it not be better to just add the flags as an
Also fine. It's just a sketch. I'm working on another suggestion that
might be better.
> extra parameter to translate_pid syscall, telling it what to translate to
> (pidfd or pid)?
>
> thanks,
>
> - Joel
>
>
>
> > + struct pid_namespace *source_ns, *target_ns;
> > + struct pid *struct_pid;
> > + pid_t result;
> > +
> > + if (flags & ~GET_PIDFD)
> > + return -EINVAL;
> > +
> > + source_ns = get_pid_ns_by_fd(source);
> > + if (IS_ERR(source_ns))
> > + return PTR_ERR(source_ns);
> > +
> > + target_ns = get_pid_ns_by_fd(target);
> > + if (IS_ERR(target_ns)) {
> > + put_pid_ns(source_ns);
> > + return PTR_ERR(target_ns);
> > + }
> > +
> > + rcu_read_lock();
> > + struct_pid = get_pid(find_pid_ns(pid, source_ns));
> > + rcu_read_unlock();
> > +
> > + if (struct_pid)
> > + result = pid_nr_ns(struct_pid, target_ns);
> > + else
> > + result = -ESRCH;
> > +
> > + if ((flags & GET_PIDFD) && (result > 0))
> > + result = pidfd_create_fd(struct_pid, O_CLOEXEC);
> > +
> > + if (!result)
> > + result = -ENOENT;
> > +
> > + put_pid(struct_pid);
> > + put_pid_ns(target_ns);
> > + put_pid_ns(source_ns);
> > +
> > + return result;
> > +}
^ permalink raw reply
* Re: [PATCH v1 2/4] pid: add pidctl()
From: Joel Fernandes @ 2019-03-26 19:51 UTC (permalink / raw)
To: Christian Brauner
Cc: jannh, khlebnikov, luto, dhowells, serge, ebiederm, linux-api,
linux-kernel, arnd, keescook, adobriyan, tglx, mtk.manpages,
bl0pbl33p, ldv, akpm, oleg, nagarathnam.muthusamy, cyphar, viro,
dancol
In-Reply-To: <20190326191942.jyqpk3ehpbvhgvyz@brauner.io>
On Tue, Mar 26, 2019 at 08:19:43PM +0100, Christian Brauner wrote:
> On Tue, Mar 26, 2019 at 02:10:12PM -0400, Joel Fernandes wrote:
> > On Tue, Mar 26, 2019 at 06:22:33PM +0100, Christian Brauner wrote:
> > > On Tue, Mar 26, 2019 at 01:06:01PM -0400, Joel Fernandes wrote:
> > > > On Tue, Mar 26, 2019 at 04:55:11PM +0100, Christian Brauner wrote:
> > > > > The pidctl() syscalls builds on, extends, and improves translate_pid() [4].
> > > > > I quote Konstantins original patchset first that has already been acked and
> > > > > picked up by Eric before and whose functionality is preserved in this
> > > > > syscall:
> > > > >
> > > > > "Each process have different pids, one for each pid namespace it belongs.
> > > > > When interaction happens within single pid-ns translation isn't required.
> > > > > More complicated scenarios needs special handling.
> > > > >
> > > > > For example:
> > > > > - reading pid-files or logs written inside container with pid namespace
> > > > > - writing logs with internal pids outside container for pushing them into
> > > > > - attaching with ptrace to tasks from different pid namespace
> > > > >
> > > > > Generally speaking, any cross pid-ns API with pids needs translation.
> > > > >
> > > > > Currently there are several interfaces that could be used here:
> > > > >
> > > > > Pid namespaces are identified by device and inode of /proc/[pid]/ns/pid.
> > > > >
> > > > > Pids for nested pid namespaces are shown in file /proc/[pid]/status.
> > > > > In some cases pid translation could be easily done using this information.
> > > > > Backward translation requires scanning all tasks and becomes really
> > > > > complicated for deeper namespace nesting.
> > > > >
> > > > > Unix socket automatically translates pid attached to SCM_CREDENTIALS.
> > > > > This requires CAP_SYS_ADMIN for sending arbitrary pids and entering
> > > > > into pid namespace, this expose process and could be insecure."
> > > > >
> > > > > The original patchset allowed two distinct operations implicitly:
> > > > > - discovering whether pid namespaces (pidns) have a parent-child
> > > > > relationship
> > > > > - translating a pid from a source pidns into a target pidns
> > > > >
> > > > > Both tasks are accomplished in the original patchset by passing a pid
> > > > > along. If the pid argument is passed as 1 the relationship between two pid
> > > > > namespaces can be discovered.
> > > > > The syscall will gain a lot clearer syntax and will be easier to use for
> > > > > userspace if the task it is asked to perform is passed through a
> > > > > command argument. Additionally, it allows us to remove an intrinsic race
> > > > > caused by using the pid argument as a way to discover the relationship
> > > > > between pid namespaces.
> > > > > This patch introduces three commands:
> > > > >
> > > > > /* PIDCMD_QUERY_PID */
> > > > > PIDCMD_QUERY_PID allows to translate a pid between pid namespaces.
> > > > > Given a source pid namespace fd return the pid of the process in the target
> > > > > namespace:
> > > >
> > > > Could we call this PIDCMD_TRANSLATE_PID please? QUERY is confusing/ambigious
> > > > IMO (see below).
> > >
> > > Yes, doesn't matter to me too much what we call it.
> >
> > Ok.
> >
> > > > > 1. pidctl(PIDCMD_QUERY_PID, pid, source_fd, -1, 0)
> > > > > - retrieve pidns identified by source_fd
> > > > > - retrieve struct pid identifed by pid in pidns identified by source_fd
> > > > > - retrieve callers pidns
> > > > > - return pid in callers pidns
> > > > >
> > > > > 2. pidctl(PIDCMD_QUERY_PID, pid, -1, target_fd, 0)
> > > > > - retrieve callers pidns
> > > > > - retrieve struct pid identifed by pid in callers pidns
> > > > > - retrieve pidns identified by target_fd
> > > > > - return pid in pidns identified by target_fd
> > > > >
> > > > > 3. pidctl(PIDCMD_QUERY_PID, 1, source_fd, -1, 0)
> > > > > - retrieve pidns identified by source_fd
> > > > > - retrieve struct pid identifed by init task in pidns identified by source_fd
> > > > > - retrieve callers pidns
> > > > > - return pid of init task of pidns identified by source_fd in callers pidns
> > > > >
> > > > > 4. pidctl(PIDCMD_QUERY_PID, pid, source_fd, target_fd, 0)
> > > > > - retrieve pidns identified by source_fd
> > > > > - retrieve struct pid identifed by pid in pidns identified by source_fd
> > > > > - retrieve pidns identified by target_fd
> > > > > - check whether struct pid can be found in pidns identified by target_fd
> > > > > - return pid in pidns identified by target_fd
> > > > >
> > > > > /* PIDCMD_QUERY_PIDNS */
> > > > > PIDCMD_QUERY_PIDNS allows to determine the relationship between pid
> > > > > namespaces.
> > > > > In the original version of the pachset passing pid as 1 would allow to
> > > > > deterimine the relationship between the pid namespaces. This is inherhently
> > > > > racy. If pid 1 inside a pid namespace has died it would report false
> > > > > negatives. For example, if pid 1 inside of the target pid namespace already
> > > > > died, it would report that the target pid namespace cannot be reached from
> > > > > the source pid namespace because it couldn't find the pid inside of the
> > > > > target pid namespace and thus falsely report to the user that the two pid
> > > > > namespaces are not related. This problem is simple to avoid. In the new
> > > > > version we simply walk the list of ancestors and check whether the
> > > > > namespace are related to each other. By doing it this way we can reliably
> > > > > report what the relationship between two pid namespace file descriptors
> > > > > looks like.
> > > > >
> > > > > 1. pidctl(PIDCMD_QUERY_PIDNS, 0, ns_fd1, ns_fd1, 0) == 0
> > > > > - pidns_of(ns_fd1) and pidns_of(ns_fd2) are unrelated to each other
> > > > >
> > > > > 2. pidctl(PIDCMD_QUERY_PIDNS, 0, ns_fd1, ns_fd2, 0) == 1
> > > > > - pidns_of(ns_fd1) == pidns_of(ns_fd2)
> > > > >
> > > > > 3. pidctl(PIDCMD_QUERY_PIDNS, 0, ns_fd1, ns_fd2, 0) == 2
> > > > > - pidns_of(ns_fd1) is ancestor of pidns_of(ns_fd2)
> > > > >
> > > > > 4. pidctl(PIDCMD_QUERY_PIDNS, 0, ns_fd1, ns_fd2, 0) == 3
> > > > > - pidns_of(ns_fd2) is ancestor of pidns_of(ns_fd1)
> > > >
> > > > Why not call this PIDCMD_COMPARE_PIDNS, since a comparison is what you're doing.
> > >
> > > Same.
> >
> > Ok, glad we agree on it.
> >
> > > >
> > > > Again QUERY is ambigious here. Above you called QUERY to translate something,
> > > > now you're calling QUERY to mean compare something. I suggest to be explicit
> > > > about the operation PIDCMD_<OPERATION>_<OPERAND-TYPE>.
> > > >
> > > > Arguably, 2 syscalls for this is cleaner:
> > > > pid_compare_namespaces(ns_fd1, ns_fd2);
> > > > pid_translate(pid, ns_fd1, nds_fd2);
> > >
> > > I don't think we want to send out pid_compare_namespaces() as a separate
> > > syscall. If that's the consensus I'd rather just drop this functionality
> > > completely.
> >
> > I mean, if pid-namespace fd comparison functionality is not needed in
> > userspace, why add it all. I suggest to drop it if not needed and can be
> > considered for addition later when needed.
> >
> > Then you're just left with GET_PID and TRANSLATE_PID which we know we do need.
> >
> > > > > These two commands - PIDCMD_QUERY_PID and PIDCMD_QUERY_PIDNS - cover and
> > > > > improve the functionality expressed implicitly in translate_pid() before.
> > > > >
> > > > > /* PIDCMD_GET_PIDFD */
> > > >
> > > > And this can be a third syscall:
> > > > pidfd_translate(pid, ns_fd1, ns_fd2).
> > >
> > > Sigh, yes. But I honestly want other people other than us to come out
> > > and say "yes, please send a PR to Linus with three separate syscalls for
> > > very closely related functionality". There's a difference between "this
> > > is how we would ideally like to do it" and "this is what is common
> > > practice and will likely get accepted". But I'm really not opposed to it
> > > per se.
> >
> > Ok.
> >
> > > > I am actually supportive of Daniel's view that by combining too many
> > > > arguments into a single syscall, becomes confusing and sometimes some
> > > > arguments have to be forced to 0 in the single shoe-horned syscall. Like you
> > >
> > > There's a difference between an ioctl() and say seccomp() which this is
> > > close to:
> > > int seccomp(unsigned int operation, unsigned int flags, void *args);
> > > The point is that the functionality is closely related not just randomly
> > > unrelated stuff. But as I said I'm more than willing to compromise.
> >
> > Sounds great, yeah whatever makes sense.
>
> One option is to do the following which removes the cmd argument all
> together in favor of a flag GET_PIDFD:
>
> +SYSCALL_DEFINE4(pidfd_translate, pid_t, pid, int, source, int, target,
> + unsigned int, flags)
> +{
Yes that's also fine. But would it not be better to just add the flags as an
extra parameter to translate_pid syscall, telling it what to translate to
(pidfd or pid)?
thanks,
- Joel
> + struct pid_namespace *source_ns, *target_ns;
> + struct pid *struct_pid;
> + pid_t result;
> +
> + if (flags & ~GET_PIDFD)
> + return -EINVAL;
> +
> + source_ns = get_pid_ns_by_fd(source);
> + if (IS_ERR(source_ns))
> + return PTR_ERR(source_ns);
> +
> + target_ns = get_pid_ns_by_fd(target);
> + if (IS_ERR(target_ns)) {
> + put_pid_ns(source_ns);
> + return PTR_ERR(target_ns);
> + }
> +
> + rcu_read_lock();
> + struct_pid = get_pid(find_pid_ns(pid, source_ns));
> + rcu_read_unlock();
> +
> + if (struct_pid)
> + result = pid_nr_ns(struct_pid, target_ns);
> + else
> + result = -ESRCH;
> +
> + if ((flags & GET_PIDFD) && (result > 0))
> + result = pidfd_create_fd(struct_pid, O_CLOEXEC);
> +
> + if (!result)
> + result = -ENOENT;
> +
> + put_pid(struct_pid);
> + put_pid_ns(target_ns);
> + put_pid_ns(source_ns);
> +
> + return result;
> +}
^ permalink raw reply
* Re: [PATCH 03/17] fpga: dfl: fme: support 512bit data width PR
From: Alan Tull @ 2019-03-26 19:33 UTC (permalink / raw)
To: Scott Wood
Cc: Wu Hao, Moritz Fischer, linux-fpga, linux-kernel, linux-api,
Ananda Ravuri, Xu Yilun
In-Reply-To: <655bf2991a4f8bf6a473c91218d6dba7748520aa.camel@redhat.com>
On Mon, Mar 25, 2019 at 5:58 PM Scott Wood <swood@redhat.com> wrote:
Hi Scott,
>
> On Mon, 2019-03-25 at 17:53 -0500, Scott Wood wrote:
> > On Mon, 2019-03-25 at 11:07 +0800, Wu Hao wrote:
> > > In early partial reconfiguration private feature, it only
> > > supports 32bit data width when writing data to hardware for
> > > PR. 512bit data width PR support is an important optimization
> > > for some specific solutions (e.g. XEON with FPGA integrated),
> > > it allows driver to use AVX512 instruction to improve the
> > > performance of partial reconfiguration. e.g. programming one
> > > 100MB bitstream image via this 512bit data width PR hardware
> > > only takes ~300ms, but 32bit revision requires ~3s per test
> > > result.
> > >
> > > Please note now this optimization is only done on revision 2
> > > of this PR private feature which is only used in integrated
> > > solution that AVX512 is always supported.
> > >
> > > Signed-off-by: Ananda Ravuri <ananda.ravuri@intel.com>
> > > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > > Signed-off-by: Wu Hao <hao.wu@intel.com>
> > > ---
> > > drivers/fpga/dfl-fme-main.c | 3 ++
> > > drivers/fpga/dfl-fme-mgr.c | 75 +++++++++++++++++++++++++++++++++++++-
> > > --
> > > -----
> > > drivers/fpga/dfl-fme-pr.c | 45 ++++++++++++++++-----------
> > > drivers/fpga/dfl-fme.h | 2 ++
> > > drivers/fpga/dfl.h | 5 +++
> > > 5 files changed, 99 insertions(+), 31 deletions(-)
> > >
> > > diff --git a/drivers/fpga/dfl-fme-main.c b/drivers/fpga/dfl-fme-main.c
> > > index 086ad24..076d74f 100644
> > > --- a/drivers/fpga/dfl-fme-main.c
> > > +++ b/drivers/fpga/dfl-fme-main.c
> > > @@ -21,6 +21,8 @@
> > > #include "dfl.h"
> > > #include "dfl-fme.h"
> > >
> > > +#define DRV_VERSION "0.8"
> >
> > What is this going to be used for? Under what circumstances will the
> > driver version be bumped? What does it have to do with 512-bit writes?
> >
> > > +#if defined(CONFIG_X86) && defined(CONFIG_AS_AVX512)
> > > +
> > > +#include <asm/fpu/api.h>
> > > +
> > > +static inline void copy512(void *src, void __iomem *dst)
> > > +{
> > > + kernel_fpu_begin();
> > > +
> > > + asm volatile("vmovdqu64 (%0), %%zmm0;"
> > > + "vmovntdq %%zmm0, (%1);"
> > > + :
> > > + : "r"(src), "r"(dst));
> > > +
> > > + kernel_fpu_end();
> > > +}
> >
> > Shouldn't there be some sort of check that AVX512 is actually supported
> > on the running system?
> >
> > Also, src should be const, and the asm statement should have a memory
> > clobber.
> >
> > > +#else
> > > +static inline void copy512(void *src, void __iomem *dst)
> > > +{
> > > + WARN_ON_ONCE(1);
> > > +}
> > > +#endif
> >
> > Likewise, this will be called if a revision 2 device is used on non-x86
> > (or on x86 with an old binutils). The driver should fall back to 32-bit
> > in such cases.
>
> Sorry, I missed the comment about revision 2 only being on integrated
> devices -- but will that always be the case? Seems worthwhile to check for
> AVX512 support anyway. And there's still the possibility of being built
> with an old binutils such that CONFIG_AS_AVX512 is not set, or running on a
> kernel where avx512 was disabled via a boot option.
The code checks for CONFIG_AS_AVX512 above.
What boot option are you referring to?
Alan
>
> What about future revisions >= 2? Currently the driver will treat them as
> if they were revision < 2. Is that intended?
>
> -Scott
>
>
^ permalink raw reply
* Re: [PATCH 23/27] bpf: Restrict kernel image access functions when the kernel is locked down
From: Andy Lutomirski @ 2019-03-26 19:22 UTC (permalink / raw)
To: James Morris
Cc: Andy Lutomirski, Stephen Hemminger, Linux API, Matthew Garrett,
LSM List, LKML, David Howells, Alexei Starovoitov,
Network Development, Chun-Yi Lee, Daniel Borkmann, Kees Cook,
Will Drewry
In-Reply-To: <alpine.LRH.2.21.1903270523240.5235@namei.org>
On Tue, Mar 26, 2019 at 11:57 AM James Morris <jmorris@namei.org> wrote:
>
> On Mon, 25 Mar 2019, Andy Lutomirski wrote:
>
> > A while back, I suggested an approach to actually make this stuff
> > mergeable: submit a patch series that adds lockdown mode, enables it
> > by command line option (and maybe sysctl) *only* and has either no
> > effect or only a token effect. Then we can add actual features to
> > lockdown mode one at a time and review them separately.
>
> This makes sense to me.
>
> >
> > And I'm going to complain loudly unless two things change about this
> > whole thing:
> >
> > 1. Lockdown mode becomes three states, not a boolean. The states are:
> > no lockdown, best-effort-to-protect-kernel-integrity, and
> > best-effort-to-protect-kernel-secrecy-and-integrity. And this BPF
> > mess illustrates why: most users will really strongly object to
> > turning off BPF when they actually just want to protect kernel
> > integrity. And as far as I know, things like Secure Boot policy will
> > mostly care about integrity, not secrecy, and tracing and such should
> > work on a normal locked-down kernel. So I think we need this knob.
>
> Another approach would be to make this entirely policy based:
>
> - Assign an ID to each lockdown point
> - Implement a policy mechanism where each ID is mapped to 0 or 1
> - Allow this policy to be specified statically or dynamically
>
> So,
>
> kernel_is_locked_down("ioperm")
>
> becomes
>
> kernel_is_locked_down(LOCKDOWN_IOPERM)
>
> and this function checks e.g.
>
> if (lockdown_polcy[id]) {
> fail or warn;
> }
>
> Thoughts?
I'm concerned that this gives too much useless flexibility to
administrators and user code in general. If you can break kernel
integrity, you can break kernel integrity -- it shouldn't really
matter *how* you break it.
--Andy
^ permalink raw reply
* Re: [PATCH V31 25/25] debugfs: Disable open() when kernel is locked down
From: Matthew Garrett @ 2019-03-26 19:21 UTC (permalink / raw)
To: Andy Lutomirski
Cc: James Morris, LSM List, LKML, David Howells, Linux API, Greg KH
In-Reply-To: <CALCETrXDF2c2-gnOyNTnEn-uPAZp0AaSB_PB8e2y8jf7zo=Cyg@mail.gmail.com>
On Tue, Mar 26, 2019 at 12:20 PM Andy Lutomirski <luto@kernel.org> wrote:
> Ugh. Some of those files are very useful. Could this perhaps still
> allow O_RDONLY if we're in INTEGRITY mode?
The previous implementation did, but Greg wanted it to go away entirely.
^ permalink raw reply
* Re: [PATCH V31 22/25] bpf: Restrict bpf when kernel lockdown is in confidentiality mode
From: Andy Lutomirski @ 2019-03-26 19:21 UTC (permalink / raw)
To: Matthew Garrett
Cc: James Morris, LSM List, LKML, David Howells, Linux API,
Andrew Lutomirski, Alexei Starovoitov, Matthew Garrett,
Network Development, Chun-Yi Lee, Daniel Borkmann
In-Reply-To: <20190326182742.16950-23-matthewgarrett@google.com>
On Tue, Mar 26, 2019 at 11:28 AM Matthew Garrett
<matthewgarrett@google.com> wrote:
>
> From: David Howells <dhowells@redhat.com>
>
> There are some bpf functions can be used to read kernel memory:
> bpf_probe_read, bpf_probe_write_user and bpf_trace_printk. These allow
> private keys in kernel memory (e.g. the hibernation image signing key) to
> be read by an eBPF program and kernel memory to be altered without
> restriction. Disable them if the kernel has been locked down in
> confidentiality mode.
>
:)
This is yet another reason to get the new improved bpf_probe_user_read
stuff landed!
^ permalink raw reply
* Re: [PATCH V31 25/25] debugfs: Disable open() when kernel is locked down
From: Andy Lutomirski @ 2019-03-26 19:20 UTC (permalink / raw)
To: Matthew Garrett
Cc: James Morris, LSM List, LKML, David Howells, Linux API,
Andrew Lutomirski, Matthew Garrett, Greg KH
In-Reply-To: <20190326182742.16950-26-matthewgarrett@google.com>
On Tue, Mar 26, 2019 at 11:28 AM Matthew Garrett
<matthewgarrett@google.com> wrote:
>
> From: Matthew Garrett <mjg59@google.com>
>
> debugfs has not been meaningfully audited in terms of ensuring that
> userland cannot trample over the kernel. At Greg's request, disable
> access to it entirely when the kernel is locked down. This is done at
> open() time rather than init time as the kernel lockdown status may be
> made stricter at runtime.
Ugh. Some of those files are very useful. Could this perhaps still
allow O_RDONLY if we're in INTEGRITY mode?
^ permalink raw reply
* Re: [PATCH v1 2/4] pid: add pidctl()
From: Christian Brauner @ 2019-03-26 19:19 UTC (permalink / raw)
To: Joel Fernandes
Cc: jannh, khlebnikov, luto, dhowells, serge, ebiederm, linux-api,
linux-kernel, arnd, keescook, adobriyan, tglx, mtk.manpages,
bl0pbl33p, ldv, akpm, oleg, nagarathnam.muthusamy, cyphar, viro,
dancol
In-Reply-To: <20190326181012.GA138478@google.com>
On Tue, Mar 26, 2019 at 02:10:12PM -0400, Joel Fernandes wrote:
> On Tue, Mar 26, 2019 at 06:22:33PM +0100, Christian Brauner wrote:
> > On Tue, Mar 26, 2019 at 01:06:01PM -0400, Joel Fernandes wrote:
> > > On Tue, Mar 26, 2019 at 04:55:11PM +0100, Christian Brauner wrote:
> > > > The pidctl() syscalls builds on, extends, and improves translate_pid() [4].
> > > > I quote Konstantins original patchset first that has already been acked and
> > > > picked up by Eric before and whose functionality is preserved in this
> > > > syscall:
> > > >
> > > > "Each process have different pids, one for each pid namespace it belongs.
> > > > When interaction happens within single pid-ns translation isn't required.
> > > > More complicated scenarios needs special handling.
> > > >
> > > > For example:
> > > > - reading pid-files or logs written inside container with pid namespace
> > > > - writing logs with internal pids outside container for pushing them into
> > > > - attaching with ptrace to tasks from different pid namespace
> > > >
> > > > Generally speaking, any cross pid-ns API with pids needs translation.
> > > >
> > > > Currently there are several interfaces that could be used here:
> > > >
> > > > Pid namespaces are identified by device and inode of /proc/[pid]/ns/pid.
> > > >
> > > > Pids for nested pid namespaces are shown in file /proc/[pid]/status.
> > > > In some cases pid translation could be easily done using this information.
> > > > Backward translation requires scanning all tasks and becomes really
> > > > complicated for deeper namespace nesting.
> > > >
> > > > Unix socket automatically translates pid attached to SCM_CREDENTIALS.
> > > > This requires CAP_SYS_ADMIN for sending arbitrary pids and entering
> > > > into pid namespace, this expose process and could be insecure."
> > > >
> > > > The original patchset allowed two distinct operations implicitly:
> > > > - discovering whether pid namespaces (pidns) have a parent-child
> > > > relationship
> > > > - translating a pid from a source pidns into a target pidns
> > > >
> > > > Both tasks are accomplished in the original patchset by passing a pid
> > > > along. If the pid argument is passed as 1 the relationship between two pid
> > > > namespaces can be discovered.
> > > > The syscall will gain a lot clearer syntax and will be easier to use for
> > > > userspace if the task it is asked to perform is passed through a
> > > > command argument. Additionally, it allows us to remove an intrinsic race
> > > > caused by using the pid argument as a way to discover the relationship
> > > > between pid namespaces.
> > > > This patch introduces three commands:
> > > >
> > > > /* PIDCMD_QUERY_PID */
> > > > PIDCMD_QUERY_PID allows to translate a pid between pid namespaces.
> > > > Given a source pid namespace fd return the pid of the process in the target
> > > > namespace:
> > >
> > > Could we call this PIDCMD_TRANSLATE_PID please? QUERY is confusing/ambigious
> > > IMO (see below).
> >
> > Yes, doesn't matter to me too much what we call it.
>
> Ok.
>
> > > > 1. pidctl(PIDCMD_QUERY_PID, pid, source_fd, -1, 0)
> > > > - retrieve pidns identified by source_fd
> > > > - retrieve struct pid identifed by pid in pidns identified by source_fd
> > > > - retrieve callers pidns
> > > > - return pid in callers pidns
> > > >
> > > > 2. pidctl(PIDCMD_QUERY_PID, pid, -1, target_fd, 0)
> > > > - retrieve callers pidns
> > > > - retrieve struct pid identifed by pid in callers pidns
> > > > - retrieve pidns identified by target_fd
> > > > - return pid in pidns identified by target_fd
> > > >
> > > > 3. pidctl(PIDCMD_QUERY_PID, 1, source_fd, -1, 0)
> > > > - retrieve pidns identified by source_fd
> > > > - retrieve struct pid identifed by init task in pidns identified by source_fd
> > > > - retrieve callers pidns
> > > > - return pid of init task of pidns identified by source_fd in callers pidns
> > > >
> > > > 4. pidctl(PIDCMD_QUERY_PID, pid, source_fd, target_fd, 0)
> > > > - retrieve pidns identified by source_fd
> > > > - retrieve struct pid identifed by pid in pidns identified by source_fd
> > > > - retrieve pidns identified by target_fd
> > > > - check whether struct pid can be found in pidns identified by target_fd
> > > > - return pid in pidns identified by target_fd
> > > >
> > > > /* PIDCMD_QUERY_PIDNS */
> > > > PIDCMD_QUERY_PIDNS allows to determine the relationship between pid
> > > > namespaces.
> > > > In the original version of the pachset passing pid as 1 would allow to
> > > > deterimine the relationship between the pid namespaces. This is inherhently
> > > > racy. If pid 1 inside a pid namespace has died it would report false
> > > > negatives. For example, if pid 1 inside of the target pid namespace already
> > > > died, it would report that the target pid namespace cannot be reached from
> > > > the source pid namespace because it couldn't find the pid inside of the
> > > > target pid namespace and thus falsely report to the user that the two pid
> > > > namespaces are not related. This problem is simple to avoid. In the new
> > > > version we simply walk the list of ancestors and check whether the
> > > > namespace are related to each other. By doing it this way we can reliably
> > > > report what the relationship between two pid namespace file descriptors
> > > > looks like.
> > > >
> > > > 1. pidctl(PIDCMD_QUERY_PIDNS, 0, ns_fd1, ns_fd1, 0) == 0
> > > > - pidns_of(ns_fd1) and pidns_of(ns_fd2) are unrelated to each other
> > > >
> > > > 2. pidctl(PIDCMD_QUERY_PIDNS, 0, ns_fd1, ns_fd2, 0) == 1
> > > > - pidns_of(ns_fd1) == pidns_of(ns_fd2)
> > > >
> > > > 3. pidctl(PIDCMD_QUERY_PIDNS, 0, ns_fd1, ns_fd2, 0) == 2
> > > > - pidns_of(ns_fd1) is ancestor of pidns_of(ns_fd2)
> > > >
> > > > 4. pidctl(PIDCMD_QUERY_PIDNS, 0, ns_fd1, ns_fd2, 0) == 3
> > > > - pidns_of(ns_fd2) is ancestor of pidns_of(ns_fd1)
> > >
> > > Why not call this PIDCMD_COMPARE_PIDNS, since a comparison is what you're doing.
> >
> > Same.
>
> Ok, glad we agree on it.
>
> > >
> > > Again QUERY is ambigious here. Above you called QUERY to translate something,
> > > now you're calling QUERY to mean compare something. I suggest to be explicit
> > > about the operation PIDCMD_<OPERATION>_<OPERAND-TYPE>.
> > >
> > > Arguably, 2 syscalls for this is cleaner:
> > > pid_compare_namespaces(ns_fd1, ns_fd2);
> > > pid_translate(pid, ns_fd1, nds_fd2);
> >
> > I don't think we want to send out pid_compare_namespaces() as a separate
> > syscall. If that's the consensus I'd rather just drop this functionality
> > completely.
>
> I mean, if pid-namespace fd comparison functionality is not needed in
> userspace, why add it all. I suggest to drop it if not needed and can be
> considered for addition later when needed.
>
> Then you're just left with GET_PID and TRANSLATE_PID which we know we do need.
>
> > > > These two commands - PIDCMD_QUERY_PID and PIDCMD_QUERY_PIDNS - cover and
> > > > improve the functionality expressed implicitly in translate_pid() before.
> > > >
> > > > /* PIDCMD_GET_PIDFD */
> > >
> > > And this can be a third syscall:
> > > pidfd_translate(pid, ns_fd1, ns_fd2).
> >
> > Sigh, yes. But I honestly want other people other than us to come out
> > and say "yes, please send a PR to Linus with three separate syscalls for
> > very closely related functionality". There's a difference between "this
> > is how we would ideally like to do it" and "this is what is common
> > practice and will likely get accepted". But I'm really not opposed to it
> > per se.
>
> Ok.
>
> > > I am actually supportive of Daniel's view that by combining too many
> > > arguments into a single syscall, becomes confusing and sometimes some
> > > arguments have to be forced to 0 in the single shoe-horned syscall. Like you
> >
> > There's a difference between an ioctl() and say seccomp() which this is
> > close to:
> > int seccomp(unsigned int operation, unsigned int flags, void *args);
> > The point is that the functionality is closely related not just randomly
> > unrelated stuff. But as I said I'm more than willing to compromise.
>
> Sounds great, yeah whatever makes sense.
One option is to do the following which removes the cmd argument all
together in favor of a flag GET_PIDFD:
+SYSCALL_DEFINE4(pidfd_translate, pid_t, pid, int, source, int, target,
+ unsigned int, flags)
+{
+ struct pid_namespace *source_ns, *target_ns;
+ struct pid *struct_pid;
+ pid_t result;
+
+ if (flags & ~GET_PIDFD)
+ return -EINVAL;
+
+ source_ns = get_pid_ns_by_fd(source);
+ if (IS_ERR(source_ns))
+ return PTR_ERR(source_ns);
+
+ target_ns = get_pid_ns_by_fd(target);
+ if (IS_ERR(target_ns)) {
+ put_pid_ns(source_ns);
+ return PTR_ERR(target_ns);
+ }
+
+ rcu_read_lock();
+ struct_pid = get_pid(find_pid_ns(pid, source_ns));
+ rcu_read_unlock();
+
+ if (struct_pid)
+ result = pid_nr_ns(struct_pid, target_ns);
+ else
+ result = -ESRCH;
+
+ if ((flags & GET_PIDFD) && (result > 0))
+ result = pidfd_create_fd(struct_pid, O_CLOEXEC);
+
+ if (!result)
+ result = -ENOENT;
+
+ put_pid(struct_pid);
+ put_pid_ns(target_ns);
+ put_pid_ns(source_ns);
+
+ return result;
+}
^ permalink raw reply
* Re: [PATCH 23/27] bpf: Restrict kernel image access functions when the kernel is locked down
From: James Morris @ 2019-03-26 18:57 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Stephen Hemminger, Linux API, Matthew Garrett, LSM List, LKML,
David Howells, Alexei Starovoitov, Network Development,
Chun-Yi Lee, Daniel Borkmann, Kees Cook, Will Drewry
In-Reply-To: <CALCETrU=EqqRwyQGBT0=_N9-E_pBM53=z+WJrd9OzV7g6xJa0A@mail.gmail.com>
On Mon, 25 Mar 2019, Andy Lutomirski wrote:
> A while back, I suggested an approach to actually make this stuff
> mergeable: submit a patch series that adds lockdown mode, enables it
> by command line option (and maybe sysctl) *only* and has either no
> effect or only a token effect. Then we can add actual features to
> lockdown mode one at a time and review them separately.
This makes sense to me.
>
> And I'm going to complain loudly unless two things change about this
> whole thing:
>
> 1. Lockdown mode becomes three states, not a boolean. The states are:
> no lockdown, best-effort-to-protect-kernel-integrity, and
> best-effort-to-protect-kernel-secrecy-and-integrity. And this BPF
> mess illustrates why: most users will really strongly object to
> turning off BPF when they actually just want to protect kernel
> integrity. And as far as I know, things like Secure Boot policy will
> mostly care about integrity, not secrecy, and tracing and such should
> work on a normal locked-down kernel. So I think we need this knob.
Another approach would be to make this entirely policy based:
- Assign an ID to each lockdown point
- Implement a policy mechanism where each ID is mapped to 0 or 1
- Allow this policy to be specified statically or dynamically
So,
kernel_is_locked_down("ioperm")
becomes
kernel_is_locked_down(LOCKDOWN_IOPERM)
and this function checks e.g.
if (lockdown_polcy[id]) {
fail or warn;
}
Thoughts?
> 2. All the proponents of this series, and the documentation, needs to
> document that it's best effort. There will always be security bugs,
> and there will always be things we miss.
Right. Maintaining this feature will be an ongoing effort, and if its not
actively maintained, it will bitrot and become useless.
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH v1 2/4] pid: add pidctl()
From: Joel Fernandes @ 2019-03-26 18:47 UTC (permalink / raw)
To: Christian Brauner
Cc: jannh, khlebnikov, luto, dhowells, serge, ebiederm, linux-api,
linux-kernel, arnd, keescook, adobriyan, tglx, mtk.manpages,
bl0pbl33p, ldv, akpm, oleg, nagarathnam.muthusamy, cyphar, viro,
dancol
In-Reply-To: <20190326181929.eypk2hzprdltcrqd@brauner.io>
On Tue, Mar 26, 2019 at 07:19:30PM +0100, Christian Brauner wrote:
[snip]
> > > > I am actually supportive of Daniel's view that by combining too many
> > > > arguments into a single syscall, becomes confusing and sometimes some
> > > > arguments have to be forced to 0 in the single shoe-horned syscall. Like you
> > >
> > > There's a difference between an ioctl() and say seccomp() which this is
> > > close to:
> > > int seccomp(unsigned int operation, unsigned int flags, void *args);
> > > The point is that the functionality is closely related not just randomly
> > > unrelated stuff. But as I said I'm more than willing to compromise.
> >
> > Sounds great, yeah whatever makes sense.
>
> In case I haven't said this enough: I really appreciate the discussion
> and in general the help on this. That probably sometimes gets lost in
> mails sometimes. :)
I appreciate you saying that and thanks for the work on this :)
- Joel
^ permalink raw reply
* [PATCH V31 25/25] debugfs: Disable open() when kernel is locked down
From: Matthew Garrett @ 2019-03-26 18:27 UTC (permalink / raw)
To: jmorris
Cc: linux-security-module, linux-kernel, dhowells, linux-api, luto,
Matthew Garrett, gregkh
In-Reply-To: <20190326182742.16950-1-matthewgarrett@google.com>
From: Matthew Garrett <mjg59@google.com>
debugfs has not been meaningfully audited in terms of ensuring that
userland cannot trample over the kernel. At Greg's request, disable
access to it entirely when the kernel is locked down. This is done at
open() time rather than init time as the kernel lockdown status may be
made stricter at runtime.
Signed-off-by: Matthew Garrett <mjg59@google.com>
Cc: gregkh@linuxfoundation.org
---
fs/debugfs/file.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index 4fce1da7db23..9ae12ef29ba0 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -142,6 +142,9 @@ static int open_proxy_open(struct inode *inode, struct file *filp)
const struct file_operations *real_fops = NULL;
int r;
+ if (kernel_is_locked_down("debugfs", LOCKDOWN_INTEGRITY))
+ return -EPERM;
+
r = debugfs_file_get(dentry);
if (r)
return r == -EIO ? -ENOENT : r;
@@ -267,6 +270,9 @@ static int full_proxy_open(struct inode *inode, struct file *filp)
struct file_operations *proxy_fops = NULL;
int r;
+ if (kernel_is_locked_down("debugfs", LOCKDOWN_INTEGRITY))
+ return -EPERM;
+
r = debugfs_file_get(dentry);
if (r)
return r == -EIO ? -ENOENT : r;
--
2.21.0.392.gf8f6787159e-goog
^ permalink raw reply related
* [PATCH V31 24/25] lockdown: Print current->comm in restriction messages
From: Matthew Garrett @ 2019-03-26 18:27 UTC (permalink / raw)
To: jmorris
Cc: linux-security-module, linux-kernel, dhowells, linux-api, luto,
Matthew Garrett
In-Reply-To: <20190326182742.16950-1-matthewgarrett@google.com>
From: David Howells <dhowells@redhat.com>
Print the content of current->comm in messages generated by lockdown to
indicate a restriction that was hit. This makes it a bit easier to find
out what caused the message.
The message now patterned something like:
Lockdown: <comm>: <what> is restricted; see man kernel_lockdown.7
Signed-off-by: David Howells <dhowells@redhat.com>
Signed-off-by: Matthew Garrett <mjg59@google.com>
---
include/linux/ima.h | 9 ++++++
kernel/kexec_file.c | 7 +++-
security/integrity/ima/ima.h | 2 ++
security/integrity/ima/ima_main.c | 2 +-
security/integrity/ima/ima_policy.c | 50 +++++++++++++++++++++++++++++
security/lock_down.c | 4 +--
6 files changed, 70 insertions(+), 4 deletions(-)
diff --git a/include/linux/ima.h b/include/linux/ima.h
index b5e16b8c50b7..05921227d700 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -127,4 +127,13 @@ static inline int ima_inode_removexattr(struct dentry *dentry,
return 0;
}
#endif /* CONFIG_IMA_APPRAISE */
+
+#if defined(CONFIG_IMA_APPRAISE) && defined(CONFIG_INTEGRITY_TRUSTED_KEYRING)
+extern bool ima_appraise_signature(enum kernel_read_file_id func);
+#else
+static inline bool ima_appraise_kexec_signature(enum kernel_read_file_id func)
+{
+ return false;
+}
+#endif /* CONFIG_IMA_APPRAISE && CONFIG_INTEGRITY_TRUSTED_KEYRING */
#endif /* _LINUX_IMA_H */
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index a1cc37c8b43b..7599039623a7 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -240,7 +240,12 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
ret = 0;
- if (kernel_is_locked_down(reason, LOCKDOWN_INTEGRITY)) {
+ /* If IMA is guaranteed to appraise a signature on the kexec
+ * image, permit it even if the kernel is otherwise locked
+ * down.
+ */
+ if (!ima_appraise_signature(READING_KEXEC_IMAGE) &&
+ kernel_is_locked_down(reason, LOCKDOWN_INTEGRITY)) {
ret = -EPERM;
goto out;
}
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index cc12f3449a72..fe03cc6f1ca4 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -115,6 +115,8 @@ struct ima_kexec_hdr {
u64 count;
};
+extern const int read_idmap[];
+
#ifdef CONFIG_HAVE_IMA_KEXEC
void ima_load_kexec_buffer(void);
#else
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 4ffac4f5c647..106f06dee9d1 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -442,7 +442,7 @@ int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
return 0;
}
-static const int read_idmap[READING_MAX_ID] = {
+const int read_idmap[READING_MAX_ID] = {
[READING_FIRMWARE] = FIRMWARE_CHECK,
[READING_FIRMWARE_PREALLOC_BUFFER] = FIRMWARE_CHECK,
[READING_MODULE] = MODULE_CHECK,
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 122797023bdb..f8f1cdb74a4f 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -1341,3 +1341,53 @@ int ima_policy_show(struct seq_file *m, void *v)
return 0;
}
#endif /* CONFIG_IMA_READ_POLICY */
+
+#if defined(CONFIG_IMA_APPRAISE) && defined(CONFIG_INTEGRITY_TRUSTED_KEYRING)
+/*
+ * ima_appraise_signature: whether IMA will appraise a given function using
+ * an IMA digital signature. This is restricted to cases where the kernel
+ * has a set of built-in trusted keys in order to avoid an attacker simply
+ * loading additional keys.
+ */
+bool ima_appraise_signature(enum kernel_read_file_id id)
+{
+ struct ima_rule_entry *entry;
+ bool found = false;
+ enum ima_hooks func;
+
+ if (id >= READING_MAX_ID)
+ return false;
+
+ func = read_idmap[id] ?: FILE_CHECK;
+
+ rcu_read_lock();
+ list_for_each_entry_rcu(entry, ima_rules, list) {
+ if (entry->action != APPRAISE)
+ continue;
+
+ /*
+ * A generic entry will match, but otherwise require that it
+ * match the func we're looking for
+ */
+ if (entry->func && entry->func != func)
+ continue;
+
+ /*
+ * We require this to be a digital signature, not a raw IMA
+ * hash.
+ */
+ if (entry->flags & IMA_DIGSIG_REQUIRED)
+ found = true;
+
+ /*
+ * We've found a rule that matches, so break now even if it
+ * didn't require a digital signature - a later rule that does
+ * won't override it, so would be a false positive.
+ */
+ break;
+ }
+
+ rcu_read_unlock();
+ return found;
+}
+#endif /* CONFIG_IMA_APPRAISE && CONFIG_INTEGRITY_TRUSTED_KEYRING */
diff --git a/security/lock_down.c b/security/lock_down.c
index 0f9ef4c30aa8..6bcffd0bb200 100644
--- a/security/lock_down.c
+++ b/security/lock_down.c
@@ -70,8 +70,8 @@ bool __kernel_is_locked_down(const char *what, enum lockdown_level level,
bool first)
{
if ((kernel_locked_down >= level) && what && first)
- pr_notice("Lockdown: %s is restricted; see man kernel_lockdown.7\n",
- what);
+ pr_notice("Lockdown: %s: %s is restricted; see man kernel_lockdown.7\n",
+ current->comm, what);
return (kernel_locked_down >= level);
}
EXPORT_SYMBOL(__kernel_is_locked_down);
--
2.21.0.392.gf8f6787159e-goog
^ permalink raw reply related
* [PATCH V31 23/25] Lock down perf when in confidentiality mode
From: Matthew Garrett @ 2019-03-26 18:27 UTC (permalink / raw)
To: jmorris
Cc: linux-security-module, linux-kernel, dhowells, linux-api, luto,
Matthew Garrett, Peter Zijlstra, Ingo Molnar,
Arnaldo Carvalho de Melo
In-Reply-To: <20190326182742.16950-1-matthewgarrett@google.com>
From: David Howells <dhowells@redhat.com>
Disallow the use of certain perf facilities that might allow userspace to
access kernel data.
Signed-off-by: David Howells <dhowells@redhat.com>
Signed-off-by: Matthew Garrett <mjg59@google.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
---
kernel/events/core.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 3cd13a30f732..6ad3d83c091c 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -10461,6 +10461,12 @@ SYSCALL_DEFINE5(perf_event_open,
return -EINVAL;
}
+ if ((attr.sample_type & PERF_SAMPLE_REGS_INTR) &&
+ kernel_is_locked_down("PERF_SAMPLE_REGS_INTR",
+ LOCKDOWN_CONFIDENTIALITY))
+ /* REGS_INTR can leak data, lockdown must prevent this */
+ return -EPERM;
+
/* Only privileged users can get physical addresses */
if ((attr.sample_type & PERF_SAMPLE_PHYS_ADDR) &&
perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN))
--
2.21.0.392.gf8f6787159e-goog
^ permalink raw reply related
* [PATCH V31 22/25] bpf: Restrict bpf when kernel lockdown is in confidentiality mode
From: Matthew Garrett @ 2019-03-26 18:27 UTC (permalink / raw)
To: jmorris
Cc: linux-security-module, linux-kernel, dhowells, linux-api, luto,
Alexei Starovoitov, Matthew Garrett, netdev, Chun-Yi Lee,
Daniel Borkmann
In-Reply-To: <20190326182742.16950-1-matthewgarrett@google.com>
From: David Howells <dhowells@redhat.com>
There are some bpf functions can be used to read kernel memory:
bpf_probe_read, bpf_probe_write_user and bpf_trace_printk. These allow
private keys in kernel memory (e.g. the hibernation image signing key) to
be read by an eBPF program and kernel memory to be altered without
restriction. Disable them if the kernel has been locked down in
confidentiality mode.
Suggested-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Signed-off-by: Matthew Garrett <mjg59@google.com>
cc: netdev@vger.kernel.org
cc: Chun-Yi Lee <jlee@suse.com>
cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
---
kernel/trace/bpf_trace.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 8b068adb9da1..9e8eda605b5e 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -137,6 +137,9 @@ BPF_CALL_3(bpf_probe_read, void *, dst, u32, size, const void *, unsafe_ptr)
{
int ret;
+ if (kernel_is_locked_down("BPF", LOCKDOWN_CONFIDENTIALITY))
+ return -EINVAL;
+
ret = probe_kernel_read(dst, unsafe_ptr, size);
if (unlikely(ret < 0))
memset(dst, 0, size);
@@ -156,6 +159,8 @@ static const struct bpf_func_proto bpf_probe_read_proto = {
BPF_CALL_3(bpf_probe_write_user, void *, unsafe_ptr, const void *, src,
u32, size)
{
+ if (kernel_is_locked_down("BPF", LOCKDOWN_CONFIDENTIALITY))
+ return -EINVAL;
/*
* Ensure we're in user context which is safe for the helper to
* run. This helper has no business in a kthread.
@@ -207,6 +212,9 @@ BPF_CALL_5(bpf_trace_printk, char *, fmt, u32, fmt_size, u64, arg1,
char buf[64];
int i;
+ if (kernel_is_locked_down("BPF", LOCKDOWN_CONFIDENTIALITY))
+ return -EINVAL;
+
/*
* bpf_check()->check_func_arg()->check_stack_boundary()
* guarantees that fmt points to bpf program stack,
@@ -535,6 +543,9 @@ BPF_CALL_3(bpf_probe_read_str, void *, dst, u32, size,
{
int ret;
+ if (kernel_is_locked_down("BPF", LOCKDOWN_CONFIDENTIALITY))
+ return -EINVAL;
+
/*
* The strncpy_from_unsafe() call will likely not fill the entire
* buffer, but that's okay in this circumstance as we're probing
--
2.21.0.392.gf8f6787159e-goog
^ permalink raw reply related
* [PATCH V31 21/25] Lock down kprobes when in confidentiality mode
From: Matthew Garrett @ 2019-03-26 18:27 UTC (permalink / raw)
To: jmorris
Cc: linux-security-module, linux-kernel, dhowells, linux-api, luto,
Alexei Starovoitov, Matthew Garrett, Naveen N . Rao,
Anil S Keshavamurthy, davem, Masami Hiramatsu
In-Reply-To: <20190326182742.16950-1-matthewgarrett@google.com>
From: David Howells <dhowells@redhat.com>
Disallow the creation of kprobes when the kernel is locked down in
confidentiality mode by preventing their registration. This prevents
kprobes from being used to access kernel memory to steal crypto data.
Reported-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Signed-off-by: Matthew Garrett <mjg59@google.com>
Cc: Naveen N. Rao <naveen.n.rao@linux.ibm.com>
Cc: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>
Cc: davem@davemloft.net
Cc: Masami Hiramatsu <mhiramat@kernel.org>
---
kernel/kprobes.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index f4ddfdd2d07e..b9781bd2db8c 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1552,6 +1552,9 @@ int register_kprobe(struct kprobe *p)
struct module *probed_mod;
kprobe_opcode_t *addr;
+ if (kernel_is_locked_down("Use of kprobes", LOCKDOWN_CONFIDENTIALITY))
+ return -EPERM;
+
/* Adjust probe address from symbol */
addr = kprobe_addr(p);
if (IS_ERR(addr))
--
2.21.0.392.gf8f6787159e-goog
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox