* Re: [RFC PATCH v9 03/13] mm: Add support for eXclusive Page Frame Ownership (XPFO)
From: Khalid Aziz @ 2019-04-17 16:49 UTC (permalink / raw)
To: Ingo Molnar
Cc: juergh, tycho, jsteckli, keescook, konrad.wilk, Juerg Haefliger,
deepa.srinivasan, chris.hyser, tyhicks, dwmw, andrew.cooper3, jcm,
boris.ostrovsky, iommu, x86, linux-arm-kernel, linux-doc,
linux-kernel, linux-mm, linux-security-module, Khalid Aziz,
Linus Torvalds, Andrew Morton, Thomas Gleixner, Andy Lutomirski,
Peter Zijlstra, Dave Hansen, Borislav Petkov, H. Peter Anvin,
Arjan van de Ven, Greg Kroah-Hartman
In-Reply-To: <20190417161042.GA43453@gmail.com>
On 4/17/19 10:15 AM, Ingo Molnar wrote:
>
> [ Sorry, had to trim the Cc: list from hell. Tried to keep all the
> mailing lists and all x86 developers. ]
>
> * Khalid Aziz <khalid.aziz@oracle.com> wrote:
>
>> From: Juerg Haefliger <juerg.haefliger@canonical.com>
>>
>> This patch adds basic support infrastructure for XPFO which protects
>> against 'ret2dir' kernel attacks. The basic idea is to enforce
>> exclusive ownership of page frames by either the kernel or userspace,
>> unless explicitly requested by the kernel. Whenever a page destined for
>> userspace is allocated, it is unmapped from physmap (the kernel's page
>> table). When such a page is reclaimed from userspace, it is mapped back
>> to physmap. Individual architectures can enable full XPFO support using
>> this infrastructure by supplying architecture specific pieces.
>
> I have a higher level, meta question:
>
> Is there any updated analysis outlining why this XPFO overhead would be
> required on x86-64 kernels running on SMAP/SMEP CPUs which should be all
> recent Intel and AMD CPUs, and with kernel that mark all direct kernel
> mappings as non-executable - which should be all reasonably modern
> kernels later than v4.0 or so?
>
> I.e. the original motivation of the XPFO patches was to prevent execution
> of direct kernel mappings. Is this motivation still present if those
> mappings are non-executable?
>
> (Sorry if this has been asked and answered in previous discussions.)
Hi Ingo,
That is a good question. Because of the cost of XPFO, we have to be very
sure we need this protection. The paper from Vasileios, Michalis and
Angelos - <http://www.cs.columbia.edu/~vpk/papers/ret2dir.sec14.pdf>,
does go into how ret2dir attacks can bypass SMAP/SMEP in sections 6.1
and 6.2.
Thanks,
Khalid
^ permalink raw reply
* Re: kernel BUG at kernel/cred.c:434!
From: Casey Schaufler @ 2019-04-17 16:42 UTC (permalink / raw)
To: Oleg Nesterov, Paul Moore
Cc: chengjian (D), Kees Cook, NeilBrown, Anna Schumaker,
linux-kernel@vger.kernel.org, Al Viro, Xiexiuqi (Xie XiuQi),
Li Bin, Jason Yan, Peter Zijlstra, Ingo Molnar,
Linux Security Module list, SELinux, Yang Yingliang, casey
In-Reply-To: <20190417162723.GK32622@redhat.com>
On 4/17/2019 9:27 AM, Oleg Nesterov wrote:
> On 04/17, Paul Moore wrote:
>> On Wed, Apr 17, 2019 at 10:57 AM Oleg Nesterov <oleg@redhat.com> wrote:
>>> On 04/17, Paul Moore wrote:
>>>> I'm tempted to simply return an error in selinux_setprocattr() if
>>>> the task's credentials are not the same as its real_cred;
>>> What about other modules? I have no idea what smack_setprocattr() is,
>>> but it too does prepare_creds/commit creds.
>>>
>>> it seems that the simplest workaround should simply add the additional
>>> cred == real_cred into proc_pid_attr_write().
>> Yes, that is simple, but I worry about what other LSMs might want to
>> do. While I believe failing if the effective creds are not the same
>> as the real_creds is okay for SELinux (possibly Smack too), I worry
>> about what other LSMs may want to do. After all,
>> proc_pid_attr_write() doesn't change the the creds itself, that is
>> something the specific LSMs do.
> Yes, but if proc_pid_attr_write() is called with cred != real_cred then
> something is already wrong?
>
> In fact, I think that something is already wrong if it is not called by
> user-space directly. Too late to ask, but why is this /proc/self/attr/
> magic not implemented via syscall(s) ?
Shell scripts, for one thing. It's a straightforward and appropriate
use of the /proc interface. System calls would require additional change
to existing programs, whereas using the /proc interface allows a good
deal to be done in the containing scripts.
^ permalink raw reply
* Re: kernel BUG at kernel/cred.c:434!
From: Oleg Nesterov @ 2019-04-17 16:27 UTC (permalink / raw)
To: Paul Moore
Cc: chengjian (D), Kees Cook, Casey Schaufler, NeilBrown,
Anna Schumaker, linux-kernel@vger.kernel.org, Al Viro,
Xiexiuqi (Xie XiuQi), Li Bin, Jason Yan, Peter Zijlstra,
Ingo Molnar, Linux Security Module list, SELinux, Yang Yingliang
In-Reply-To: <CAHC9VhSzo4DsMRknOA-4KKYJEir0wW74+quLheTjRw94OmhscA@mail.gmail.com>
On 04/17, Paul Moore wrote:
>
> On Wed, Apr 17, 2019 at 10:57 AM Oleg Nesterov <oleg@redhat.com> wrote:
> > On 04/17, Paul Moore wrote:
> > >
> > > I'm tempted to simply return an error in selinux_setprocattr() if
> > > the task's credentials are not the same as its real_cred;
> >
> > What about other modules? I have no idea what smack_setprocattr() is,
> > but it too does prepare_creds/commit creds.
> >
> > it seems that the simplest workaround should simply add the additional
> > cred == real_cred into proc_pid_attr_write().
>
> Yes, that is simple, but I worry about what other LSMs might want to
> do. While I believe failing if the effective creds are not the same
> as the real_creds is okay for SELinux (possibly Smack too), I worry
> about what other LSMs may want to do. After all,
> proc_pid_attr_write() doesn't change the the creds itself, that is
> something the specific LSMs do.
Yes, but if proc_pid_attr_write() is called with cred != real_cred then
something is already wrong?
In fact, I think that something is already wrong if it is not called by
user-space directly. Too late to ask, but why is this /proc/self/attr/
magic not implemented via syscall(s) ?
But, Paul, this is up to you. I don't understand this all even remotely.
Oleg.
^ permalink raw reply
* Re: [RFC PATCH v9 03/13] mm: Add support for eXclusive Page Frame Ownership (XPFO)
From: Ingo Molnar @ 2019-04-17 16:15 UTC (permalink / raw)
To: Khalid Aziz
Cc: juergh, tycho, jsteckli, keescook, konrad.wilk, Juerg Haefliger,
deepa.srinivasan, chris.hyser, tyhicks, dwmw, andrew.cooper3, jcm,
boris.ostrovsky, iommu, x86, linux-arm-kernel, linux-doc,
linux-kernel, linux-mm, linux-security-module, Khalid Aziz,
Linus Torvalds, Andrew Morton, Thomas Gleixner, Andy Lutomirski,
Peter Zijlstra, Dave Hansen, Borislav Petkov, H. Peter Anvin,
Arjan van de Ven, Greg Kroah-Hartman
In-Reply-To: <f1ac3700970365fb979533294774af0b0dd84b3b.1554248002.git.khalid.aziz@oracle.com>
[ Sorry, had to trim the Cc: list from hell. Tried to keep all the
mailing lists and all x86 developers. ]
* Khalid Aziz <khalid.aziz@oracle.com> wrote:
> From: Juerg Haefliger <juerg.haefliger@canonical.com>
>
> This patch adds basic support infrastructure for XPFO which protects
> against 'ret2dir' kernel attacks. The basic idea is to enforce
> exclusive ownership of page frames by either the kernel or userspace,
> unless explicitly requested by the kernel. Whenever a page destined for
> userspace is allocated, it is unmapped from physmap (the kernel's page
> table). When such a page is reclaimed from userspace, it is mapped back
> to physmap. Individual architectures can enable full XPFO support using
> this infrastructure by supplying architecture specific pieces.
I have a higher level, meta question:
Is there any updated analysis outlining why this XPFO overhead would be
required on x86-64 kernels running on SMAP/SMEP CPUs which should be all
recent Intel and AMD CPUs, and with kernel that mark all direct kernel
mappings as non-executable - which should be all reasonably modern
kernels later than v4.0 or so?
I.e. the original motivation of the XPFO patches was to prevent execution
of direct kernel mappings. Is this motivation still present if those
mappings are non-executable?
(Sorry if this has been asked and answered in previous discussions.)
Thanks,
Ingo
^ permalink raw reply
* Re: kernel BUG at kernel/cred.c:434!
From: Paul Moore @ 2019-04-17 15:40 UTC (permalink / raw)
To: Oleg Nesterov
Cc: chengjian (D), Kees Cook, Casey Schaufler, NeilBrown,
Anna Schumaker, linux-kernel@vger.kernel.org, Al Viro,
Xiexiuqi (Xie XiuQi), Li Bin, Jason Yan, Peter Zijlstra,
Ingo Molnar, Linux Security Module list, SELinux, Yang Yingliang
In-Reply-To: <20190417145711.GI32622@redhat.com>
On Wed, Apr 17, 2019 at 10:57 AM Oleg Nesterov <oleg@redhat.com> wrote:
> On 04/17, Paul Moore wrote:
> >
> > I'm tempted to simply return an error in selinux_setprocattr() if
> > the task's credentials are not the same as its real_cred;
>
> What about other modules? I have no idea what smack_setprocattr() is,
> but it too does prepare_creds/commit creds.
>
> it seems that the simplest workaround should simply add the additional
> cred == real_cred into proc_pid_attr_write().
Yes, that is simple, but I worry about what other LSMs might want to
do. While I believe failing if the effective creds are not the same
as the real_creds is okay for SELinux (possibly Smack too), I worry
about what other LSMs may want to do. After all,
proc_pid_attr_write() doesn't change the the creds itself, that is
something the specific LSMs do.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: kernel BUG at kernel/cred.c:434!
From: Casey Schaufler @ 2019-04-17 15:39 UTC (permalink / raw)
To: Oleg Nesterov, Paul Moore
Cc: chengjian (D), Kees Cook, NeilBrown, Anna Schumaker,
linux-kernel@vger.kernel.org, Al Viro, Xiexiuqi (Xie XiuQi),
Li Bin, Jason Yan, Peter Zijlstra, Ingo Molnar,
Linux Security Module list, SELinux, Yang Yingliang
In-Reply-To: <20190417145711.GI32622@redhat.com>
On 4/17/2019 7:57 AM, Oleg Nesterov wrote:
> On 04/17, Paul Moore wrote:
>> I'm tempted to simply return an error in selinux_setprocattr() if
>> the task's credentials are not the same as its real_cred;
> What about other modules? I have no idea what smack_setprocattr() is,
> but it too does prepare_creds/commit creds.
For what it's worth, my test for Smack does not reproduce
the problem.
>
> it seems that the simplest workaround should simply add the additional
> cred == real_cred into proc_pid_attr_write().
>
> Oleg.
>
^ permalink raw reply
* Re: [RFC PATCH v1 1/5] fs: Add support for an O_MAYEXEC flag on sys_open()
From: Mickaël Salaün @ 2019-04-17 15:04 UTC (permalink / raw)
To: Florian Weimer, Steve Grubb
Cc: Jan Kara, Mickaël Salaün, linux-kernel, Al Viro,
James Morris, Jonathan Corbet, Kees Cook, Matthew Garrett,
Michael Kerrisk, Mimi Zohar, Philippe Trébuchet, Shuah Khan,
Thibaut Sautereau, Vincent Strubel, Yves-Alexis Perez,
kernel-hardening, linux-api, linux-security-module, linux-fsdevel,
Matthew Bobrowski
In-Reply-To: <875zrd7xy8.fsf@oldenburg2.str.redhat.com>
On 17/04/2019 12:01, Florian Weimer wrote:
> * Steve Grubb:
>
>> On Tuesday, April 16, 2019 7:49:39 AM EDT Florian Weimer wrote:
>>> * Steve Grubb:
>>>> This flag that is being proposed means that you would have to patch all
>>>> interpreters to use it. If you are sure that upstreams will accept that,
>>>> why not just change the policy to interpreters shouldn't execute
>>>> anything unless the execute bit is set? That is simpler and doesn't need
>>>> a kernel change. And setting the execute bit is an auditable event.
>>>
>>> I think we need something like O_MAYEXEC so that security policies can
>>> be enforced and noexec mounts can be detected.
>>
>> Application whitelisting can already today stop unknown software without
>> needing O_MAYEXEC.
Whitelisting may be a lot of thing (path/TPE, signed binaries…), but
being able to handle this with a global system configuration (instead of
app-specific hardcoded configuration) is a good idea. ;)
>
> I'm somewhat interested in using this to add a proper check for
> executability to explicit dynamic loader invocations. In other words,
> this
>
> /lib64/ld-linux-x86-64.so.2 /path/to/noexec/fs/program
>
> should refuse to run the program if the program is located on a file
> system mounted with the noexec attribute.
What if a sysadmin need to do this on an executable mount point? Being
able to enforce a security policy according to a configuration may fit
to much more use cases.
>
>> The problem is that passing O_MAYEXEC is opt-in. You can use ptrace/seccomp/
>> bpf/LD_PRELOAD/LD_AUDIT to remove that bit from an otherwise normal program.
>> This does not require privs to do so.
>
> That doesn't really help with the above.
Right, ptrace/LD_PRELOAD and so on must be addressed by something else
than only O_MAYEXEC.
>
>> But let's consider that this comes to pass and every interpreter is
>> updated and IMA can see the O_MAYEXEC flag. Attackers now simply pivot
>> to running programs via stdin. It never touches disk and therefore
>> nothing enforces security policy. This already is among the most
>> common ways that malware runs today to evade detection.
As my previous reply, use cases like stdin may be restricted as well.
>
> Are you referring to Windows malware using Powershell?
>
> I'm not sure this is applicable to Linux. We do not have much
> behavioral monitoring anyway.
>
> Thanks,
> Florian
>
--
Mickaël Salaün
ANSSI/SDE/ST/LAM
Les données à caractère personnel recueillies et traitées dans le cadre de cet échange, le sont à seule fin d’exécution d’une relation professionnelle et s’opèrent dans cette seule finalité et pour la durée nécessaire à cette relation. Si vous souhaitez faire usage de vos droits de consultation, de rectification et de suppression de vos données, veuillez contacter contact.rgpd@sgdsn.gouv.fr. Si vous avez reçu ce message par erreur, nous vous remercions d’en informer l’expéditeur et de détruire le message. The personal data collected and processed during this exchange aims solely at completing a business relationship and is limited to the necessary duration of that relationship. If you wish to use your rights of consultation, rectification and deletion of your data, please contact: contact.rgpd@sgdsn.gouv.fr. If you have received this message in error, we thank you for informing the sender and destroying the message.
^ permalink raw reply
* Re: [RFC PATCH v1 1/5] fs: Add support for an O_MAYEXEC flag on sys_open()
From: Mickaël Salaün @ 2019-04-17 14:55 UTC (permalink / raw)
To: Steve Grubb, Jan Kara
Cc: Mickaël Salaün, linux-kernel, Al Viro, James Morris,
Jonathan Corbet, Kees Cook, Matthew Garrett, Michael Kerrisk,
Mimi Zohar, Philippe Trébuchet, Shuah Khan,
Thibaut Sautereau, Vincent Strubel, Yves-Alexis Perez,
kernel-hardening, linux-api, linux-security-module, linux-fsdevel,
Matthew Bobrowski, Florian Weimer
In-Reply-To: <3452959.b6JmBh7Lnt@x2>
On 15/04/2019 20:47, Steve Grubb wrote:
> Hello,
>
> On Wednesday, December 12, 2018 9:43:06 AM EDT Jan Kara wrote:
>> On Wed 12-12-18 09:17:08, Mickaël Salaün wrote:
>>> When the O_MAYEXEC flag is passed, sys_open() may be subject to
>>> additional restrictions depending on a security policy implemented by an
>>> LSM through the inode_permission hook.
>>>
>>> The underlying idea is to be able to restrict scripts interpretation
>>> according to a policy defined by the system administrator. For this to
>>> be possible, script interpreters must use the O_MAYEXEC flag
>>> appropriately. To be fully effective, these interpreters also need to
>>> handle the other ways to execute code (for which the kernel can't help):
>>> command line parameters (e.g., option -e for Perl), module loading
>>> (e.g., option -m for Python), stdin, file sourcing, environment
>>> variables, configuration files... According to the threat model, it may
>>> be acceptable to allow some script interpreters (e.g. Bash) to interpret
>>> commands from stdin, may it be a TTY or a pipe, because it may not be
>>> enough to (directly) perform syscalls.
>>>
>>> A simple security policy implementation is available in a following
>>> patch for Yama.
>>>
>>> This is an updated subset of the patch initially written by Vincent
>>> Strubel for CLIP OS:
>>> https://github.com/clipos-archive/src_platform_clip-patches/blob/f5cb330d
>>> 6b684752e403b4e41b39f7004d88e561/1901_open_mayexec.patch This patch has
>>> been used for more than 10 years with customized script interpreters.
>>> Some examples can be found here:
>>> https://github.com/clipos-archive/clipos4_portage-overlay/search?q=O_MAYE
>>> XEC
>>>
>>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>>> Signed-off-by: Thibaut Sautereau <thibaut.sautereau@ssi.gouv.fr>
>>> Signed-off-by: Vincent Strubel <vincent.strubel@ssi.gouv.fr>
>>> Reviewed-by: Philippe Trébuchet <philippe.trebuchet@ssi.gouv.fr>
>>> Cc: Al Viro <viro@zeniv.linux.org.uk>
>>> Cc: Kees Cook <keescook@chromium.org>
>>> Cc: Mickaël Salaün <mickael.salaun@ssi.gouv.fr>
>>
>> ...
>>
>>> diff --git a/fs/open.c b/fs/open.c
>>> index 0285ce7dbd51..75479b79a58f 100644
>>> --- a/fs/open.c
>>> +++ b/fs/open.c
>>> @@ -974,6 +974,10 @@ static inline int build_open_flags(int flags,
>>> umode_t mode, struct open_flags *o>
>>> if (flags & O_APPEND)
>>>
>>> acc_mode |= MAY_APPEND;
>>>
>>> + /* Check execution permissions on open. */
>>> + if (flags & O_MAYEXEC)
>>> + acc_mode |= MAY_OPENEXEC;
>>> +
>>>
>>> op->acc_mode = acc_mode;
>>>
>>> op->intent = flags & O_PATH ? 0 : LOOKUP_OPEN;
>>
>> I don't feel experienced enough in security to tell whether we want this
>> functionality or not. But if we do this, shouldn't we also set FMODE_EXEC
>> on the resulting struct file? That way also security_file_open() can be
>> used to arbitrate such executable opens and in particular
>> fanotify permission event FAN_OPEN_EXEC will get properly generated which I
>> guess is desirable (support for it is sitting in my tree waiting for the
>> merge window) - adding some audit people involved in FAN_OPEN_EXEC to CC.
>> Just an idea...
>
> Late in replying. But I think it's important to have a deep look into the
> issue.
>
> TL;DR - This is a gentle man's handshake. It won't _really_ solve the
> problem.
Thanks for your comments. You should find most answers in this thread:
https://lore.kernel.org/lkml/20181212081712.32347-4-mic@digikod.net/
The threat model targets persistent attacks. This O_MAYEXEC flag is not
a silver bullet but it's a needed block to enforce a security policy on
a trusted system. This means that every component executable on the
system must be controlled, which means they may need some bit of
customization. Today no userspace application use this flag (except in
CLIP OS), but we need to first create a feature before it can be used.
It is very important to have in mind that a system security policy need
to have a (central) security manager, in this case the kernel thanks to
Yama's policy (but it could be SELinux, IMA or any other LSM). The goal
is not to give to the developer the job of defining a security policy
for the *system*; this job is for the system administrator (or the distro).
>
> This flag that is being proposed means that you would have to patch all
> interpreters to use it. If you are sure that upstreams will accept that, why
> not just change the policy to interpreters shouldn't execute anything unless
> the execute bit is set? That is simpler and doesn't need a kernel change. And
> setting the execute bit is an auditable event.
As said above, the definition of a the security policy is the job of the
system administrator. Moreover, the security policy may be defined by
the mount point restrictions (i.e. noexec) but it should be definable
with something else (e.g. a SELinux or IMA policy which may be agnostic
to the mount points).
>
> The bottom line is that any interpreter has to become a security policy
> enforcement point whether by indicating it wants to execute by setting a flag
> or by refusing to use a file without execute bit set. But this just moves the
> problem to one that is harder to fix. Why in the world does any programming
> language allow programs to be loaded via stdin?
>
> It is possible to wget a program and pipe it into python which subsequently
> pulls down an ELF shared object and runs it all without touching disk via
> memfd_create (e.g. SnakeEater). This is all direct to memory execution. And
> direct to memory bypasses anti-virus, selinux, IMA, application whitelisting,
> and other integrity schemes.
>
> So, to fix this problem, you really need to not allow any programs to load via
> stdin so that everything that executes has to touch disk. This way you can
> get a fanotify event and see the application and vote yes/no on allowing it.
> And this will be particularly harder with the memfd_create fix for the runc
> container breakout. Prior to that, there were very few uses of that system
> call. Now it may be very common which means finding malicious use just got
> harder to spot.
As said above, stdin must be restricted in some way. You may want to
take a look at the CLIP OS patches (which doesn't only add the O_MAYEXEC
flag but restrict other way to interpret code). It may be foolish to
block or restrict stdin for interpreters on a developer workstation, but
it may make sense for an embedded custom system.
The same apply for memfd_create. If you want to enforce a security
policy on this kind of *file descriptor*, you should ask to the proper
LSM to do so. The current Yama patch deal with this kind of FD if they
are accessed through /proc/*/fd because the procfs is mounted with
noexec. Anyway, the interpreter must *inform* the LSM that it wants to
execute/interpret something from this FD, which is done thanks to the
O_MAYEXEC flag.
>
> But assuming these problems got fixed, then we have yet another place to look.
> Many interpreters allow you to specify a command to run via arguments. Some
> have a small buffer and some allow lengthy programs to be entered as an
> argument. One strategy might be that an attacker can bootstrap a lengthier
> program across the network. Python for example allows loading modules across
> a network. All you need to put in the commandline is the override for the
> module loader and a couple modules to import. It then loads the modules
> remotely. Getting rid of this hole will likely lead to some unhappy people -
> meaning it can't be fixed.
Again, this depend on the threat model and the corresponding product. If
you want to handle everything on your system, then you may need some
adjustments.
>
> And even if we get that fixed, we have one last hole to plug. Shells. One can
> simply start a shell and paste their program into the shell and then execute
> it. You can easily do this with bash or python or any language that has a
> REPL (read–eval–print loop). To fix this means divorcing the notion of a
> language from a REPL. Production systems really do not need a Python shell,
> they need the interpreter. I doubt that this would be popular. But fixing each
> of these issues is what it would take to prevent unknown software from
> running. Not going this far leaves holes.
This is also covered by the threat model defined in the patch 3/5 (i.e.
protect the kernel by restricting arbitrary syscalls).
Regards,
--
Mickaël Salaün
ANSSI/SDE/ST/LAM
Les données à caractère personnel recueillies et traitées dans le cadre de cet échange, le sont à seule fin d’exécution d’une relation professionnelle et s’opèrent dans cette seule finalité et pour la durée nécessaire à cette relation. Si vous souhaitez faire usage de vos droits de consultation, de rectification et de suppression de vos données, veuillez contacter contact.rgpd@sgdsn.gouv.fr. Si vous avez reçu ce message par erreur, nous vous remercions d’en informer l’expéditeur et de détruire le message. The personal data collected and processed during this exchange aims solely at completing a business relationship and is limited to the necessary duration of that relationship. If you wish to use your rights of consultation, rectification and deletion of your data, please contact: contact.rgpd@sgdsn.gouv.fr. If you have received this message in error, we thank you for informing the sender and destroying the message.
^ permalink raw reply
* Re: kernel BUG at kernel/cred.c:434!
From: Oleg Nesterov @ 2019-04-17 14:57 UTC (permalink / raw)
To: Paul Moore
Cc: chengjian (D), Kees Cook, Casey Schaufler, NeilBrown,
Anna Schumaker, linux-kernel@vger.kernel.org, Al Viro,
Xiexiuqi (Xie XiuQi), Li Bin, Jason Yan, Peter Zijlstra,
Ingo Molnar, Linux Security Module list, SELinux, Yang Yingliang
In-Reply-To: <CAHC9VhSpnSpYLLOcFUFLM=UU8tUTLAzJYb2NgdbN-nLPrh=55g@mail.gmail.com>
On 04/17, Paul Moore wrote:
>
> I'm tempted to simply return an error in selinux_setprocattr() if
> the task's credentials are not the same as its real_cred;
What about other modules? I have no idea what smack_setprocattr() is,
but it too does prepare_creds/commit creds.
it seems that the simplest workaround should simply add the additional
cred == real_cred into proc_pid_attr_write().
Oleg.
^ permalink raw reply
* Re: [PATCH 23/57] docs: netlabel: convert it to ReST
From: Paul Moore @ 2019-04-17 14:36 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Linux Doc Mailing List, Mauro Carvalho Chehab, linux-kernel,
Jonathan Corbet, netdev, linux-security-module
In-Reply-To: <104211eb0db14790ad328fff7ad7f712d8d647f9.1555382110.git.mchehab+samsung@kernel.org>
On Mon, Apr 15, 2019 at 10:56 PM Mauro Carvalho Chehab
<mchehab+samsung@kernel.org> wrote:
>
> Convert netlabel documentation to ReST.
>
> This was trivial: just add proper title markups.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
> ---
> Documentation/netlabel/cipso_ipv4.txt | 19 +++++++++++++------
> Documentation/netlabel/introduction.txt | 16 +++++++++++-----
> Documentation/netlabel/lsm_interface.txt | 16 +++++++++++-----
> 3 files changed, 35 insertions(+), 16 deletions(-)
Thanks for doing this. I should probably revisit these docs to make
sure they are still valid/correct; if nothing else that email address
is from two employers ago (my apologies to the current email address
holder).
Acked-by: Paul Moore <paul@paul-moore.com>
> diff --git a/Documentation/netlabel/cipso_ipv4.txt b/Documentation/netlabel/cipso_ipv4.txt
> index a6075481fd60..cbd3f3231221 100644
> --- a/Documentation/netlabel/cipso_ipv4.txt
> +++ b/Documentation/netlabel/cipso_ipv4.txt
> @@ -1,10 +1,13 @@
> +===================================
> NetLabel CIPSO/IPv4 Protocol Engine
> -==============================================================================
> +===================================
> +
> Paul Moore, paul.moore@hp.com
>
> May 17, 2006
>
> - * Overview
> +Overview
> +========
>
> The NetLabel CIPSO/IPv4 protocol engine is based on the IETF Commercial
> IP Security Option (CIPSO) draft from July 16, 1992. A copy of this
> @@ -13,7 +16,8 @@ draft can be found in this directory
> it to an RFC standard it has become a de-facto standard for labeled
> networking and is used in many trusted operating systems.
>
> - * Outbound Packet Processing
> +Outbound Packet Processing
> +==========================
>
> The CIPSO/IPv4 protocol engine applies the CIPSO IP option to packets by
> adding the CIPSO label to the socket. This causes all packets leaving the
> @@ -24,7 +28,8 @@ label by using the NetLabel security module API; if the NetLabel "domain" is
> configured to use CIPSO for packet labeling then a CIPSO IP option will be
> generated and attached to the socket.
>
> - * Inbound Packet Processing
> +Inbound Packet Processing
> +=========================
>
> The CIPSO/IPv4 protocol engine validates every CIPSO IP option it finds at the
> IP layer without any special handling required by the LSM. However, in order
> @@ -33,7 +38,8 @@ NetLabel security module API to extract the security attributes of the packet.
> This is typically done at the socket layer using the 'socket_sock_rcv_skb()'
> LSM hook.
>
> - * Label Translation
> +Label Translation
> +=================
>
> The CIPSO/IPv4 protocol engine contains a mechanism to translate CIPSO security
> attributes such as sensitivity level and category to values which are
> @@ -42,7 +48,8 @@ Domain Of Interpretation (DOI) definition and are configured through the
> NetLabel user space communication layer. Each DOI definition can have a
> different security attribute mapping table.
>
> - * Label Translation Cache
> +Label Translation Cache
> +=======================
>
> The NetLabel system provides a framework for caching security attribute
> mappings from the network labels to the corresponding LSM identifiers. The
> diff --git a/Documentation/netlabel/introduction.txt b/Documentation/netlabel/introduction.txt
> index 3caf77bcff0f..9333bbb0adc1 100644
> --- a/Documentation/netlabel/introduction.txt
> +++ b/Documentation/netlabel/introduction.txt
> @@ -1,10 +1,13 @@
> +=====================
> NetLabel Introduction
> -==============================================================================
> +=====================
> +
> Paul Moore, paul.moore@hp.com
>
> August 2, 2006
>
> - * Overview
> +Overview
> +========
>
> NetLabel is a mechanism which can be used by kernel security modules to attach
> security attributes to outgoing network packets generated from user space
> @@ -12,7 +15,8 @@ applications and read security attributes from incoming network packets. It
> is composed of three main components, the protocol engines, the communication
> layer, and the kernel security module API.
>
> - * Protocol Engines
> +Protocol Engines
> +================
>
> The protocol engines are responsible for both applying and retrieving the
> network packet's security attributes. If any translation between the network
> @@ -24,7 +28,8 @@ the NetLabel kernel security module API described below.
> Detailed information about each NetLabel protocol engine can be found in this
> directory.
>
> - * Communication Layer
> +Communication Layer
> +===================
>
> The communication layer exists to allow NetLabel configuration and monitoring
> from user space. The NetLabel communication layer uses a message based
> @@ -33,7 +38,8 @@ formatting of these NetLabel messages as well as the Generic NETLINK family
> names can be found in the 'net/netlabel/' directory as comments in the
> header files as well as in 'include/net/netlabel.h'.
>
> - * Security Module API
> +Security Module API
> +===================
>
> The purpose of the NetLabel security module API is to provide a protocol
> independent interface to the underlying NetLabel protocol engines. In addition
> diff --git a/Documentation/netlabel/lsm_interface.txt b/Documentation/netlabel/lsm_interface.txt
> index 638c74f7de7f..026fc267f798 100644
> --- a/Documentation/netlabel/lsm_interface.txt
> +++ b/Documentation/netlabel/lsm_interface.txt
> @@ -1,10 +1,13 @@
> +========================================
> NetLabel Linux Security Module Interface
> -==============================================================================
> +========================================
> +
> Paul Moore, paul.moore@hp.com
>
> May 17, 2006
>
> - * Overview
> +Overview
> +========
>
> NetLabel is a mechanism which can set and retrieve security attributes from
> network packets. It is intended to be used by LSM developers who want to make
> @@ -12,7 +15,8 @@ use of a common code base for several different packet labeling protocols.
> The NetLabel security module API is defined in 'include/net/netlabel.h' but a
> brief overview is given below.
>
> - * NetLabel Security Attributes
> +NetLabel Security Attributes
> +============================
>
> Since NetLabel supports multiple different packet labeling protocols and LSMs
> it uses the concept of security attributes to refer to the packet's security
> @@ -24,7 +28,8 @@ configuration. It is up to the LSM developer to translate the NetLabel
> security attributes into whatever security identifiers are in use for their
> particular LSM.
>
> - * NetLabel LSM Protocol Operations
> +NetLabel LSM Protocol Operations
> +================================
>
> These are the functions which allow the LSM developer to manipulate the labels
> on outgoing packets as well as read the labels on incoming packets. Functions
> @@ -32,7 +37,8 @@ exist to operate both on sockets as well as the sk_buffs directly. These high
> level functions are translated into low level protocol operations based on how
> the administrator has configured the NetLabel subsystem.
>
> - * NetLabel Label Mapping Cache Operations
> +NetLabel Label Mapping Cache Operations
> +=======================================
>
> Depending on the exact configuration, translation between the network packet
> label and the internal LSM security identifier can be time consuming. The
> --
> 2.20.1
>
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: kernel BUG at kernel/cred.c:434!
From: Paul Moore @ 2019-04-17 14:30 UTC (permalink / raw)
To: chengjian (D)
Cc: Kees Cook, Oleg Nesterov, Casey Schaufler, NeilBrown,
Anna Schumaker, linux-kernel@vger.kernel.org, Al Viro,
Xiexiuqi (Xie XiuQi), Li Bin, Jason Yan, Peter Zijlstra,
Ingo Molnar, Linux Security Module list, SELinux, Yang Yingliang
In-Reply-To: <fa325d38-db66-0ae7-0a6c-75934a9b0653@huawei.com>
On Tue, Apr 16, 2019 at 10:46 AM chengjian (D) <cj.chengjian@huawei.com> wrote:
> On 2019/4/16 11:40, Kees Cook wrote:
> > On Mon, Apr 15, 2019 at 11:20 AM Paul Moore <paul@paul-moore.com> wrote:
> >> On Mon, Apr 15, 2019 at 11:05 AM Oleg Nesterov <oleg@redhat.com> wrote:
> >>> On 04/15, Paul Moore wrote:
> >>>> On Mon, Apr 15, 2019 at 9:43 AM Oleg Nesterov <oleg@redhat.com> wrote:
> >>>>> Well, acct("/proc/self/attr/current") doesn't look like a good idea, but I do
> >>>>> not know where should we put the additional check... And probably
> >>>>> "echo /proc/self/attr/current > /proc/sys/kernel/core_pattern" can hit the
> >>>>> same problem, do_coredump() does override_creds() too.
> >>>>>
> >>>>> May be just add
> >>>>>
> >>>>> if (current->cred != current->real_cred)
> >>>>> return -EACCES;
> >>>>>
> >>>>> into proc_pid_attr_write(), I dunno.
> >>>> Is the problem that do_acct_process() is calling override_creds() and
> >>>> the returned/old credentials are being freed before do_acct_process()
> >>>> can reinstall the creds via revert_creds()? Presumably because the
> >>>> process accounting is causing the credentials to be replaced?
> >>> Afaics, the problem is that do_acct_process() does override_creds() and
> >>> then __kernel_write(). Which calls proc_pid_attr_write(), which in turn calls
> >>> selinux_setprocattr(), which does another prepare_creds() + commit_creds();
> >>> and commit_creds() hits
> >>>
> >>> BUG_ON(task->cred != old);
> >> Gotcha. In the process of looking at the backtrace I forgot about the
> >> BUG_ON() at the top of the oops message.
> >>
> >> I wonder what terrible things would happen if we changed the BUG_ON()
> >> in commit_creds to simple returning an error an error code to the
> >> caller. There is a warning/requirement in commit_creds() function
> >> header comment that it should always return 0.
> > Would callers be expected to call abort_creds() on failure? There are
> > a number of places where it'd need fixing up. And would likely be best
> > with a __must_check marking.
> >
> > It seems like avoiding the pathological case might be simpler?
>
> Yeah, Avoiding this pathological case is a better solution.
No arguments that this is particularly messed up scenario, I'm just
trying to arrive at a solution that isn't too ugly.
> From: Yang Yingliang <yangyingliang@huawei.com>
> Date: Sat, 13 Apr 2019 21:56:01 +0800
> Subject: [PATCH] fix cred bug_on
>
> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
> ---
> kernel/acct.c | 3 ++-
> kernel/cred.c | 6 ++++++
> security/selinux/hooks.c | 2 ++
> 3 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/acct.c b/kernel/acct.c
> index addf7732fb56..f2065f899eee 100644
> --- a/kernel/acct.c
> +++ b/kernel/acct.c
> @@ -522,7 +522,8 @@ static void do_acct_process(struct bsd_acct_struct
> *acct)
> }
> out:
> current->signal->rlim[RLIMIT_FSIZE].rlim_cur = flim;
> - revert_creds(orig_cred);
> + if (orig_cred == current->real_cred) // [2]
> + revert_creds(orig_cred);
> }
>
> /**
> diff --git a/kernel/cred.c b/kernel/cred.c
> index ecf03657e71c..c4d5ba92fb9b 100644
> --- a/kernel/cred.c
> +++ b/kernel/cred.c
> @@ -522,6 +522,9 @@ const struct cred *override_creds(const struct cred
> *new)
> {
> const struct cred *old = current->cred;
>
> + if (old == new) // [3]
> + return old;
> +
> kdebug("override_creds(%p{%d,%d})", new,
> atomic_read(&new->usage),
> read_cred_subscribers(new));
> @@ -551,6 +554,9 @@ void revert_creds(const struct cred *old)
> {
> const struct cred *override = current->cred;
>
> + if (override == old) // [3]
> + return;
> +
> kdebug("revert_creds(%p{%d,%d})", old,
> atomic_read(&old->usage),
> read_cred_subscribers(old));
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index b5017beb4ef7..bc8108e4e90f 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6590,6 +6590,8 @@ static int selinux_setprocattr(const char *name,
> void *value, size_t size)
> goto abort_change;
> }
>
> + if (current->cred != current->real_cred) // [1]
> + revert_creds(current->real_cred);
> commit_creds(new);
> return size;
Doing the revert only to then commit the creds seems really ugly to
me. I'm tempted to simply return an error in selinux_setprocattr() if
the task's credentials are not the same as its real_cred; if we do
that I believe we should resolve this problem. The accounting write
to the SELinux file in /proc would fail of course, but I think we can
all consider that as a positive side-effect.
While I don't think this should have a negative impact on anything
else, I haven't convinced myself of that just yet.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: fanotify and LSM path hooks
From: Miklos Szeredi @ 2019-04-17 14:14 UTC (permalink / raw)
To: Jan Kara
Cc: Amir Goldstein, linux-fsdevel, Al Viro, Matthew Bobrowski,
LSM List, overlayfs
In-Reply-To: <20190417140558.GB15563@quack2.suse.cz>
On Wed, Apr 17, 2019 at 4:06 PM Jan Kara <jack@suse.cz> wrote:
>
> On Wed 17-04-19 14:14:58, Miklos Szeredi wrote:
> > On Wed, Apr 17, 2019 at 1:30 PM Jan Kara <jack@suse.cz> wrote:
> > >
> > > On Tue 16-04-19 21:24:44, Amir Goldstein wrote:
> > > > > I'm not so sure about directory pre-modification hooks. Given the amount of
> > > > > problems we face with applications using fanotify permission events and
> > > > > deadlocking the system, I'm not very fond of expanding that API... AFAIU
> > > > > you want to use such hooks for recording (and persisting) that some change
> > > > > is going to happen and provide crash-consistency guarantees for such
> > > > > journal?
> > > > >
> > > >
> > > > That's the general idea.
> > > > I have two use cases for pre-modification hooks:
> > > > 1. VFS level snapshots
> > > > 2. persistent change tracking
> > > >
> > > > TBH, I did not consider implementing any of the above in userspace,
> > > > so I do not have a specific interest in extending the fanotify API.
> > > > I am actually interested in pre-modify fsnotify hooks (not fanotify),
> > > > that a snapshot or change tracking subsystem can register with.
> > > > An in-kernel fsnotify event handler can set a flag in current task
> > > > struct to circumvent system deadlocks on nested filesystem access.
> > >
> > > OK, I'm not opposed to fsnotify pre-modify hooks as such. As long as
> > > handlers stay within the kernel, I'm fine with that. After all this is what
> > > LSMs are already doing. Just exposing this to userspace for arbitration is
> > > what I have a problem with.
> >
> > There's one more usecase that I'd like to explore: providing coherent
> > view of host filesystem in virtualized environments. This requires
> > that guest is synchronously notified when the host filesystem changes.
> > I do agree, however, that adding sync hooks to userspace is
> > problematic.
> >
> > One idea would be to use shared memory instead of a procedural
> > notification. I.e. application (hypervisor) registers a pointer to a
> > version number that the kernel associates with the given inode. When
> > the inode is changed, then the version number is incremented. The
> > guest kernel can then look at the version number when verifying cache
> > validity. That way perfect coherency is guaranteed between host and
> > guest filesystems without allowing a broken guest or even a broken
> > hypervisor to DoS the host.
>
> Well, statx() and looking at i_version can do this for you. So I guess
> that's too slow for your purposes?
Okay, missing piece of information: we want to make use of the dcache
and icache in the guest kernel, otherwise lookup/stat will be
painfully slow. That would preclude doing statx() or anything else
that requires a synchronous round trip to the host for the likely case
of a valid cache.
> Also how many inodes do you want to
> monitor like this?
Everything that's in the guest caches. Which means: a lot.
Thanks,
Miklos
^ permalink raw reply
* Re: fanotify and LSM path hooks
From: Jan Kara @ 2019-04-17 14:05 UTC (permalink / raw)
To: Miklos Szeredi
Cc: Jan Kara, Amir Goldstein, linux-fsdevel, Al Viro,
Matthew Bobrowski, LSM List, overlayfs
In-Reply-To: <CAJfpegsyzYTkUw7rgpQfB0BKqK-LBZh6fBMUO5iq2JHv1youzw@mail.gmail.com>
On Wed 17-04-19 14:14:58, Miklos Szeredi wrote:
> On Wed, Apr 17, 2019 at 1:30 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Tue 16-04-19 21:24:44, Amir Goldstein wrote:
> > > > I'm not so sure about directory pre-modification hooks. Given the amount of
> > > > problems we face with applications using fanotify permission events and
> > > > deadlocking the system, I'm not very fond of expanding that API... AFAIU
> > > > you want to use such hooks for recording (and persisting) that some change
> > > > is going to happen and provide crash-consistency guarantees for such
> > > > journal?
> > > >
> > >
> > > That's the general idea.
> > > I have two use cases for pre-modification hooks:
> > > 1. VFS level snapshots
> > > 2. persistent change tracking
> > >
> > > TBH, I did not consider implementing any of the above in userspace,
> > > so I do not have a specific interest in extending the fanotify API.
> > > I am actually interested in pre-modify fsnotify hooks (not fanotify),
> > > that a snapshot or change tracking subsystem can register with.
> > > An in-kernel fsnotify event handler can set a flag in current task
> > > struct to circumvent system deadlocks on nested filesystem access.
> >
> > OK, I'm not opposed to fsnotify pre-modify hooks as such. As long as
> > handlers stay within the kernel, I'm fine with that. After all this is what
> > LSMs are already doing. Just exposing this to userspace for arbitration is
> > what I have a problem with.
>
> There's one more usecase that I'd like to explore: providing coherent
> view of host filesystem in virtualized environments. This requires
> that guest is synchronously notified when the host filesystem changes.
> I do agree, however, that adding sync hooks to userspace is
> problematic.
>
> One idea would be to use shared memory instead of a procedural
> notification. I.e. application (hypervisor) registers a pointer to a
> version number that the kernel associates with the given inode. When
> the inode is changed, then the version number is incremented. The
> guest kernel can then look at the version number when verifying cache
> validity. That way perfect coherency is guaranteed between host and
> guest filesystems without allowing a broken guest or even a broken
> hypervisor to DoS the host.
Well, statx() and looking at i_version can do this for you. So I guess
that's too slow for your purposes? Also how many inodes do you want to
monitor like this?
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* Re: fanotify and LSM path hooks
From: Miklos Szeredi @ 2019-04-17 12:14 UTC (permalink / raw)
To: Jan Kara
Cc: Amir Goldstein, linux-fsdevel, Al Viro, Matthew Bobrowski,
LSM List, overlayfs
In-Reply-To: <20190417113012.GC26435@quack2.suse.cz>
On Wed, Apr 17, 2019 at 1:30 PM Jan Kara <jack@suse.cz> wrote:
>
> On Tue 16-04-19 21:24:44, Amir Goldstein wrote:
> > > I'm not so sure about directory pre-modification hooks. Given the amount of
> > > problems we face with applications using fanotify permission events and
> > > deadlocking the system, I'm not very fond of expanding that API... AFAIU
> > > you want to use such hooks for recording (and persisting) that some change
> > > is going to happen and provide crash-consistency guarantees for such
> > > journal?
> > >
> >
> > That's the general idea.
> > I have two use cases for pre-modification hooks:
> > 1. VFS level snapshots
> > 2. persistent change tracking
> >
> > TBH, I did not consider implementing any of the above in userspace,
> > so I do not have a specific interest in extending the fanotify API.
> > I am actually interested in pre-modify fsnotify hooks (not fanotify),
> > that a snapshot or change tracking subsystem can register with.
> > An in-kernel fsnotify event handler can set a flag in current task
> > struct to circumvent system deadlocks on nested filesystem access.
>
> OK, I'm not opposed to fsnotify pre-modify hooks as such. As long as
> handlers stay within the kernel, I'm fine with that. After all this is what
> LSMs are already doing. Just exposing this to userspace for arbitration is
> what I have a problem with.
There's one more usecase that I'd like to explore: providing coherent
view of host filesystem in virtualized environments. This requires
that guest is synchronously notified when the host filesystem changes.
I do agree, however, that adding sync hooks to userspace is
problematic.
One idea would be to use shared memory instead of a procedural
notification. I.e. application (hypervisor) registers a pointer to a
version number that the kernel associates with the given inode. When
the inode is changed, then the version number is incremented. The
guest kernel can then look at the version number when verifying cache
validity. That way perfect coherency is guaranteed between host and
guest filesystems without allowing a broken guest or even a broken
hypervisor to DoS the host.
Thanks,
Miklos
^ permalink raw reply
* Re: fanotify and LSM path hooks
From: Jan Kara @ 2019-04-17 11:30 UTC (permalink / raw)
To: Amir Goldstein
Cc: Jan Kara, linux-fsdevel, Al Viro, Miklos Szeredi,
Matthew Bobrowski, LSM List, overlayfs
In-Reply-To: <CAOQ4uxh66kAozqseiEokqM3wDJws7=cnY-aFXH_0515nvsi2-A@mail.gmail.com>
On Tue 16-04-19 21:24:44, Amir Goldstein wrote:
> > I'm not so sure about directory pre-modification hooks. Given the amount of
> > problems we face with applications using fanotify permission events and
> > deadlocking the system, I'm not very fond of expanding that API... AFAIU
> > you want to use such hooks for recording (and persisting) that some change
> > is going to happen and provide crash-consistency guarantees for such
> > journal?
> >
>
> That's the general idea.
> I have two use cases for pre-modification hooks:
> 1. VFS level snapshots
> 2. persistent change tracking
>
> TBH, I did not consider implementing any of the above in userspace,
> so I do not have a specific interest in extending the fanotify API.
> I am actually interested in pre-modify fsnotify hooks (not fanotify),
> that a snapshot or change tracking subsystem can register with.
> An in-kernel fsnotify event handler can set a flag in current task
> struct to circumvent system deadlocks on nested filesystem access.
OK, I'm not opposed to fsnotify pre-modify hooks as such. As long as
handlers stay within the kernel, I'm fine with that. After all this is what
LSMs are already doing. Just exposing this to userspace for arbitration is
what I have a problem with.
> My current implementation of overlayfs snapshots [1] uses a stackable
> filesystem (a.k.a. snapshot fs) as a means for pre-modify hooks.
> This approach has some advantages and some disadvantages
> compared to fsnotify pre-modify hooks.
>
> With fsnotify pre-modify hooks it would be possible to protect
> the underlying filesystem from un-monitored changes even when
> filesystem is accessed from another mount point or another namespace.
>
> As a matter of fact, the protection to underlying filesystem changes
> needed for overlayfs snapshots is also useful for standard overlayfs -
> Modification to underlying overlayfs layers are strongly discouraged,
> but there is nothing preventing the user from making such modifications.
> If overlayfs were to register for fsnotify pre-modify hooks on underlying
> filesystem, it could prevent users from modifying underlying layers.
>
> And not only that - because security_inode_rename() hook is called
> with s_vfs_rename_mutex held, it is safe to use d_ancestor() to
> prevent renames in and out of overlay layer subtrees.
> With that protection in place, it is safe (?) to use is_subdir() in other
> hooks to check if an object is under an overlayfs layer subtree,
> because the entire ancestry below the layers roots is stable.
Uf, OK, should be. But it looks subtle. Not sure what Al will say about it
;).
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* Re: [PATCH] mm: security: introduce CONFIG_INIT_HEAP_ALL
From: Alexander Potapenko @ 2019-04-17 11:03 UTC (permalink / raw)
To: Christopher Lameter
Cc: Andrew Morton, linux-security-module,
Linux Memory Management List, Nick Desaulniers, Kostya Serebryany,
Dmitriy Vyukov, Kees Cook, Sandeep Patil, Laura Abbott,
Kernel Hardening
In-Reply-To: <0100016a26fc7605-a9c76ac4-387c-47a3-8c53-a8d208eb0925-000000@email.amazonses.com>
On Tue, Apr 16, 2019 at 6:30 PM Christopher Lameter <cl@linux.com> wrote:
>
> On Tue, 16 Apr 2019, Alexander Potapenko wrote:
>
> > > Hmmm... But we already have debugging options that poison objects and
> > > pages?
> > Laura Abbott mentioned in one of the previous threads
> > (https://marc.info/?l=kernel-hardening&m=155474181528491&w=2) that:
> >
> > """
> > I've looked at doing something similar in the past (failing to find
> > the thread this morning...) and while this will work, it has pretty
> > serious performance issues. It's not actually the poisoning which
> > is expensive but that turning on debugging removes the cpu slab
> > which has significant performance penalties.
>
> Ok you could rework that logic to be able to keep the per cpu slabs?
I'll look into that. There's a lot going on with checking those
poisoned bytes, although we don't need that for hardening.
What do you think about the proposed approach to page initialization?
We could separate that part from slab poisoning.
> Also if you do the zeroing then you need to do it in the hotpath. And this
> patch introduces new instructions to that hotpath for checking and
> executing the zeroing.
Right now the patch doesn't slow down the default case when
CONFIG_INIT_HEAP_ALL=n, as GFP_INIT_ALWAYS_ON is 0.
In the case heap initialization is enabled we could probably omit the
gfp_flags check, as it'll be always zero in the case there's a
constructor or RCU flag is set.
So we'll have two branches instead of one in the case CONFIG_INIT_HEAP_ALL=y.
--
Alexander Potapenko
Software Engineer
Google Germany GmbH
Erika-Mann-Straße, 33
80636 München
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
^ permalink raw reply
* [PATCH v20 16/28] x86/sgx: Add provisioning
From: Jarkko Sakkinen @ 2019-04-17 10:39 UTC (permalink / raw)
To: linux-kernel, x86, linux-sgx
Cc: akpm, dave.hansen, sean.j.christopherson, nhorman, npmccallum,
serge.ayoun, shay.katz-zamir, haitao.huang, andriy.shevchenko,
tglx, kai.svahn, bp, josh, luto, kai.huang, rientjes,
Jarkko Sakkinen, James Morris, Serge E . Hallyn,
linux-security-module
In-Reply-To: <20190417103938.7762-1-jarkko.sakkinen@linux.intel.com>
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 6250 bytes --]
In order to provide a mechanism for devilering provisoning rights:
1. Add a new device file /dev/sgx/provision that works as a token for
allowing an enclave to have the provisioning privileges.
2. Add a new ioctl called SGX_IOC_ENCLAVE_SET_ATTRIBUTE that accepts the
following data structure:
struct sgx_enclave_set_attribute {
__u64 addr;
__u64 attribute_fd;
};
A daemon could sit on top of /dev/sgx/provision and send a file
descriptor of this file to a process that needs to be able to provision
enclaves.
The way this API is used is straight-forward. Lets assume that dev_fd is
a handle to /dev/sgx/enclave and prov_fd is a handle to
/dev/sgx/provision. You would allow SGX_IOC_ENCLAVE_CREATE to
initialize an enclave with the PROVISIONKEY attribute by
params.addr = <enclave address>;
params.token_fd = prov_fd;
ioctl(dev_fd, SGX_IOC_ENCLAVE_SET_ATTRIBUTE, ¶ms);
Cc: James Morris <jmorris@namei.org>
Cc: Serge E. Hallyn <serge@hallyn.com>
Cc: linux-security-module@vger.kernel.org
Suggested-by: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
arch/x86/include/uapi/asm/sgx.h | 11 ++++++
arch/x86/kernel/cpu/sgx/driver/driver.h | 2 +-
arch/x86/kernel/cpu/sgx/driver/ioctl.c | 51 +++++++++++++++++++++++++
arch/x86/kernel/cpu/sgx/driver/main.c | 11 +++++-
4 files changed, 73 insertions(+), 2 deletions(-)
diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
index 7bf627ac4958..3b80acde8671 100644
--- a/arch/x86/include/uapi/asm/sgx.h
+++ b/arch/x86/include/uapi/asm/sgx.h
@@ -16,6 +16,8 @@
_IOW(SGX_MAGIC, 0x01, struct sgx_enclave_add_page)
#define SGX_IOC_ENCLAVE_INIT \
_IOW(SGX_MAGIC, 0x02, struct sgx_enclave_init)
+#define SGX_IOC_ENCLAVE_SET_ATTRIBUTE \
+ _IOW(SGX_MAGIC, 0x03, struct sgx_enclave_set_attribute)
/* IOCTL return values */
#define SGX_POWER_LOST_ENCLAVE 0x40000000
@@ -54,4 +56,13 @@ struct sgx_enclave_init {
__u64 sigstruct;
};
+/**
+ * struct sgx_enclave_set_attribute - parameter structure for the
+ * %SGX_IOC_ENCLAVE_SET_ATTRIBUTE ioctl
+ * @attribute_fd: file handle of the attribute file in the securityfs
+ */
+struct sgx_enclave_set_attribute {
+ __u64 attribute_fd;
+};
+
#endif /* _UAPI_ASM_X86_SGX_H */
diff --git a/arch/x86/kernel/cpu/sgx/driver/driver.h b/arch/x86/kernel/cpu/sgx/driver/driver.h
index 507712eb0a68..153b4a48aa6f 100644
--- a/arch/x86/kernel/cpu/sgx/driver/driver.h
+++ b/arch/x86/kernel/cpu/sgx/driver/driver.h
@@ -31,7 +31,7 @@ extern u64 sgx_attributes_reserved_mask;
extern u64 sgx_xfrm_reserved_mask;
extern u32 sgx_xsave_size_tbl[64];
-extern const struct file_operations sgx_fs_provision_fops;
+extern const struct file_operations sgx_provision_fops;
long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg);
diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index f88226518b21..65c9fb7b2a95 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -714,6 +714,54 @@ static long sgx_ioc_enclave_init(struct file *filep, unsigned int cmd,
return ret;
}
+/**
+ * sgx_ioc_enclave_set_attribute - handler for %SGX_IOC_ENCLAVE_SET_ATTRIBUTE
+ * @filep: open file to /dev/sgx
+ * @cmd: the command value
+ * @arg: pointer to a struct sgx_enclave_set_attribute instance
+ *
+ * Mark the enclave as being allowed to access a restricted attribute bit.
+ * The requested attribute is specified via the attribute_fd field in the
+ * provided struct sgx_enclave_set_attribute. The attribute_fd must be a
+ * handle to an SGX attribute file, e.g. “/dev/sgx/provision".
+ *
+ * Failure to explicitly request access to a restricted attribute will cause
+ * sgx_ioc_enclave_init() to fail. Currently, the only restricted attribute
+ * is access to the PROVISION_KEY.
+ *
+ * Note, access to the EINITTOKEN_KEY is disallowed entirely.
+ *
+ * Return: 0 on success, -errno otherwise
+ */
+static long sgx_ioc_enclave_set_attribute(struct file *filep, unsigned int cmd,
+ unsigned long arg)
+{
+ struct sgx_enclave_set_attribute *params = (void *)arg;
+ struct file *attribute_file;
+ struct sgx_encl *encl;
+ int ret;
+
+ attribute_file = fget(params->attribute_fd);
+ if (!attribute_file->f_op)
+ return -EINVAL;
+
+ if (attribute_file->f_op != &sgx_provision_fops) {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ ret = sgx_encl_get(params->addr, &encl);
+ if (ret)
+ goto out;
+
+ encl->allowed_attributes |= SGX_ATTR_PROVISIONKEY;
+ kref_put(&encl->refcount, sgx_encl_release);
+
+out:
+ fput(attribute_file);
+ return ret;
+}
+
typedef long (*sgx_ioc_t)(struct file *filep, unsigned int cmd,
unsigned long arg);
@@ -733,6 +781,9 @@ long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
case SGX_IOC_ENCLAVE_INIT:
handler = sgx_ioc_enclave_init;
break;
+ case SGX_IOC_ENCLAVE_SET_ATTRIBUTE:
+ handler = sgx_ioc_enclave_set_attribute;
+ break;
default:
return -ENOIOCTLCMD;
}
diff --git a/arch/x86/kernel/cpu/sgx/driver/main.c b/arch/x86/kernel/cpu/sgx/driver/main.c
index d371add399cd..8b79c4a60037 100644
--- a/arch/x86/kernel/cpu/sgx/driver/main.c
+++ b/arch/x86/kernel/cpu/sgx/driver/main.c
@@ -103,7 +103,7 @@ static const struct file_operations sgx_encl_fops = {
.get_unmapped_area = sgx_get_unmapped_area,
};
-static const struct file_operations sgx_provision_fops = {
+const struct file_operations sgx_provision_fops = {
.owner = THIS_MODULE,
};
@@ -261,8 +261,16 @@ static int sgx_dev_init(struct device *parent)
if (ret)
goto err_encl_dev_add;
+ ret = cdev_device_add(&sgx_dev->provision_cdev,
+ &sgx_dev->provision_dev);
+ if (ret)
+ goto err_provision_dev_add;
+
return 0;
+err_provision_dev_add:
+ cdev_device_del(&sgx_dev->encl_cdev, &sgx_dev->encl_dev);
+
err_encl_dev_add:
destroy_workqueue(sgx_encl_wq);
@@ -289,6 +297,7 @@ static int sgx_drv_remove(struct platform_device *pdev)
struct sgx_dev_ctx *ctx = dev_get_drvdata(&pdev->dev);
cdev_device_del(&ctx->encl_cdev, &ctx->encl_dev);
+ cdev_device_del(&ctx->provision_cdev, &ctx->provision_dev);
destroy_workqueue(sgx_encl_wq);
return 0;
--
2.19.1
^ permalink raw reply related
* Re: [RFC PATCH v1 1/5] fs: Add support for an O_MAYEXEC flag on sys_open()
From: Florian Weimer @ 2019-04-17 10:01 UTC (permalink / raw)
To: Steve Grubb
Cc: Jan Kara, Mickaël Salaün, linux-kernel, Al Viro,
James Morris, Jonathan Corbet, Kees Cook, Matthew Garrett,
Michael Kerrisk, Mickaël Salaün, Mimi Zohar,
Philippe Trébuchet, Shuah Khan, Thibaut Sautereau,
Vincent Strubel, Yves-Alexis Perez, kernel-hardening, linux-api,
linux-security-module, linux-fsdevel, Matthew Bobrowski
In-Reply-To: <2361288.r9sZr2NHLB@x2>
* Steve Grubb:
> On Tuesday, April 16, 2019 7:49:39 AM EDT Florian Weimer wrote:
>> * Steve Grubb:
>> > This flag that is being proposed means that you would have to patch all
>> > interpreters to use it. If you are sure that upstreams will accept that,
>> > why not just change the policy to interpreters shouldn't execute
>> > anything unless the execute bit is set? That is simpler and doesn't need
>> > a kernel change. And setting the execute bit is an auditable event.
>>
>> I think we need something like O_MAYEXEC so that security policies can
>> be enforced and noexec mounts can be detected.
>
> Application whitelisting can already today stop unknown software without
> needing O_MAYEXEC.
I'm somewhat interested in using this to add a proper check for
executability to explicit dynamic loader invocations. In other words,
this
/lib64/ld-linux-x86-64.so.2 /path/to/noexec/fs/program
should refuse to run the program if the program is located on a file
system mounted with the noexec attribute.
> The problem is that passing O_MAYEXEC is opt-in. You can use ptrace/seccomp/
> bpf/LD_PRELOAD/LD_AUDIT to remove that bit from an otherwise normal program.
> This does not require privs to do so.
That doesn't really help with the above.
> But let's consider that this comes to pass and every interpreter is
> updated and IMA can see the O_MAYEXEC flag. Attackers now simply pivot
> to running programs via stdin. It never touches disk and therefore
> nothing enforces security policy. This already is among the most
> common ways that malware runs today to evade detection.
Are you referring to Windows malware using Powershell?
I'm not sure this is applicable to Linux. We do not have much
behavioral monitoring anyway.
Thanks,
Florian
^ permalink raw reply
* Re: [PATCH] crypto: testmgr - allocate buffers with __GFP_COMP
From: Robin Murphy @ 2019-04-17 9:54 UTC (permalink / raw)
To: Russell King - ARM Linux admin, Matthew Wilcox
Cc: Kees Cook, Rik van Riel, Linux Kernel Mailing List, Eric Biggers,
Linux-MM, linux-security-module, Geert Uytterhoeven, linux-crypto,
Dmitry Vyukov, Laura Abbott, Linux ARM, Herbert Xu
In-Reply-To: <20190417080919.54wywpzrt3psn4vj@shell.armlinux.org.uk>
On 17/04/2019 09:09, Russell King - ARM Linux admin wrote:
> On Tue, Apr 16, 2019 at 09:08:22PM -0700, Matthew Wilcox wrote:
>> On Mon, Apr 15, 2019 at 10:14:51PM -0500, Kees Cook wrote:
>>> On Mon, Apr 15, 2019 at 9:18 PM Matthew Wilcox <willy@infradead.org> wrote:
>>>> I agree; if the crypto code is never going to try to go from the address of
>>>> a byte in the allocation back to the head page, then there's no need to
>>>> specify GFP_COMP.
>>>>
>>>> But that leaves us in the awkward situation where
>>>> HARDENED_USERCOPY_PAGESPAN does need to be able to figure out whether
>>>> 'ptr + n - 1' lies within the same allocation as ptr. Without using
>>>> a compound page, there's no indication in the VM structures that these
>>>> two pages were allocated as part of the same allocation.
>>>>
>>>> We could force all multi-page allocations to be compound pages if
>>>> HARDENED_USERCOPY_PAGESPAN is enabled, but I worry that could break
>>>> something. We could make it catch fewer problems by succeeding if the
>>>> page is not compound. I don't know, these all seem like bad choices
>>>> to me.
>>>
>>> If GFP_COMP is _not_ the correct signal about adjacent pages being
>>> part of the same allocation, then I agree: we need to drop this check
>>> entirely from PAGESPAN. Is there anything else that indicates this
>>> property? (Or where might we be able to store that info?)
>>
>> As far as I know, the page allocator does not store size information
>> anywhere, unless you use GFP_COMP. That's why you have to pass
>> the 'order' to free_pages() and __free_pages(). It's also why
>> alloc_pages_exact() works (follow all the way into split_page()).
>>
>>> There are other pagespan checks, though, so those could stay. But I'd
>>> really love to gain page allocator allocation size checking ...
>>
>> I think that's a great idea, but I'm not sure how you'll be able to
>> do that.
>
> However, we have had code (maybe historically now) that has allocated
> a higher order page and then handed back pages that it doesn't need -
> for example, when the code requires multiple contiguous pages but does
> not require a power-of-2 size of contiguous pages.
'git grep alloc_pages_exact' suggests it's not historical yet...
Robin.
^ permalink raw reply
* Re: [PATCH] crypto: testmgr - allocate buffers with __GFP_COMP
From: Russell King - ARM Linux admin @ 2019-04-17 8:09 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Kees Cook, Linux ARM, Herbert Xu, Rik van Riel,
Linux Kernel Mailing List, Eric Biggers, Linux-MM,
linux-security-module, Geert Uytterhoeven, linux-crypto,
Laura Abbott, Dmitry Vyukov
In-Reply-To: <20190417040822.GB7751@bombadil.infradead.org>
On Tue, Apr 16, 2019 at 09:08:22PM -0700, Matthew Wilcox wrote:
> On Mon, Apr 15, 2019 at 10:14:51PM -0500, Kees Cook wrote:
> > On Mon, Apr 15, 2019 at 9:18 PM Matthew Wilcox <willy@infradead.org> wrote:
> > > I agree; if the crypto code is never going to try to go from the address of
> > > a byte in the allocation back to the head page, then there's no need to
> > > specify GFP_COMP.
> > >
> > > But that leaves us in the awkward situation where
> > > HARDENED_USERCOPY_PAGESPAN does need to be able to figure out whether
> > > 'ptr + n - 1' lies within the same allocation as ptr. Without using
> > > a compound page, there's no indication in the VM structures that these
> > > two pages were allocated as part of the same allocation.
> > >
> > > We could force all multi-page allocations to be compound pages if
> > > HARDENED_USERCOPY_PAGESPAN is enabled, but I worry that could break
> > > something. We could make it catch fewer problems by succeeding if the
> > > page is not compound. I don't know, these all seem like bad choices
> > > to me.
> >
> > If GFP_COMP is _not_ the correct signal about adjacent pages being
> > part of the same allocation, then I agree: we need to drop this check
> > entirely from PAGESPAN. Is there anything else that indicates this
> > property? (Or where might we be able to store that info?)
>
> As far as I know, the page allocator does not store size information
> anywhere, unless you use GFP_COMP. That's why you have to pass
> the 'order' to free_pages() and __free_pages(). It's also why
> alloc_pages_exact() works (follow all the way into split_page()).
>
> > There are other pagespan checks, though, so those could stay. But I'd
> > really love to gain page allocator allocation size checking ...
>
> I think that's a great idea, but I'm not sure how you'll be able to
> do that.
However, we have had code (maybe historically now) that has allocated
a higher order page and then handed back pages that it doesn't need -
for example, when the code requires multiple contiguous pages but does
not require a power-of-2 size of contiguous pages.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* Re: [PATCH] crypto: testmgr - allocate buffers with __GFP_COMP
From: Matthew Wilcox @ 2019-04-17 4:08 UTC (permalink / raw)
To: Kees Cook
Cc: Herbert Xu, Eric Biggers, Rik van Riel, linux-crypto,
Dmitry Vyukov, Geert Uytterhoeven, linux-security-module,
Linux ARM, Linux Kernel Mailing List, Laura Abbott, Linux-MM
In-Reply-To: <CAGXu5jKaVB=bTJCBWhsxAny7-OkzXQ+8KCd5O+_-7hKcJFiqKw@mail.gmail.com>
On Mon, Apr 15, 2019 at 10:14:51PM -0500, Kees Cook wrote:
> On Mon, Apr 15, 2019 at 9:18 PM Matthew Wilcox <willy@infradead.org> wrote:
> > I agree; if the crypto code is never going to try to go from the address of
> > a byte in the allocation back to the head page, then there's no need to
> > specify GFP_COMP.
> >
> > But that leaves us in the awkward situation where
> > HARDENED_USERCOPY_PAGESPAN does need to be able to figure out whether
> > 'ptr + n - 1' lies within the same allocation as ptr. Without using
> > a compound page, there's no indication in the VM structures that these
> > two pages were allocated as part of the same allocation.
> >
> > We could force all multi-page allocations to be compound pages if
> > HARDENED_USERCOPY_PAGESPAN is enabled, but I worry that could break
> > something. We could make it catch fewer problems by succeeding if the
> > page is not compound. I don't know, these all seem like bad choices
> > to me.
>
> If GFP_COMP is _not_ the correct signal about adjacent pages being
> part of the same allocation, then I agree: we need to drop this check
> entirely from PAGESPAN. Is there anything else that indicates this
> property? (Or where might we be able to store that info?)
As far as I know, the page allocator does not store size information
anywhere, unless you use GFP_COMP. That's why you have to pass
the 'order' to free_pages() and __free_pages(). It's also why
alloc_pages_exact() works (follow all the way into split_page()).
> There are other pagespan checks, though, so those could stay. But I'd
> really love to gain page allocator allocation size checking ...
I think that's a great idea, but I'm not sure how you'll be able to
do that.
^ permalink raw reply
* Re: [PATCH] apparmor: fix spelling mistake "immutible" -> "immutable"
From: John Johansen @ 2019-04-16 19:56 UTC (permalink / raw)
To: Colin King, James Morris, Serge E . Hallyn, linux-security-module
Cc: kernel-janitors, linux-kernel
In-Reply-To: <20190416144218.1842-1-colin.king@canonical.com>
On 4/16/19 7:42 AM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> There is a spelling mistake in an information message string, fix it.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
Acked-by: John Johansen <john.johansen@canonical.com>
I'll pull it into the apparmor tree
> ---
> security/apparmor/policy.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c
> index 71a3e6291478..04f2480e8374 100644
> --- a/security/apparmor/policy.c
> +++ b/security/apparmor/policy.c
> @@ -587,7 +587,7 @@ static int replacement_allowed(struct aa_profile *profile, int noreplace,
> {
> if (profile) {
> if (profile->label.flags & FLAG_IMMUTIBLE) {
> - *info = "cannot replace immutible profile";
> + *info = "cannot replace immutable profile";
> return -EPERM;
> } else if (noreplace) {
> *info = "profile already exists";
>
^ permalink raw reply
* Re: fanotify and LSM path hooks
From: Amir Goldstein @ 2019-04-16 18:24 UTC (permalink / raw)
To: Jan Kara
Cc: linux-fsdevel, Al Viro, Miklos Szeredi, Matthew Bobrowski,
LSM List, overlayfs
In-Reply-To: <20190416154513.GB13422@quack2.suse.cz>
On Tue, Apr 16, 2019 at 6:45 PM Jan Kara <jack@suse.cz> wrote:
>
> Hi Amir!
>
> On Sun 14-04-19 19:04:14, Amir Goldstein wrote:
> > I started to look at directory pre-modification "permission" hooks
> > that were discussed on last year's LSFMM:
> > https://lwn.net/Articles/755277/
> >
> > The two existing fanotify_perm() hooks are called
> > from security_file_permission() and security_file_open()
> > and depend on build time CONFIG_SECURITY.
> > If you look at how the fsnotify_perm() hooks are planted inside the
> > generic security hooks, one might wonder, why are fanotify permission
> > hooks getting a special treatment and are not registering as LSM hooks?
> >
> > One benefit from an fanotify LSM, besides more generic code, would be
> > that fanotify permission hooks could be disabled with boot parameters.
> >
> > I only bring this up because security hooks seems like the most natural
> > place to add pre-modify fanotify events for the purpose of maintaining
> > a filesystem change journal. It would be ugly to spray more fsnotify hooks
> > inside security hooks instead of registering an fanotify LSM, but maybe
> > there are downsides of registering fanotify as LSM that I am not aware of?
>
> I kind of like the idea of generating fanotify permission events from
> special LSM hooks.
>
Cool, I think that all we really need to do is call security_add_hooks().
[reducing LSM CC list]
> I'm not so sure about directory pre-modification hooks. Given the amount of
> problems we face with applications using fanotify permission events and
> deadlocking the system, I'm not very fond of expanding that API... AFAIU
> you want to use such hooks for recording (and persisting) that some change
> is going to happen and provide crash-consistency guarantees for such
> journal?
>
That's the general idea.
I have two use cases for pre-modification hooks:
1. VFS level snapshots
2. persistent change tracking
TBH, I did not consider implementing any of the above in userspace,
so I do not have a specific interest in extending the fanotify API.
I am actually interested in pre-modify fsnotify hooks (not fanotify),
that a snapshot or change tracking subsystem can register with.
An in-kernel fsnotify event handler can set a flag in current task
struct to circumvent system deadlocks on nested filesystem access.
My current implementation of overlayfs snapshots [1] uses a stackable
filesystem (a.k.a. snapshot fs) as a means for pre-modify hooks.
This approach has some advantages and some disadvantages
compared to fsnotify pre-modify hooks.
With fsnotify pre-modify hooks it would be possible to protect
the underlying filesystem from un-monitored changes even when
filesystem is accessed from another mount point or another namespace.
As a matter of fact, the protection to underlying filesystem changes
needed for overlayfs snapshots is also useful for standard overlayfs -
Modification to underlying overlayfs layers are strongly discouraged,
but there is nothing preventing the user from making such modifications.
If overlayfs were to register for fsnotify pre-modify hooks on underlying
filesystem, it could prevent users from modifying underlying layers.
And not only that - because security_inode_rename() hook is called
with s_vfs_rename_mutex held, it is safe to use d_ancestor() to
prevent renames in and out of overlay layer subtrees.
With that protection in place, it is safe (?) to use is_subdir() in other
hooks to check if an object is under an overlayfs layer subtree,
because the entire ancestry below the layers roots is stable.
Will see if I can sketch a POC.
Thanks,
Amir.
[1] https://github.com/amir73il/overlayfs/wiki/Overlayfs-snapshots
^ permalink raw reply
* Re: [PATCH] mm: security: introduce CONFIG_INIT_HEAP_ALL
From: Christopher Lameter @ 2019-04-16 16:30 UTC (permalink / raw)
To: Alexander Potapenko
Cc: Andrew Morton, linux-security-module,
Linux Memory Management List, Nick Desaulniers, Kostya Serebryany,
Dmitriy Vyukov, Kees Cook, Sandeep Patil, Laura Abbott,
Kernel Hardening
In-Reply-To: <CAG_fn=U6aWfBXdkcWs0_1pqggAC16Yg8Q6rxLiVeiO83q1hOCw@mail.gmail.com>
On Tue, 16 Apr 2019, Alexander Potapenko wrote:
> > Hmmm... But we already have debugging options that poison objects and
> > pages?
> Laura Abbott mentioned in one of the previous threads
> (https://marc.info/?l=kernel-hardening&m=155474181528491&w=2) that:
>
> """
> I've looked at doing something similar in the past (failing to find
> the thread this morning...) and while this will work, it has pretty
> serious performance issues. It's not actually the poisoning which
> is expensive but that turning on debugging removes the cpu slab
> which has significant performance penalties.
Ok you could rework that logic to be able to keep the per cpu slabs?
Also if you do the zeroing then you need to do it in the hotpath. And this
patch introduces new instructions to that hotpath for checking and
executing the zeroing.
^ permalink raw reply
* Re: [PATCH] mm: security: introduce CONFIG_INIT_HEAP_ALL
From: Alexander Potapenko @ 2019-04-16 16:01 UTC (permalink / raw)
To: Christopher Lameter
Cc: Andrew Morton, linux-security-module,
Linux Memory Management List, Nick Desaulniers, Kostya Serebryany,
Dmitriy Vyukov, Kees Cook, Sandeep Patil, Laura Abbott,
Kernel Hardening
In-Reply-To: <0100016a26c711be-b99971ca-49f5-482c-9028-962ee471f733-000000@email.amazonses.com>
On Tue, Apr 16, 2019 at 5:32 PM Christopher Lameter <cl@linux.com> wrote:
>
> On Fri, 12 Apr 2019, Alexander Potapenko wrote:
>
> > diff --git a/mm/slab.h b/mm/slab.h
> > index 43ac818b8592..4bb10af0031b 100644
> > --- a/mm/slab.h
> > +++ b/mm/slab.h
> > @@ -167,6 +167,16 @@ static inline slab_flags_t kmem_cache_flags(unsigned int object_size,
> > SLAB_TEMPORARY | \
> > SLAB_ACCOUNT)
> >
> > +/*
> > + * Do we need to initialize this allocation?
> > + * Always true for __GFP_ZERO, CONFIG_INIT_HEAP_ALL enforces initialization
> > + * of caches without constructors and RCU.
> > + */
> > +#define SLAB_WANT_INIT(cache, gfp_flags) \
> > + ((GFP_INIT_ALWAYS_ON && !(cache)->ctor && \
> > + !((cache)->flags & SLAB_TYPESAFE_BY_RCU)) || \
> > + (gfp_flags & __GFP_ZERO))
>
> This is another complex thing to maintain when adding flags to the slab
> allocator.
>
> > +config INIT_HEAP_ALL
> > + bool "Initialize kernel heap allocations"
>
> "Zero pages and objects allocated in the kernel"
>
> > + default n
> > + help
> > + Enforce initialization of pages allocated from page allocator
> > + and objects returned by kmalloc and friends.
> > + Allocated memory is initialized with zeroes, preventing possible
> > + information leaks and making the control-flow bugs that depend
> > + on uninitialized values more deterministic.
>
> Hmmm... But we already have debugging options that poison objects and
> pages?
Laura Abbott mentioned in one of the previous threads
(https://marc.info/?l=kernel-hardening&m=155474181528491&w=2) that:
"""
I've looked at doing something similar in the past (failing to find
the thread this morning...) and while this will work, it has pretty
serious performance issues. It's not actually the poisoning which
is expensive but that turning on debugging removes the cpu slab
which has significant performance penalties.
I'd rather go back to the proposal of just poisoning the slab
at alloc/free without using SLAB_POISON.
"""
, so slab poisoning is probably off the table.
--
Alexander Potapenko
Software Engineer
Google Germany GmbH
Erika-Mann-Straße, 33
80636 München
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
^ permalink raw reply
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