* Re: [Non-DoD Source] Re: [PATCH] fanotify, inotify, dnotify, security: add security hook for fs notifications
From: Aaron Goidel @ 2019-08-09 16:25 UTC (permalink / raw)
To: Amir Goldstein, Paul Moore
Cc: selinux, LSM List, linux-fsdevel, David Howells, Jan Kara,
James Morris, Stephen Smalley, linux-kernel
In-Reply-To: <CAOQ4uxigYZunXgq0BubRFNM51Kh_g3wrtyNH77PozUX+3sM=aQ@mail.gmail.com>
On 8/9/19 5:06 AM, Amir Goldstein wrote:
> On Thu, Aug 8, 2019 at 9:33 PM Paul Moore <paul@paul-moore.com> wrote:
>>
>> On Wed, Jul 31, 2019 at 11:35 AM Aaron Goidel <acgoide@tycho.nsa.gov> wrote:
>>> As of now, setting watches on filesystem objects has, at most, applied a
>>> check for read access to the inode, and in the case of fanotify, requires
>>> CAP_SYS_ADMIN. No specific security hook or permission check has been
>>> provided to control the setting of watches. Using any of inotify, dnotify,
>>> or fanotify, it is possible to observe, not only write-like operations, but
>>> even read access to a file. Modeling the watch as being merely a read from
>>> the file is insufficient for the needs of SELinux. This is due to the fact
>>> that read access should not necessarily imply access to information about
>>> when another process reads from a file. Furthermore, fanotify watches grant
>>> more power to an application in the form of permission events. While
>>> notification events are solely, unidirectional (i.e. they only pass
>>> information to the receiving application), permission events are blocking.
>>> Permission events make a request to the receiving application which will
>>> then reply with a decision as to whether or not that action may be
>>> completed. This causes the issue of the watching application having the
>>> ability to exercise control over the triggering process. Without drawing a
>>> distinction within the permission check, the ability to read would imply
>>> the greater ability to control an application. Additionally, mount and
>>> superblock watches apply to all files within the same mount or superblock.
>>> Read access to one file should not necessarily imply the ability to watch
>>> all files accessed within a given mount or superblock.
>>>
>>> In order to solve these issues, a new LSM hook is implemented and has been
>>> placed within the system calls for marking filesystem objects with inotify,
>>> fanotify, and dnotify watches. These calls to the hook are placed at the
>>> point at which the target path has been resolved and are provided with the
>>> path struct, the mask of requested notification events, and the type of
>>> object on which the mark is being set (inode, superblock, or mount). The
>>> mask and obj_type have already been translated into common FS_* values
>>> shared by the entirety of the fs notification infrastructure. The path
>>> struct is passed rather than just the inode so that the mount is available,
>>> particularly for mount watches. This also allows for use of the hook by
>>> pathname-based security modules. However, since the hook is intended for
>>> use even by inode based security modules, it is not placed under the
>>> CONFIG_SECURITY_PATH conditional. Otherwise, the inode-based security
>>> modules would need to enable all of the path hooks, even though they do not
>>> use any of them.
>>>
>>> This only provides a hook at the point of setting a watch, and presumes
>>> that permission to set a particular watch implies the ability to receive
>>> all notification about that object which match the mask. This is all that
>>> is required for SELinux. If other security modules require additional hooks
>>> or infrastructure to control delivery of notification, these can be added
>>> by them. It does not make sense for us to propose hooks for which we have
>>> no implementation. The understanding that all notifications received by the
>>> requesting application are all strictly of a type for which the application
>>> has been granted permission shows that this implementation is sufficient in
>>> its coverage.
>>>
>>> Security modules wishing to provide complete control over fanotify must
>>> also implement a security_file_open hook that validates that the access
>>> requested by the watching application is authorized. Fanotify has the issue
>>> that it returns a file descriptor with the file mode specified during
>>> fanotify_init() to the watching process on event. This is already covered
>>> by the LSM security_file_open hook if the security module implements
>>> checking of the requested file mode there. Otherwise, a watching process
>>> can obtain escalated access to a file for which it has not been authorized.
>>>
>>> The selinux_path_notify hook implementation works by adding five new file
>>> permissions: watch, watch_mount, watch_sb, watch_reads, and watch_with_perm
>>> (descriptions about which will follow), and one new filesystem permission:
>>> watch (which is applied to superblock checks). The hook then decides which
>>> subset of these permissions must be held by the requesting application
>>> based on the contents of the provided mask and the obj_type. The
>>> selinux_file_open hook already checks the requested file mode and therefore
>>> ensures that a watching process cannot escalate its access through
>>> fanotify.
>>>
>>> The watch, watch_mount, and watch_sb permissions are the baseline
>>> permissions for setting a watch on an object and each are a requirement for
>>> any watch to be set on a file, mount, or superblock respectively. It should
>>> be noted that having either of the other two permissions (watch_reads and
>>> watch_with_perm) does not imply the watch, watch_mount, or watch_sb
>>> permission. Superblock watches further require the filesystem watch
>>> permission to the superblock. As there is no labeled object in view for
>>> mounts, there is no specific check for mount watches beyond watch_mount to
>>> the inode. Such a check could be added in the future, if a suitable labeled
>>> object existed representing the mount.
>>>
>>> The watch_reads permission is required to receive notifications from
>>> read-exclusive events on filesystem objects. These events include accessing
>>> a file for the purpose of reading and closing a file which has been opened
>>> read-only. This distinction has been drawn in order to provide a direct
>>> indication in the policy for this otherwise not obvious capability. Read
>>> access to a file should not necessarily imply the ability to observe read
>>> events on a file.
>>>
>>> Finally, watch_with_perm only applies to fanotify masks since it is the
>>> only way to set a mask which allows for the blocking, permission event.
>>> This permission is needed for any watch which is of this type. Though
>>> fanotify requires CAP_SYS_ADMIN, this is insufficient as it gives implicit
>>> trust to root, which we do not do, and does not support least privilege.
>>>
>>> Signed-off-by: Aaron Goidel <acgoide@tycho.nsa.gov>
>>> ---
>>> fs/notify/dnotify/dnotify.c | 15 +++++++--
>>> fs/notify/fanotify/fanotify_user.c | 27 +++++++++++++++--
>>> fs/notify/inotify/inotify_user.c | 13 ++++++--
>>> include/linux/lsm_hooks.h | 9 +++++-
>>> include/linux/security.h | 10 ++++--
>>> security/security.c | 6 ++++
>>> security/selinux/hooks.c | 47 +++++++++++++++++++++++++++++
>>> security/selinux/include/classmap.h | 5 +--
>>> 8 files changed, 120 insertions(+), 12 deletions(-)
>>
>> Other than Casey's comments, and ACK, I'm not seeing much commentary
>> on this patch so FS and LSM folks consider this your last chance - if
>> I don't hear any objections by the end of this week I'll plan on
>> merging this into selinux/next next week.
>
> Please consider it is summer time so people may be on vacation like I was...
>
> First a suggestion, take it or leave it.
> The name of the hook _notify() seems misleading to me.
> naming the hook security_path_watch() seems much more
> appropriate and matching the name of the constants FILE__WATCH
> used by selinux.
>
> Few more comments below..
>
>>
>>> diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
>>> index 250369d6901d..87a7f61bc91c 100644
>>> --- a/fs/notify/dnotify/dnotify.c
>>> +++ b/fs/notify/dnotify/dnotify.c
>>> @@ -22,6 +22,7 @@
>>> #include <linux/sched/signal.h>
>>> #include <linux/dnotify.h>
>>> #include <linux/init.h>
>>> +#include <linux/security.h>
>>> #include <linux/spinlock.h>
>>> #include <linux/slab.h>
>>> #include <linux/fdtable.h>
>>> @@ -288,6 +289,17 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
>>> goto out_err;
>>> }
>>>
>>> + /*
>>> + * convert the userspace DN_* "arg" to the internal FS_*
>>> + * defined in fsnotify
>>> + */
>>> + mask = convert_arg(arg);
>>> +
>>> + error = security_path_notify(&filp->f_path, mask,
>>> + FSNOTIFY_OBJ_TYPE_INODE);
>>> + if (error)
>>> + goto out_err;
>>> +
>>> /* expect most fcntl to add new rather than augment old */
>>> dn = kmem_cache_alloc(dnotify_struct_cache, GFP_KERNEL);
>>> if (!dn) {
>>> @@ -302,9 +314,6 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
>>> goto out_err;
>>> }
>>>
>>> - /* convert the userspace DN_* "arg" to the internal FS_* defines in fsnotify */
>>> - mask = convert_arg(arg);
>>> -
>>> /* set up the new_fsn_mark and new_dn_mark */
>>> new_fsn_mark = &new_dn_mark->fsn_mark;
>>> fsnotify_init_mark(new_fsn_mark, dnotify_group);
>>> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
>>> index a90bb19dcfa2..b83f27021f54 100644
>>> --- a/fs/notify/fanotify/fanotify_user.c
>>> +++ b/fs/notify/fanotify/fanotify_user.c
>>> @@ -528,9 +528,10 @@ static const struct file_operations fanotify_fops = {
>>> };
>>>
>>> static int fanotify_find_path(int dfd, const char __user *filename,
>>> - struct path *path, unsigned int flags)
>>> + struct path *path, unsigned int flags, __u64 mask)
>>> {
>>> int ret;
>>> + unsigned int obj_type;
>>>
>>> pr_debug("%s: dfd=%d filename=%p flags=%x\n", __func__,
>>> dfd, filename, flags);
>>> @@ -567,8 +568,30 @@ static int fanotify_find_path(int dfd, const char __user *filename,
>>>
>>> /* you can only watch an inode if you have read permissions on it */
>>> ret = inode_permission(path->dentry->d_inode, MAY_READ);
>>> + if (ret) {
>>> + path_put(path);
>>> + goto out;
>>> + }
>>> +
>>> + switch (flags & FANOTIFY_MARK_TYPE_BITS) {
>>> + case FAN_MARK_MOUNT:
>>> + obj_type = FSNOTIFY_OBJ_TYPE_VFSMOUNT;
>>> + break;
>>> + case FAN_MARK_FILESYSTEM:
>>> + obj_type = FSNOTIFY_OBJ_TYPE_SB;
>>> + break;
>>> + case FAN_MARK_INODE:
>>> + obj_type = FSNOTIFY_OBJ_TYPE_INODE;
>>> + break;
>>> + default:
>>> + ret = -EINVAL;
>>> + goto out;
>>> + }
>
> Sorry, I just can't stand this extra switch statement here.
> Please initialize obj_type at the very first switch statement in
> do_fanotify_mark() and pass it to fanotify_find_path().
> Preferably also make it a helper that returns either
> valid obj_type or <0 for error.
>
>
I have no problem moving the initialization of obj_type up one level to
do_fanotify_mark(). I don't think that a helper is necessary at this
juncture as this logic seems to only exist in one place. Should this
change, then there would be merit to having a separate function.
>>> +
>>> + ret = security_path_notify(path, mask, obj_type);
>>> if (ret)
>>> path_put(path);
>
> It is probably best to mask out FANOTIFY_EVENT_FLAGS
> when calling the hook. Although FAN_EVENT_ON_CHILD
> and FAN_ONDIR do map to corresponding FS_ constants,
> the security hooks from dnotify and inotify do not pass these
> flags, so the security module cannot use them as reliable
> information, so it will have to assume that they have been
> requested anyway.
>
> Alternatively, make sure that dnotify/inotify security hooks
> always set these two flags, by fixing up and using the
> dnotify/inotify arg_to_mask conversion helpers before calling
> the security hook.
>
I think that at this point either approach you mentioned makes just as
much sense. If it's all the same to you, Amir, I'll just change the
caller in fanotify to include (mask) & ~(FANOTIFY_EVENT_FLAGS)
>>> +
>>> out:
>>> return ret;
>>> }
>>> @@ -1014,7 +1037,7 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
>>> goto fput_and_out;
>>> }
>>>
>>> - ret = fanotify_find_path(dfd, pathname, &path, flags);
>>> + ret = fanotify_find_path(dfd, pathname, &path, flags, mask);
>>> if (ret)
>>> goto fput_and_out;
>>>
>>> diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
>>> index 7b53598c8804..e0d593c2d437 100644
>>> --- a/fs/notify/inotify/inotify_user.c
>>> +++ b/fs/notify/inotify/inotify_user.c
>>> @@ -39,6 +39,7 @@
>>> #include <linux/poll.h>
>>> #include <linux/wait.h>
>>> #include <linux/memcontrol.h>
>>> +#include <linux/security.h>
>>>
>>> #include "inotify.h"
>>> #include "../fdinfo.h"
>>> @@ -342,7 +343,8 @@ static const struct file_operations inotify_fops = {
>>> /*
>>> * find_inode - resolve a user-given path to a specific inode
>>> */
>>> -static int inotify_find_inode(const char __user *dirname, struct path *path, unsigned flags)
>>> +static int inotify_find_inode(const char __user *dirname, struct path *path,
>>> + unsigned int flags, __u64 mask)
>>> {
>>> int error;
>>>
>>> @@ -351,8 +353,15 @@ static int inotify_find_inode(const char __user *dirname, struct path *path, uns
>>> return error;
>>> /* you can only watch an inode if you have read permissions on it */
>>> error = inode_permission(path->dentry->d_inode, MAY_READ);
>>> + if (error) {
>>> + path_put(path);
>>> + return error;
>>> + }
>>> + error = security_path_notify(path, mask,
>>> + FSNOTIFY_OBJ_TYPE_INODE);
>>> if (error)
>>> path_put(path);
>>> +
>>> return error;
>>> }
>>>
>>> @@ -744,7 +753,7 @@ SYSCALL_DEFINE3(inotify_add_watch, int, fd, const char __user *, pathname,
>>> if (mask & IN_ONLYDIR)
>>> flags |= LOOKUP_DIRECTORY;
>>>
>>> - ret = inotify_find_inode(pathname, &path, flags);
>>> + ret = inotify_find_inode(pathname, &path, flags, mask);
>
> Please use (mask & IN_ALL_EVENTS) for converting to common FS_ flags
> or use the inotify_arg_to_mask() conversion helper, which contains more
> details irrelevant for the security hook.
> Otherwise mask may contain flags like IN_MASK_CREATE, which mean
> different things on different backends and the security module cannot tell
> the difference.
>
> Also note that at this point, before inotify_arg_to_mask(), the mask does
> not yet contain FS_EVENT_ON_CHILD, which could be interesting for
> the security hook (fanotify users can opt-in with FAN_EVENT_ON_CHILD).
> Not a big deal though as security hook can assume the worse
> (that events on child are requested).
>
I'll use (mask & IN_ALL_EVENTS).
> Thanks,
> Amir.
>
I can implement the changes in the ways I mentioned above. I don't see a
need for anything more in the cases you brought up since none of them
change the logic of the hook itself or would make a substantive
difference to the operation of the hook given its current implementation.
--
Aaron
^ permalink raw reply
* Re: [Non-DoD Source] Re: [PATCH] fanotify, inotify, dnotify, security: add security hook for fs notifications
From: Amir Goldstein @ 2019-08-09 16:29 UTC (permalink / raw)
To: Aaron Goidel
Cc: Paul Moore, selinux, LSM List, linux-fsdevel, David Howells,
Jan Kara, James Morris, Stephen Smalley, linux-kernel
In-Reply-To: <03ad3773-bea7-77de-0a1f-4bd6f41d3211@tycho.nsa.gov>
...
> >> First a suggestion, take it or leave it.
> >> The name of the hook _notify() seems misleading to me.
> >> naming the hook security_path_watch() seems much more
> >> appropriate and matching the name of the constants FILE__WATCH
> >> used by selinux.
> >
> > I guess I'm not too bothered by either name, Aaron? FWIW, if I was
> > writing this hook, I would probably name it
> > security_fsnotify_path(...).
> >
Or even just security_fsnotify()
>
> While I'm not necessarily attached to the name, I feel as though
> "misleading" is too strong a word here.
Agree. It is not misleading, but I should note that you yourself
named the security class "watch", so why the inconsistency?
> Notify seems to be an
> appropriate enough term to me as every call to the hook, and thus all
> the logic to which the hook adds security, lives in the notify/ subtree.
>
Well, if nobody cares about the name, it's fine by me.
I wanted to point your attention to this proposal by David Howells:
https://lore.kernel.org/linux-fsdevel/155991706847.15579.4702772917586301113.stgit@warthog.procyon.org.uk/
His proposal adds new types of watches, for keyring changes,
mount changes, etc and he proposed security hooks for setting
new watches named "watch_XXX" and for posting notifications
called "post_notification". The latter was later rejected by
Stephen Smalley:
https://lore.kernel.org/linux-fsdevel/cd657aab-e11c-c0b1-2e36-dd796ca75b75@tycho.nsa.gov/
https://lore.kernel.org/linux-fsdevel/541e5cb3-142b-fe87-dff6-260b46d34f2d@tycho.nsa.gov/
Just to have a perspective why the hook name "notify_path" may end up
being a bit ambiguous down the road.
Thanks,
Amir.
^ permalink raw reply
* Re: [Non-DoD Source] Re: [PATCH] fanotify, inotify, dnotify, security: add security hook for fs notifications
From: Amir Goldstein @ 2019-08-09 16:43 UTC (permalink / raw)
To: Aaron Goidel
Cc: Paul Moore, selinux, LSM List, linux-fsdevel, David Howells,
Jan Kara, James Morris, Stephen Smalley, linux-kernel
In-Reply-To: <e69f95ba-3da7-380a-ef14-cc866172d79a@tycho.nsa.gov>
> >>> + switch (flags & FANOTIFY_MARK_TYPE_BITS) {
> >>> + case FAN_MARK_MOUNT:
> >>> + obj_type = FSNOTIFY_OBJ_TYPE_VFSMOUNT;
> >>> + break;
> >>> + case FAN_MARK_FILESYSTEM:
> >>> + obj_type = FSNOTIFY_OBJ_TYPE_SB;
> >>> + break;
> >>> + case FAN_MARK_INODE:
> >>> + obj_type = FSNOTIFY_OBJ_TYPE_INODE;
> >>> + break;
> >>> + default:
> >>> + ret = -EINVAL;
> >>> + goto out;
> >>> + }
> >
> > Sorry, I just can't stand this extra switch statement here.
> > Please initialize obj_type at the very first switch statement in
> > do_fanotify_mark() and pass it to fanotify_find_path().
> > Preferably also make it a helper that returns either
> > valid obj_type or <0 for error.
> >
> >
> I have no problem moving the initialization of obj_type up one level to
> do_fanotify_mark(). I don't think that a helper is necessary at this
> juncture as this logic seems to only exist in one place. Should this
> change, then there would be merit to having a separate function.
Ok.
> >>> +
> >>> + ret = security_path_notify(path, mask, obj_type);
> >>> if (ret)
> >>> path_put(path);
> >
> > It is probably best to mask out FANOTIFY_EVENT_FLAGS
> > when calling the hook. Although FAN_EVENT_ON_CHILD
> > and FAN_ONDIR do map to corresponding FS_ constants,
> > the security hooks from dnotify and inotify do not pass these
> > flags, so the security module cannot use them as reliable
> > information, so it will have to assume that they have been
> > requested anyway.
> >
> > Alternatively, make sure that dnotify/inotify security hooks
> > always set these two flags, by fixing up and using the
> > dnotify/inotify arg_to_mask conversion helpers before calling
> > the security hook.
> >
> I think that at this point either approach you mentioned makes just as
> much sense. If it's all the same to you, Amir, I'll just change the
> caller in fanotify to include (mask) & ~(FANOTIFY_EVENT_FLAGS)
On second look, let's go with (mask & ALL_FSNOTIFY_EVENTS)
It seems simpler and more appropriate way to convert to FS_ flags.
[...]
> >>>
> >>> - ret = inotify_find_inode(pathname, &path, flags);
> >>> + ret = inotify_find_inode(pathname, &path, flags, mask);
> >
> > Please use (mask & IN_ALL_EVENTS) for converting to common FS_ flags
> > or use the inotify_arg_to_mask() conversion helper, which contains more
> > details irrelevant for the security hook.
> > Otherwise mask may contain flags like IN_MASK_CREATE, which mean
> > different things on different backends and the security module cannot tell
> > the difference.
> >
> > Also note that at this point, before inotify_arg_to_mask(), the mask does
> > not yet contain FS_EVENT_ON_CHILD, which could be interesting for
> > the security hook (fanotify users can opt-in with FAN_EVENT_ON_CHILD).
> > Not a big deal though as security hook can assume the worse
> > (that events on child are requested).
> >
> I'll use (mask & IN_ALL_EVENTS).
OK.
>
> I can implement the changes in the ways I mentioned above. I don't see a
> need for anything more in the cases you brought up since none of them
> change the logic of the hook itself or would make a substantive
> difference to the operation of the hook given its current implementation.
>
Agree. If more flags are needed for LSMs they could be added later.
Thanks,
Amir.
^ permalink raw reply
* [PATCH v2] fanotify, inotify, dnotify, security: add security hook for fs notifications
From: Aaron Goidel @ 2019-08-09 18:14 UTC (permalink / raw)
To: paul
Cc: selinux, linux-security-module, linux-fsdevel, dhowells, jack,
amir73il, jmorris, sds, linux-kernel, Aaron Goidel,
Casey Schaufler
As of now, setting watches on filesystem objects has, at most, applied a
check for read access to the inode, and in the case of fanotify, requires
CAP_SYS_ADMIN. No specific security hook or permission check has been
provided to control the setting of watches. Using any of inotify, dnotify,
or fanotify, it is possible to observe, not only write-like operations, but
even read access to a file. Modeling the watch as being merely a read from
the file is insufficient for the needs of SELinux. This is due to the fact
that read access should not necessarily imply access to information about
when another process reads from a file. Furthermore, fanotify watches grant
more power to an application in the form of permission events. While
notification events are solely, unidirectional (i.e. they only pass
information to the receiving application), permission events are blocking.
Permission events make a request to the receiving application which will
then reply with a decision as to whether or not that action may be
completed. This causes the issue of the watching application having the
ability to exercise control over the triggering process. Without drawing a
distinction within the permission check, the ability to read would imply
the greater ability to control an application. Additionally, mount and
superblock watches apply to all files within the same mount or superblock.
Read access to one file should not necessarily imply the ability to watch
all files accessed within a given mount or superblock.
In order to solve these issues, a new LSM hook is implemented and has been
placed within the system calls for marking filesystem objects with inotify,
fanotify, and dnotify watches. These calls to the hook are placed at the
point at which the target path has been resolved and are provided with the
path struct, the mask of requested notification events, and the type of
object on which the mark is being set (inode, superblock, or mount). The
mask and obj_type have already been translated into common FS_* values
shared by the entirety of the fs notification infrastructure. The path
struct is passed rather than just the inode so that the mount is available,
particularly for mount watches. This also allows for use of the hook by
pathname-based security modules. However, since the hook is intended for
use even by inode based security modules, it is not placed under the
CONFIG_SECURITY_PATH conditional. Otherwise, the inode-based security
modules would need to enable all of the path hooks, even though they do not
use any of them.
This only provides a hook at the point of setting a watch, and presumes
that permission to set a particular watch implies the ability to receive
all notification about that object which match the mask. This is all that
is required for SELinux. If other security modules require additional hooks
or infrastructure to control delivery of notification, these can be added
by them. It does not make sense for us to propose hooks for which we have
no implementation. The understanding that all notifications received by the
requesting application are all strictly of a type for which the application
has been granted permission shows that this implementation is sufficient in
its coverage.
Security modules wishing to provide complete control over fanotify must
also implement a security_file_open hook that validates that the access
requested by the watching application is authorized. Fanotify has the issue
that it returns a file descriptor with the file mode specified during
fanotify_init() to the watching process on event. This is already covered
by the LSM security_file_open hook if the security module implements
checking of the requested file mode there. Otherwise, a watching process
can obtain escalated access to a file for which it has not been authorized.
The selinux_path_notify hook implementation works by adding five new file
permissions: watch, watch_mount, watch_sb, watch_reads, and watch_with_perm
(descriptions about which will follow), and one new filesystem permission:
watch (which is applied to superblock checks). The hook then decides which
subset of these permissions must be held by the requesting application
based on the contents of the provided mask and the obj_type. The
selinux_file_open hook already checks the requested file mode and therefore
ensures that a watching process cannot escalate its access through
fanotify.
The watch, watch_mount, and watch_sb permissions are the baseline
permissions for setting a watch on an object and each are a requirement for
any watch to be set on a file, mount, or superblock respectively. It should
be noted that having either of the other two permissions (watch_reads and
watch_with_perm) does not imply the watch, watch_mount, or watch_sb
permission. Superblock watches further require the filesystem watch
permission to the superblock. As there is no labeled object in view for
mounts, there is no specific check for mount watches beyond watch_mount to
the inode. Such a check could be added in the future, if a suitable labeled
object existed representing the mount.
The watch_reads permission is required to receive notifications from
read-exclusive events on filesystem objects. These events include accessing
a file for the purpose of reading and closing a file which has been opened
read-only. This distinction has been drawn in order to provide a direct
indication in the policy for this otherwise not obvious capability. Read
access to a file should not necessarily imply the ability to observe read
events on a file.
Finally, watch_with_perm only applies to fanotify masks since it is the
only way to set a mask which allows for the blocking, permission event.
This permission is needed for any watch which is of this type. Though
fanotify requires CAP_SYS_ADMIN, this is insufficient as it gives implicit
trust to root, which we do not do, and does not support least privilege.
Signed-off-by: Aaron Goidel <acgoide@tycho.nsa.gov>
Acked-by: Casey Schaufler <casey@schaufler-ca.com>
---
v2:
- move initialization of obj_type up to remove duplicate work
- convert inotify and fanotify flags to common FS_* flags
fs/notify/dnotify/dnotify.c | 15 +++++++--
fs/notify/fanotify/fanotify_user.c | 19 ++++++++++--
fs/notify/inotify/inotify_user.c | 14 +++++++--
include/linux/lsm_hooks.h | 9 +++++-
include/linux/security.h | 10 ++++--
security/security.c | 6 ++++
security/selinux/hooks.c | 47 +++++++++++++++++++++++++++++
security/selinux/include/classmap.h | 5 +--
8 files changed, 113 insertions(+), 12 deletions(-)
diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
index 250369d6901d..87a7f61bc91c 100644
--- a/fs/notify/dnotify/dnotify.c
+++ b/fs/notify/dnotify/dnotify.c
@@ -22,6 +22,7 @@
#include <linux/sched/signal.h>
#include <linux/dnotify.h>
#include <linux/init.h>
+#include <linux/security.h>
#include <linux/spinlock.h>
#include <linux/slab.h>
#include <linux/fdtable.h>
@@ -288,6 +289,17 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
goto out_err;
}
+ /*
+ * convert the userspace DN_* "arg" to the internal FS_*
+ * defined in fsnotify
+ */
+ mask = convert_arg(arg);
+
+ error = security_path_notify(&filp->f_path, mask,
+ FSNOTIFY_OBJ_TYPE_INODE);
+ if (error)
+ goto out_err;
+
/* expect most fcntl to add new rather than augment old */
dn = kmem_cache_alloc(dnotify_struct_cache, GFP_KERNEL);
if (!dn) {
@@ -302,9 +314,6 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
goto out_err;
}
- /* convert the userspace DN_* "arg" to the internal FS_* defines in fsnotify */
- mask = convert_arg(arg);
-
/* set up the new_fsn_mark and new_dn_mark */
new_fsn_mark = &new_dn_mark->fsn_mark;
fsnotify_init_mark(new_fsn_mark, dnotify_group);
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index a90bb19dcfa2..8b4e2ad6d208 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -528,7 +528,8 @@ static const struct file_operations fanotify_fops = {
};
static int fanotify_find_path(int dfd, const char __user *filename,
- struct path *path, unsigned int flags)
+ struct path *path, unsigned int flags, __u64 mask,
+ unsigned int obj_type)
{
int ret;
@@ -567,8 +568,15 @@ static int fanotify_find_path(int dfd, const char __user *filename,
/* you can only watch an inode if you have read permissions on it */
ret = inode_permission(path->dentry->d_inode, MAY_READ);
+ if (ret) {
+ path_put(path);
+ goto out;
+ }
+
+ ret = security_path_notify(path, mask, obj_type);
if (ret)
path_put(path);
+
out:
return ret;
}
@@ -931,6 +939,7 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
__kernel_fsid_t __fsid, *fsid = NULL;
u32 valid_mask = FANOTIFY_EVENTS | FANOTIFY_EVENT_FLAGS;
unsigned int mark_type = flags & FANOTIFY_MARK_TYPE_BITS;
+ unsigned int obj_type;
int ret;
pr_debug("%s: fanotify_fd=%d flags=%x dfd=%d pathname=%p mask=%llx\n",
@@ -945,8 +954,13 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
switch (mark_type) {
case FAN_MARK_INODE:
+ obj_type = FSNOTIFY_OBJ_TYPE_INODE;
+ break;
case FAN_MARK_MOUNT:
+ obj_type = FSNOTIFY_OBJ_TYPE_VFSMOUNT;
+ break;
case FAN_MARK_FILESYSTEM:
+ obj_type = FSNOTIFY_OBJ_TYPE_SB;
break;
default:
return -EINVAL;
@@ -1014,7 +1028,8 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
goto fput_and_out;
}
- ret = fanotify_find_path(dfd, pathname, &path, flags);
+ ret = fanotify_find_path(dfd, pathname, &path, flags,
+ (mask & ALL_FSNOTIFY_EVENTS), obj_type);
if (ret)
goto fput_and_out;
diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index 7b53598c8804..408e9917ed42 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -39,6 +39,7 @@
#include <linux/poll.h>
#include <linux/wait.h>
#include <linux/memcontrol.h>
+#include <linux/security.h>
#include "inotify.h"
#include "../fdinfo.h"
@@ -342,7 +343,8 @@ static const struct file_operations inotify_fops = {
/*
* find_inode - resolve a user-given path to a specific inode
*/
-static int inotify_find_inode(const char __user *dirname, struct path *path, unsigned flags)
+static int inotify_find_inode(const char __user *dirname, struct path *path,
+ unsigned int flags, __u64 mask)
{
int error;
@@ -351,8 +353,15 @@ static int inotify_find_inode(const char __user *dirname, struct path *path, uns
return error;
/* you can only watch an inode if you have read permissions on it */
error = inode_permission(path->dentry->d_inode, MAY_READ);
+ if (error) {
+ path_put(path);
+ return error;
+ }
+ error = security_path_notify(path, mask,
+ FSNOTIFY_OBJ_TYPE_INODE);
if (error)
path_put(path);
+
return error;
}
@@ -744,7 +753,8 @@ SYSCALL_DEFINE3(inotify_add_watch, int, fd, const char __user *, pathname,
if (mask & IN_ONLYDIR)
flags |= LOOKUP_DIRECTORY;
- ret = inotify_find_inode(pathname, &path, flags);
+ ret = inotify_find_inode(pathname, &path, flags,
+ (mask & IN_ALL_EVENTS));
if (ret)
goto fput_and_out;
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 47f58cfb6a19..ead98af9c602 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -339,6 +339,9 @@
* Check for permission to change root directory.
* @path contains the path structure.
* Return 0 if permission is granted.
+ * @path_notify:
+ * Check permissions before setting a watch on events as defined by @mask,
+ * on an object at @path, whose type is defined by @obj_type.
* @inode_readlink:
* Check the permission to read the symbolic link.
* @dentry contains the dentry structure for the file link.
@@ -1535,7 +1538,9 @@ union security_list_options {
int (*path_chown)(const struct path *path, kuid_t uid, kgid_t gid);
int (*path_chroot)(const struct path *path);
#endif
-
+ /* Needed for inode based security check */
+ int (*path_notify)(const struct path *path, u64 mask,
+ unsigned int obj_type);
int (*inode_alloc_security)(struct inode *inode);
void (*inode_free_security)(struct inode *inode);
int (*inode_init_security)(struct inode *inode, struct inode *dir,
@@ -1860,6 +1865,8 @@ struct security_hook_heads {
struct hlist_head path_chown;
struct hlist_head path_chroot;
#endif
+ /* Needed for inode based modules as well */
+ struct hlist_head path_notify;
struct hlist_head inode_alloc_security;
struct hlist_head inode_free_security;
struct hlist_head inode_init_security;
diff --git a/include/linux/security.h b/include/linux/security.h
index 659071c2e57c..7d9c1da1f659 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -259,7 +259,8 @@ int security_dentry_create_files_as(struct dentry *dentry, int mode,
struct qstr *name,
const struct cred *old,
struct cred *new);
-
+int security_path_notify(const struct path *path, u64 mask,
+ unsigned int obj_type);
int security_inode_alloc(struct inode *inode);
void security_inode_free(struct inode *inode);
int security_inode_init_security(struct inode *inode, struct inode *dir,
@@ -387,7 +388,6 @@ int security_ismaclabel(const char *name);
int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen);
int security_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid);
void security_release_secctx(char *secdata, u32 seclen);
-
void security_inode_invalidate_secctx(struct inode *inode);
int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
@@ -621,6 +621,12 @@ static inline int security_move_mount(const struct path *from_path,
return 0;
}
+static inline int security_path_notify(const struct path *path, u64 mask,
+ unsigned int obj_type)
+{
+ return 0;
+}
+
static inline int security_inode_alloc(struct inode *inode)
{
return 0;
diff --git a/security/security.c b/security/security.c
index 613a5c00e602..30687e1366b7 100644
--- a/security/security.c
+++ b/security/security.c
@@ -871,6 +871,12 @@ int security_move_mount(const struct path *from_path, const struct path *to_path
return call_int_hook(move_mount, 0, from_path, to_path);
}
+int security_path_notify(const struct path *path, u64 mask,
+ unsigned int obj_type)
+{
+ return call_int_hook(path_notify, 0, path, mask, obj_type);
+}
+
int security_inode_alloc(struct inode *inode)
{
int rc = lsm_inode_alloc(inode);
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index f77b314d0575..a47376d1c924 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -92,6 +92,8 @@
#include <linux/kernfs.h>
#include <linux/stringhash.h> /* for hashlen_string() */
#include <uapi/linux/mount.h>
+#include <linux/fsnotify.h>
+#include <linux/fanotify.h>
#include "avc.h"
#include "objsec.h"
@@ -3261,6 +3263,50 @@ static int selinux_inode_removexattr(struct dentry *dentry, const char *name)
return -EACCES;
}
+static int selinux_path_notify(const struct path *path, u64 mask,
+ unsigned int obj_type)
+{
+ int ret;
+ u32 perm;
+
+ struct common_audit_data ad;
+
+ ad.type = LSM_AUDIT_DATA_PATH;
+ ad.u.path = *path;
+
+ /*
+ * Set permission needed based on the type of mark being set.
+ * Performs an additional check for sb watches.
+ */
+ switch (obj_type) {
+ case FSNOTIFY_OBJ_TYPE_VFSMOUNT:
+ perm = FILE__WATCH_MOUNT;
+ break;
+ case FSNOTIFY_OBJ_TYPE_SB:
+ perm = FILE__WATCH_SB;
+ ret = superblock_has_perm(current_cred(), path->dentry->d_sb,
+ FILESYSTEM__WATCH, &ad);
+ if (ret)
+ return ret;
+ break;
+ case FSNOTIFY_OBJ_TYPE_INODE:
+ perm = FILE__WATCH;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ // check if the mask is requesting ability to set a blocking watch
+ if (mask & (ALL_FSNOTIFY_PERM_EVENTS))
+ perm |= FILE__WATCH_WITH_PERM; // if so, check that permission
+
+ // is the mask asking to watch file reads?
+ if (mask & (FS_ACCESS | FS_ACCESS_PERM | FS_CLOSE_NOWRITE))
+ perm |= FILE__WATCH_READS; // check that permission as well
+
+ return path_has_perm(current_cred(), path, perm);
+}
+
/*
* Copy the inode security context value to the user.
*
@@ -6798,6 +6844,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
LSM_HOOK_INIT(inode_getsecid, selinux_inode_getsecid),
LSM_HOOK_INIT(inode_copy_up, selinux_inode_copy_up),
LSM_HOOK_INIT(inode_copy_up_xattr, selinux_inode_copy_up_xattr),
+ LSM_HOOK_INIT(path_notify, selinux_path_notify),
LSM_HOOK_INIT(kernfs_init_security, selinux_kernfs_init_security),
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index 201f7e588a29..32e9b03be3dd 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -7,7 +7,8 @@
#define COMMON_FILE_PERMS COMMON_FILE_SOCK_PERMS, "unlink", "link", \
"rename", "execute", "quotaon", "mounton", "audit_access", \
- "open", "execmod"
+ "open", "execmod", "watch", "watch_mount", "watch_sb", \
+ "watch_with_perm", "watch_reads"
#define COMMON_SOCK_PERMS COMMON_FILE_SOCK_PERMS, "bind", "connect", \
"listen", "accept", "getopt", "setopt", "shutdown", "recvfrom", \
@@ -60,7 +61,7 @@ struct security_class_mapping secclass_map[] = {
{ "filesystem",
{ "mount", "remount", "unmount", "getattr",
"relabelfrom", "relabelto", "associate", "quotamod",
- "quotaget", NULL } },
+ "quotaget", "watch", NULL } },
{ "file",
{ COMMON_FILE_PERMS,
"execute_no_trans", "entrypoint", NULL } },
--
2.21.0
^ permalink raw reply related
* [PATCH V39] Lock down module params that specify hardware parameters (eg. ioport)
From: Matthew Garrett @ 2019-08-09 20:58 UTC (permalink / raw)
To: jmorris
Cc: linux-security-module, linux-kernel, linux-api, David Howells,
Alan Cox, Matthew Garrett, Kees Cook, Jessica Yu
In-Reply-To: <20190808111246.GA29211@linux-8ccs>
From: David Howells <dhowells@redhat.com>
Provided an annotation for module parameters that specify hardware
parameters (such as io ports, iomem addresses, irqs, dma channels, fixed
dma buffers and other types).
Suggested-by: Alan Cox <gnomes@lxorguk.ukuu.org.uk>
Signed-off-by: David Howells <dhowells@redhat.com>
Signed-off-by: Matthew Garrett <mjg59@google.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Cc: Jessica Yu <jeyu@kernel.org>
---
include/linux/security.h | 1 +
kernel/params.c | 21 ++++++++++++++++-----
security/lockdown/lockdown.c | 1 +
3 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/include/linux/security.h b/include/linux/security.h
index 8f7048395114..43fa3486522b 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -113,6 +113,7 @@ enum lockdown_reason {
LOCKDOWN_ACPI_TABLES,
LOCKDOWN_PCMCIA_CIS,
LOCKDOWN_TIOCSSERIAL,
+ LOCKDOWN_MODULE_PARAMETERS,
LOCKDOWN_INTEGRITY_MAX,
LOCKDOWN_CONFIDENTIALITY_MAX,
};
diff --git a/kernel/params.c b/kernel/params.c
index cf448785d058..8e56f8b12d8f 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -12,6 +12,7 @@
#include <linux/err.h>
#include <linux/slab.h>
#include <linux/ctype.h>
+#include <linux/security.h>
#ifdef CONFIG_SYSFS
/* Protects all built-in parameters, modules use their own param_lock */
@@ -96,13 +97,19 @@ bool parameq(const char *a, const char *b)
return parameqn(a, b, strlen(a)+1);
}
-static void param_check_unsafe(const struct kernel_param *kp)
+static bool param_check_unsafe(const struct kernel_param *kp)
{
+ if (kp->flags & KERNEL_PARAM_FL_HWPARAM &&
+ security_locked_down(LOCKDOWN_MODULE_PARAMETERS))
+ return false;
+
if (kp->flags & KERNEL_PARAM_FL_UNSAFE) {
pr_notice("Setting dangerous option %s - tainting kernel\n",
kp->name);
add_taint(TAINT_USER, LOCKDEP_STILL_OK);
}
+
+ return true;
}
static int parse_one(char *param,
@@ -132,8 +139,10 @@ static int parse_one(char *param,
pr_debug("handling %s with %p\n", param,
params[i].ops->set);
kernel_param_lock(params[i].mod);
- param_check_unsafe(¶ms[i]);
- err = params[i].ops->set(val, ¶ms[i]);
+ if (param_check_unsafe(¶ms[i]))
+ err = params[i].ops->set(val, ¶ms[i]);
+ else
+ err = -EPERM;
kernel_param_unlock(params[i].mod);
return err;
}
@@ -553,8 +562,10 @@ static ssize_t param_attr_store(struct module_attribute *mattr,
return -EPERM;
kernel_param_lock(mk->mod);
- param_check_unsafe(attribute->param);
- err = attribute->param->ops->set(buf, attribute->param);
+ if (param_check_unsafe(attribute->param))
+ err = attribute->param->ops->set(buf, attribute->param);
+ else
+ err = -EPERM;
kernel_param_unlock(mk->mod);
if (!err)
return len;
diff --git a/security/lockdown/lockdown.c b/security/lockdown/lockdown.c
index 00a3a6438dd2..5177938cfa0d 100644
--- a/security/lockdown/lockdown.c
+++ b/security/lockdown/lockdown.c
@@ -28,6 +28,7 @@ static char *lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1] = {
[LOCKDOWN_ACPI_TABLES] = "modifying ACPI tables",
[LOCKDOWN_PCMCIA_CIS] = "direct PCMCIA CIS storage",
[LOCKDOWN_TIOCSSERIAL] = "reconfiguration of serial port IO",
+ [LOCKDOWN_MODULE_PARAMETERS] = "unsafe module parameters",
[LOCKDOWN_INTEGRITY_MAX] = "integrity",
[LOCKDOWN_CONFIDENTIALITY_MAX] = "confidentiality",
};
--
2.23.0.rc1.153.gdeed80330f-goog
^ permalink raw reply related
* [PATCH V39] Enforce module signatures if the kernel is locked down
From: Matthew Garrett @ 2019-08-09 20:59 UTC (permalink / raw)
To: jmorris
Cc: linux-security-module, linux-kernel, linux-api, David Howells,
Matthew Garrett, Kees Cook, Jessica Yu
In-Reply-To: <20190808100059.GA30260@linux-8ccs>
From: David Howells <dhowells@redhat.com>
If the kernel is locked down, require that all modules have valid
signatures that we can verify.
I have adjusted the errors generated:
(1) If there's no signature (ENODATA) or we can't check it (ENOPKG,
ENOKEY), then:
(a) If signatures are enforced then EKEYREJECTED is returned.
(b) If there's no signature or we can't check it, but the kernel is
locked down then EPERM is returned (this is then consistent with
other lockdown cases).
(2) If the signature is unparseable (EBADMSG, EINVAL), the signature fails
the check (EKEYREJECTED) or a system error occurs (eg. ENOMEM), we
return the error we got.
Note that the X.509 code doesn't check for key expiry as the RTC might not
be valid or might not have been transferred to the kernel's clock yet.
[Modified by Matthew Garrett to remove the IMA integration. This will
be replaced with integration with the IMA architecture policy
patchset.]
Signed-off-by: David Howells <dhowells@redhat.com>
Signed-off-by: Matthew Garrett <matthewgarrett@google.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Cc: Jessica Yu <jeyu@kernel.org>
---
include/linux/security.h | 1 +
init/Kconfig | 5 +++++
kernel/module.c | 37 +++++++++++++++++++++++++++++-------
security/lockdown/Kconfig | 1 +
security/lockdown/lockdown.c | 1 +
5 files changed, 38 insertions(+), 7 deletions(-)
diff --git a/include/linux/security.h b/include/linux/security.h
index 54a0532ec12f..8e70063074a1 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -103,6 +103,7 @@ enum lsm_event {
*/
enum lockdown_reason {
LOCKDOWN_NONE,
+ LOCKDOWN_MODULE_SIGNATURE,
LOCKDOWN_INTEGRITY_MAX,
LOCKDOWN_CONFIDENTIALITY_MAX,
};
diff --git a/init/Kconfig b/init/Kconfig
index bd7d650d4a99..1f0f53774c3e 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -2017,6 +2017,11 @@ config MODULE_SIG
kernel build dependency so that the signing tool can use its crypto
library.
+ You should enable this option if you wish to use either
+ CONFIG_SECURITY_LOCKDOWN_LSM or lockdown functionality imposed via
+ another LSM - otherwise unsigned modules will be loadable regardless
+ of the lockdown policy.
+
!!!WARNING!!! If you enable this option, you MUST make sure that the
module DOES NOT get stripped after being signed. This includes the
debuginfo strip done by some packagers (such as rpmbuild) and
diff --git a/kernel/module.c b/kernel/module.c
index cd8df516666d..318209889e26 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2771,8 +2771,9 @@ static inline void kmemleak_load_module(const struct module *mod,
#ifdef CONFIG_MODULE_SIG
static int module_sig_check(struct load_info *info, int flags)
{
- int err = -ENOKEY;
+ int err = -ENODATA;
const unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1;
+ const char *reason;
const void *mod = info->hdr;
/*
@@ -2787,16 +2788,38 @@ static int module_sig_check(struct load_info *info, int flags)
err = mod_verify_sig(mod, info);
}
- if (!err) {
+ switch (err) {
+ case 0:
info->sig_ok = true;
return 0;
- }
- /* Not having a signature is only an error if we're strict. */
- if (err == -ENOKEY && !is_module_sig_enforced())
- err = 0;
+ /* We don't permit modules to be loaded into trusted kernels
+ * without a valid signature on them, but if we're not
+ * enforcing, certain errors are non-fatal.
+ */
+ case -ENODATA:
+ reason = "Loading of unsigned module";
+ goto decide;
+ case -ENOPKG:
+ reason = "Loading of module with unsupported crypto";
+ goto decide;
+ case -ENOKEY:
+ reason = "Loading of module with unavailable key";
+ decide:
+ if (is_module_sig_enforced()) {
+ pr_notice("%s is rejected\n", reason);
+ return -EKEYREJECTED;
+ }
- return err;
+ return security_locked_down(LOCKDOWN_MODULE_SIGNATURE);
+
+ /* All other errors are fatal, including nomem, unparseable
+ * signatures and signature check failures - even if signatures
+ * aren't required.
+ */
+ default:
+ return err;
+ }
}
#else /* !CONFIG_MODULE_SIG */
static int module_sig_check(struct load_info *info, int flags)
diff --git a/security/lockdown/Kconfig b/security/lockdown/Kconfig
index 7374ba76d8eb..f9dd683261e9 100644
--- a/security/lockdown/Kconfig
+++ b/security/lockdown/Kconfig
@@ -1,6 +1,7 @@
config SECURITY_LOCKDOWN_LSM
bool "Basic module for enforcing kernel lockdown"
depends on SECURITY
+ select MODULE_SIG if MODULES
help
Build support for an LSM that enforces a coarse kernel lockdown
behaviour.
diff --git a/security/lockdown/lockdown.c b/security/lockdown/lockdown.c
index d30c4d254b5f..2c53fd9f5c9b 100644
--- a/security/lockdown/lockdown.c
+++ b/security/lockdown/lockdown.c
@@ -18,6 +18,7 @@ static enum lockdown_reason kernel_locked_down;
static char *lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1] = {
[LOCKDOWN_NONE] = "none",
+ [LOCKDOWN_MODULE_SIGNATURE] = "unsigned module loading",
[LOCKDOWN_INTEGRITY_MAX] = "integrity",
[LOCKDOWN_CONFIDENTIALITY_MAX] = "confidentiality",
};
--
2.23.0.rc1.153.gdeed80330f-goog
^ permalink raw reply related
* Re: [PATCH V38 00/29] security: Add support for locking down the kernel
From: James Morris @ 2019-08-10 6:08 UTC (permalink / raw)
To: Matthew Garrett; +Cc: linux-security-module, linux-kernel, linux-api
In-Reply-To: <20190808000721.124691-1-matthewgarrett@google.com>
On Wed, 7 Aug 2019, Matthew Garrett wrote:
> Fixed an unused function parameter in patch 19, otherwise identical to
> V37.
Applied to
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git next-lockdown
and next-testing
Please verify and test, as I had to make a few minor fixups for my v5.2
base.
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* [security:next-lockdown 1/29] init/main.c:572:2: error: implicit declaration of function 'early_security_init'; did you mean 'security_init'?
From: kbuild test robot @ 2019-08-10 6:40 UTC (permalink / raw)
To: Matthew Garrett; +Cc: kbuild-all, linux-security-module, James Morris
[-- Attachment #1: Type: text/plain, Size: 6367 bytes --]
tree: https://kernel.googlesource.com/pub/scm/linux/kernel/git/jmorris/linux-security.git next-lockdown
head: 05ef41e93e1a40d6b2d9846284824ec6f67fe422
commit: 45d29f9e9b8b9533e0ba1b3051d9d39ba29abad6 [1/29] security: Support early LSMs
config: i386-alldefconfig (attached as .config)
compiler: gcc-7 (Debian 7.4.0-10) 7.4.0
reproduce:
git checkout 45d29f9e9b8b9533e0ba1b3051d9d39ba29abad6
# save the attached .config to linux build tree
make ARCH=i386
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
init/main.c: In function 'start_kernel':
>> init/main.c:572:2: error: implicit declaration of function 'early_security_init'; did you mean 'security_init'? [-Werror=implicit-function-declaration]
early_security_init();
^~~~~~~~~~~~~~~~~~~
security_init
cc1: some warnings being treated as errors
vim +572 init/main.c
550
551 asmlinkage __visible void __init start_kernel(void)
552 {
553 char *command_line;
554 char *after_dashes;
555
556 set_task_stack_end_magic(&init_task);
557 smp_setup_processor_id();
558 debug_objects_early_init();
559
560 cgroup_init_early();
561
562 local_irq_disable();
563 early_boot_irqs_disabled = true;
564
565 /*
566 * Interrupts are still disabled. Do necessary setups, then
567 * enable them.
568 */
569 boot_cpu_init();
570 page_address_init();
571 pr_notice("%s", linux_banner);
> 572 early_security_init();
573 setup_arch(&command_line);
574 mm_init_cpumask(&init_mm);
575 setup_command_line(command_line);
576 setup_nr_cpu_ids();
577 setup_per_cpu_areas();
578 smp_prepare_boot_cpu(); /* arch-specific boot-cpu hooks */
579 boot_cpu_hotplug_init();
580
581 build_all_zonelists(NULL);
582 page_alloc_init();
583
584 pr_notice("Kernel command line: %s\n", boot_command_line);
585 /* parameters may set static keys */
586 jump_label_init();
587 parse_early_param();
588 after_dashes = parse_args("Booting kernel",
589 static_command_line, __start___param,
590 __stop___param - __start___param,
591 -1, -1, NULL, &unknown_bootoption);
592 if (!IS_ERR_OR_NULL(after_dashes))
593 parse_args("Setting init args", after_dashes, NULL, 0, -1, -1,
594 NULL, set_init_arg);
595
596 /*
597 * These use large bootmem allocations and must precede
598 * kmem_cache_init()
599 */
600 setup_log_buf(0);
601 vfs_caches_init_early();
602 sort_main_extable();
603 trap_init();
604 mm_init();
605
606 ftrace_init();
607
608 /* trace_printk can be enabled here */
609 early_trace_init();
610
611 /*
612 * Set up the scheduler prior starting any interrupts (such as the
613 * timer interrupt). Full topology setup happens at smp_init()
614 * time - but meanwhile we still have a functioning scheduler.
615 */
616 sched_init();
617 /*
618 * Disable preemption - early bootup scheduling is extremely
619 * fragile until we cpu_idle() for the first time.
620 */
621 preempt_disable();
622 if (WARN(!irqs_disabled(),
623 "Interrupts were enabled *very* early, fixing it\n"))
624 local_irq_disable();
625 radix_tree_init();
626
627 /*
628 * Set up housekeeping before setting up workqueues to allow the unbound
629 * workqueue to take non-housekeeping into account.
630 */
631 housekeeping_init();
632
633 /*
634 * Allow workqueue creation and work item queueing/cancelling
635 * early. Work item execution depends on kthreads and starts after
636 * workqueue_init().
637 */
638 workqueue_init_early();
639
640 rcu_init();
641
642 /* Trace events are available after this */
643 trace_init();
644
645 if (initcall_debug)
646 initcall_debug_enable();
647
648 context_tracking_init();
649 /* init some links before init_ISA_irqs() */
650 early_irq_init();
651 init_IRQ();
652 tick_init();
653 rcu_init_nohz();
654 init_timers();
655 hrtimers_init();
656 softirq_init();
657 timekeeping_init();
658
659 /*
660 * For best initial stack canary entropy, prepare it after:
661 * - setup_arch() for any UEFI RNG entropy and boot cmdline access
662 * - timekeeping_init() for ktime entropy used in rand_initialize()
663 * - rand_initialize() to get any arch-specific entropy like RDRAND
664 * - add_latent_entropy() to get any latent entropy
665 * - adding command line entropy
666 */
667 rand_initialize();
668 add_latent_entropy();
669 add_device_randomness(command_line, strlen(command_line));
670 boot_init_stack_canary();
671
672 time_init();
673 printk_safe_init();
674 perf_event_init();
675 profile_init();
676 call_function_init();
677 WARN(!irqs_disabled(), "Interrupts were enabled early\n");
678
679 early_boot_irqs_disabled = false;
680 local_irq_enable();
681
682 kmem_cache_init_late();
683
684 /*
685 * HACK ALERT! This is early. We're enabling the console before
686 * we've done PCI setups etc, and console_init() must be aware of
687 * this. But we do want output early, in case something goes wrong.
688 */
689 console_init();
690 if (panic_later)
691 panic("Too many boot %s vars at `%s'", panic_later,
692 panic_param);
693
694 lockdep_init();
695
696 /*
697 * Need to run this when irqs are enabled, because it wants
698 * to self-test [hard/soft]-irqs on/off lock inversion bugs
699 * too:
700 */
701 locking_selftest();
702
703 /*
704 * This needs to be called before any devices perform DMA
705 * operations that might use the SWIOTLB bounce buffers. It will
706 * mark the bounce buffers as decrypted so that their usage will
707 * not cause "plain-text" data to be decrypted when accessed.
708 */
709 mem_encrypt_init();
710
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 10500 bytes --]
^ permalink raw reply
* [PATCH] security: fix ptr_ret.cocci warnings
From: kbuild test robot @ 2019-08-10 6:58 UTC (permalink / raw)
To: Matthew Garrett
Cc: kbuild-all, linux-security-module, James Morris, Kees Cook
In-Reply-To: <201908101444.zkWryFiG%lkp@intel.com>
From: kbuild test robot <lkp@intel.com>
security/lockdown/lockdown.c:157:1-3: WARNING: PTR_ERR_OR_ZERO can be used
Use PTR_ERR_OR_ZERO rather than if(IS_ERR(...)) + PTR_ERR
Generated by: scripts/coccinelle/api/ptr_ret.cocci
Fixes: 80d14015a8e3 ("security: Add a static lockdown policy LSM")
CC: Matthew Garrett <matthewgarrett@google.com>
Signed-off-by: kbuild test robot <lkp@intel.com>
---
tree: https://kernel.googlesource.com/pub/scm/linux/kernel/git/jmorris/linux-security.git next-lockdown
head: 05ef41e93e1a40d6b2d9846284824ec6f67fe422
commit: 80d14015a8e3109f9b5e3e39b0bc78e1c3a1f315 [3/29] security: Add a static lockdown policy LSM
lockdown.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
--- a/security/lockdown/lockdown.c
+++ b/security/lockdown/lockdown.c
@@ -154,10 +154,7 @@ static int __init lockdown_secfs_init(vo
dentry = securityfs_create_file("lockdown", 0600, NULL, NULL,
&lockdown_ops);
- if (IS_ERR(dentry))
- return PTR_ERR(dentry);
-
- return 0;
+ return PTR_ERR_OR_ZERO(dentry);
}
core_initcall(lockdown_secfs_init);
^ permalink raw reply
* [security:next-lockdown 3/29] security/lockdown/lockdown.c:157:1-3: WARNING: PTR_ERR_OR_ZERO can be used
From: kbuild test robot @ 2019-08-10 6:58 UTC (permalink / raw)
To: Matthew Garrett
Cc: kbuild-all, linux-security-module, James Morris, Kees Cook
tree: https://kernel.googlesource.com/pub/scm/linux/kernel/git/jmorris/linux-security.git next-lockdown
head: 05ef41e93e1a40d6b2d9846284824ec6f67fe422
commit: 80d14015a8e3109f9b5e3e39b0bc78e1c3a1f315 [3/29] security: Add a static lockdown policy LSM
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
coccinelle warnings: (new ones prefixed by >>)
>> security/lockdown/lockdown.c:157:1-3: WARNING: PTR_ERR_OR_ZERO can be used
Please review and possibly fold the followup patch.
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
^ permalink raw reply
* [security:next-lockdown 2/29] htmldocs: include/linux/lsm_hooks.h:1812: warning: Function parameter or member 'locked_down' not described in 'security_list_options'
From: kbuild test robot @ 2019-08-10 7:34 UTC (permalink / raw)
To: Matthew Garrett; +Cc: kbuild-all, linux-security-module, James Morris
[-- Attachment #1: Type: text/plain, Size: 23173 bytes --]
tree: https://kernel.googlesource.com/pub/scm/linux/kernel/git/jmorris/linux-security.git next-lockdown
head: 05ef41e93e1a40d6b2d9846284824ec6f67fe422
commit: c360be6c444ff1b53867cf02b7df5f64bf996258 [2/29] security: Add a "locked down" LSM hook
reproduce: make htmldocs
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1285: warning: Function parameter or member 'incr' not described in 'amdgpu_vm_update_flags'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1285: warning: Function parameter or member 'flags' not described in 'amdgpu_vm_update_flags'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:2812: warning: Function parameter or member 'pasid' not described in 'amdgpu_vm_make_compute'
drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c:375: warning: Excess function parameter 'entry' description in 'amdgpu_irq_dispatch'
drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c:376: warning: Function parameter or member 'ih' not described in 'amdgpu_irq_dispatch'
drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c:376: warning: Excess function parameter 'entry' description in 'amdgpu_irq_dispatch'
drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c:1: warning: no structured comments found
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:128: warning: Incorrect use of kernel-doc format: * @atomic_obj
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:210: warning: Function parameter or member 'atomic_obj' not described in 'amdgpu_display_manager'
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:210: warning: Function parameter or member 'backlight_link' not described in 'amdgpu_display_manager'
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:210: warning: Function parameter or member 'backlight_caps' not described in 'amdgpu_display_manager'
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:210: warning: Function parameter or member 'freesync_module' not described in 'amdgpu_display_manager'
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:210: warning: Function parameter or member 'fw_dmcu' not described in 'amdgpu_display_manager'
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:210: warning: Function parameter or member 'dmcu_fw_version' not described in 'amdgpu_display_manager'
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c:1: warning: no structured comments found
include/drm/drm_drv.h:722: warning: Function parameter or member 'gem_prime_pin' not described in 'drm_driver'
include/drm/drm_drv.h:722: warning: Function parameter or member 'gem_prime_unpin' not described in 'drm_driver'
include/drm/drm_drv.h:722: warning: Function parameter or member 'gem_prime_res_obj' not described in 'drm_driver'
include/drm/drm_drv.h:722: warning: Function parameter or member 'gem_prime_get_sg_table' not described in 'drm_driver'
include/drm/drm_drv.h:722: warning: Function parameter or member 'gem_prime_import_sg_table' not described in 'drm_driver'
include/drm/drm_drv.h:722: warning: Function parameter or member 'gem_prime_vmap' not described in 'drm_driver'
include/drm/drm_drv.h:722: warning: Function parameter or member 'gem_prime_vunmap' not described in 'drm_driver'
include/drm/drm_drv.h:722: warning: Function parameter or member 'gem_prime_mmap' not described in 'drm_driver'
include/drm/drm_modeset_helper_vtables.h:1004: warning: Function parameter or member 'prepare_writeback_job' not described in 'drm_connector_helper_funcs'
include/drm/drm_modeset_helper_vtables.h:1004: warning: Function parameter or member 'cleanup_writeback_job' not described in 'drm_connector_helper_funcs'
include/drm/drm_atomic_state_helper.h:1: warning: no structured comments found
drivers/gpu/drm/scheduler/sched_main.c:419: warning: Function parameter or member 'full_recovery' not described in 'drm_sched_start'
drivers/gpu/drm/i915/i915_vma.h:50: warning: cannot understand function prototype: 'struct i915_vma '
drivers/gpu/drm/i915/i915_vma.h:1: warning: no structured comments found
drivers/gpu/drm/i915/intel_guc_fwif.h:536: warning: cannot understand function prototype: 'struct guc_log_buffer_state '
drivers/gpu/drm/i915/i915_trace.h:1: warning: no structured comments found
drivers/gpu/drm/i915/i915_reg.h:156: warning: bad line:
include/linux/interconnect.h:1: warning: no structured comments found
include/linux/skbuff.h:893: warning: Function parameter or member 'dev_scratch' not described in 'sk_buff'
include/linux/skbuff.h:893: warning: Function parameter or member 'list' not described in 'sk_buff'
include/linux/skbuff.h:893: warning: Function parameter or member 'ip_defrag_offset' not described in 'sk_buff'
include/linux/skbuff.h:893: warning: Function parameter or member 'skb_mstamp_ns' not described in 'sk_buff'
include/linux/skbuff.h:893: warning: Function parameter or member '__cloned_offset' not described in 'sk_buff'
include/linux/skbuff.h:893: warning: Function parameter or member 'head_frag' not described in 'sk_buff'
include/linux/skbuff.h:893: warning: Function parameter or member '__pkt_type_offset' not described in 'sk_buff'
include/linux/skbuff.h:893: warning: Function parameter or member 'encapsulation' not described in 'sk_buff'
include/linux/skbuff.h:893: warning: Function parameter or member 'encap_hdr_csum' not described in 'sk_buff'
include/linux/skbuff.h:893: warning: Function parameter or member 'csum_valid' not described in 'sk_buff'
include/linux/skbuff.h:893: warning: Function parameter or member '__pkt_vlan_present_offset' not described in 'sk_buff'
include/linux/skbuff.h:893: warning: Function parameter or member 'vlan_present' not described in 'sk_buff'
include/linux/skbuff.h:893: warning: Function parameter or member 'csum_complete_sw' not described in 'sk_buff'
include/linux/skbuff.h:893: warning: Function parameter or member 'csum_level' not described in 'sk_buff'
include/linux/skbuff.h:893: warning: Function parameter or member 'inner_protocol_type' not described in 'sk_buff'
include/linux/skbuff.h:893: warning: Function parameter or member 'remcsum_offload' not described in 'sk_buff'
include/linux/skbuff.h:893: warning: Function parameter or member 'sender_cpu' not described in 'sk_buff'
include/linux/skbuff.h:893: warning: Function parameter or member 'reserved_tailroom' not described in 'sk_buff'
include/linux/skbuff.h:893: warning: Function parameter or member 'inner_ipproto' not described in 'sk_buff'
include/net/sock.h:233: warning: Function parameter or member 'skc_addrpair' not described in 'sock_common'
include/net/sock.h:233: warning: Function parameter or member 'skc_portpair' not described in 'sock_common'
include/net/sock.h:233: warning: Function parameter or member 'skc_ipv6only' not described in 'sock_common'
include/net/sock.h:233: warning: Function parameter or member 'skc_net_refcnt' not described in 'sock_common'
include/net/sock.h:233: warning: Function parameter or member 'skc_v6_daddr' not described in 'sock_common'
include/net/sock.h:233: warning: Function parameter or member 'skc_v6_rcv_saddr' not described in 'sock_common'
include/net/sock.h:233: warning: Function parameter or member 'skc_cookie' not described in 'sock_common'
include/net/sock.h:233: warning: Function parameter or member 'skc_listener' not described in 'sock_common'
include/net/sock.h:233: warning: Function parameter or member 'skc_tw_dr' not described in 'sock_common'
include/net/sock.h:233: warning: Function parameter or member 'skc_rcv_wnd' not described in 'sock_common'
include/net/sock.h:233: warning: Function parameter or member 'skc_tw_rcv_nxt' not described in 'sock_common'
include/net/sock.h:515: warning: Function parameter or member 'sk_rx_skb_cache' not described in 'sock'
include/net/sock.h:515: warning: Function parameter or member 'sk_wq_raw' not described in 'sock'
include/net/sock.h:515: warning: Function parameter or member 'tcp_rtx_queue' not described in 'sock'
include/net/sock.h:515: warning: Function parameter or member 'sk_tx_skb_cache' not described in 'sock'
include/net/sock.h:515: warning: Function parameter or member 'sk_route_forced_caps' not described in 'sock'
include/net/sock.h:515: warning: Function parameter or member 'sk_txtime_report_errors' not described in 'sock'
include/net/sock.h:515: warning: Function parameter or member 'sk_validate_xmit_skb' not described in 'sock'
include/net/sock.h:515: warning: Function parameter or member 'sk_bpf_storage' not described in 'sock'
include/net/sock.h:2439: warning: Function parameter or member 'tcp_rx_skb_cache_key' not described in 'DECLARE_STATIC_KEY_FALSE'
include/net/sock.h:2439: warning: Excess function parameter 'sk' description in 'DECLARE_STATIC_KEY_FALSE'
include/net/sock.h:2439: warning: Excess function parameter 'skb' description in 'DECLARE_STATIC_KEY_FALSE'
include/linux/netdevice.h:2040: warning: Function parameter or member 'gso_partial_features' not described in 'net_device'
include/linux/netdevice.h:2040: warning: Function parameter or member 'l3mdev_ops' not described in 'net_device'
include/linux/netdevice.h:2040: warning: Function parameter or member 'xfrmdev_ops' not described in 'net_device'
include/linux/netdevice.h:2040: warning: Function parameter or member 'tlsdev_ops' not described in 'net_device'
include/linux/netdevice.h:2040: warning: Function parameter or member 'name_assign_type' not described in 'net_device'
include/linux/netdevice.h:2040: warning: Function parameter or member 'ieee802154_ptr' not described in 'net_device'
include/linux/netdevice.h:2040: warning: Function parameter or member 'mpls_ptr' not described in 'net_device'
include/linux/netdevice.h:2040: warning: Function parameter or member 'xdp_prog' not described in 'net_device'
include/linux/netdevice.h:2040: warning: Function parameter or member 'gro_flush_timeout' not described in 'net_device'
include/linux/netdevice.h:2040: warning: Function parameter or member 'nf_hooks_ingress' not described in 'net_device'
include/linux/netdevice.h:2040: warning: Function parameter or member '____cacheline_aligned_in_smp' not described in 'net_device'
include/linux/netdevice.h:2040: warning: Function parameter or member 'qdisc_hash' not described in 'net_device'
include/linux/netdevice.h:2040: warning: Function parameter or member 'xps_cpus_map' not described in 'net_device'
include/linux/netdevice.h:2040: warning: Function parameter or member 'xps_rxqs_map' not described in 'net_device'
include/linux/phylink.h:56: warning: Function parameter or member '__ETHTOOL_DECLARE_LINK_MODE_MASK(advertising' not described in 'phylink_link_state'
include/linux/phylink.h:56: warning: Function parameter or member '__ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising' not described in 'phylink_link_state'
include/linux/lsm_hooks.h:1812: warning: Function parameter or member 'quotactl' not described in 'security_list_options'
include/linux/lsm_hooks.h:1812: warning: Function parameter or member 'quota_on' not described in 'security_list_options'
include/linux/lsm_hooks.h:1812: warning: Function parameter or member 'sb_free_mnt_opts' not described in 'security_list_options'
include/linux/lsm_hooks.h:1812: warning: Function parameter or member 'sb_eat_lsm_opts' not described in 'security_list_options'
include/linux/lsm_hooks.h:1812: warning: Function parameter or member 'sb_kern_mount' not described in 'security_list_options'
include/linux/lsm_hooks.h:1812: warning: Function parameter or member 'sb_show_options' not described in 'security_list_options'
include/linux/lsm_hooks.h:1812: warning: Function parameter or member 'sb_add_mnt_opt' not described in 'security_list_options'
include/linux/lsm_hooks.h:1812: warning: Function parameter or member 'd_instantiate' not described in 'security_list_options'
include/linux/lsm_hooks.h:1812: warning: Function parameter or member 'getprocattr' not described in 'security_list_options'
include/linux/lsm_hooks.h:1812: warning: Function parameter or member 'setprocattr' not described in 'security_list_options'
>> include/linux/lsm_hooks.h:1812: warning: Function parameter or member 'locked_down' not described in 'security_list_options'
Documentation/admin-guide/mm/numaperf.rst:168: WARNING: Footnote [1] is not referenced.
Documentation/bpf/btf.rst:154: WARNING: Unexpected indentation.
Documentation/bpf/btf.rst:163: WARNING: Unexpected indentation.
lib/list_sort.c:162: WARNING: Unexpected indentation.
lib/list_sort.c:163: WARNING: Block quote ends without a blank line; unexpected unindent.
include/linux/xarray.h:232: WARNING: Unexpected indentation.
kernel/time/hrtimer.c:1120: WARNING: Block quote ends without a blank line; unexpected unindent.
kernel/signal.c:349: WARNING: Inline literal start-string without end-string.
include/uapi/linux/firewire-cdev.h:312: WARNING: Inline literal start-string without end-string.
Documentation/driver-api/gpio/driver.rst:419: WARNING: Unknown target name: "devm".
include/linux/i2c.h:510: WARNING: Inline strong start-string without end-string.
drivers/ata/libata-core.c:5947: WARNING: Unknown target name: "hw".
drivers/message/fusion/mptbase.c:5057: WARNING: Definition list ends without a blank line; unexpected unindent.
drivers/tty/serial/serial_core.c:1959: WARNING: Definition list ends without a blank line; unexpected unindent.
include/linux/regulator/driver.h:286: WARNING: Unknown target name: "regulator_regmap_x_voltage".
Documentation/driver-api/soundwire/locking.rst:50: WARNING: Inconsistent literal block quoting.
Documentation/driver-api/soundwire/locking.rst:51: WARNING: Line block ends without a blank line.
Documentation/driver-api/soundwire/locking.rst:55: WARNING: Inline substitution_reference start-string without end-string.
Documentation/driver-api/soundwire/locking.rst:56: WARNING: Line block ends without a blank line.
include/linux/spi/spi.h:380: WARNING: Unexpected indentation.
fs/posix_acl.c:636: WARNING: Inline emphasis start-string without end-string.
fs/debugfs/inode.c:385: WARNING: Inline literal start-string without end-string.
fs/debugfs/inode.c:464: WARNING: Inline literal start-string without end-string.
fs/debugfs/inode.c:496: WARNING: Inline literal start-string without end-string.
fs/debugfs/inode.c:583: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:394: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:400: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:439: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:445: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:484: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:490: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:530: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:536: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:578: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:584: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:845: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:851: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:898: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:904: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:1001: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:1001: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:1096: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:1102: WARNING: Inline literal start-string without end-string.
drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c:2024: WARNING: Inline emphasis start-string without end-string.
drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c:2026: WARNING: Inline emphasis start-string without end-string.
Documentation/networking/device_drivers/freescale/dpaa2/dpio-driver.rst:43: WARNING: Definition list ends without a blank line; unexpected unindent.
Documentation/networking/device_drivers/freescale/dpaa2/dpio-driver.rst:63: WARNING: Unexpected indentation.
Documentation/networking/dsa/sja1105.rst:91: WARNING: Block quote ends without a blank line; unexpected unindent.
Documentation/networking/dsa/sja1105.rst:91: WARNING: Block quote ends without a blank line; unexpected unindent.
include/linux/netdevice.h:3482: WARNING: Inline emphasis start-string without end-string.
include/linux/netdevice.h:3482: WARNING: Inline emphasis start-string without end-string.
net/core/dev.c:5018: WARNING: Unknown target name: "page_is".
Documentation/security/keys/core.rst:1597: WARNING: Inline literal start-string without end-string.
Documentation/security/keys/core.rst:1597: WARNING: Inline emphasis start-string without end-string.
Documentation/security/keys/core.rst:1597: WARNING: Inline emphasis start-string without end-string.
Documentation/security/keys/core.rst:1598: WARNING: Inline emphasis start-string without end-string.
Documentation/security/keys/core.rst:1598: WARNING: Inline emphasis start-string without end-string.
Documentation/security/keys/core.rst:1600: WARNING: Inline literal start-string without end-string.
Documentation/security/keys/core.rst:1600: WARNING: Inline emphasis start-string without end-string.
Documentation/security/keys/core.rst:1600: WARNING: Inline emphasis start-string without end-string.
Documentation/security/keys/core.rst:1600: WARNING: Inline emphasis start-string without end-string.
Documentation/security/keys/core.rst:1600: WARNING: Inline emphasis start-string without end-string.
Documentation/security/keys/core.rst:1666: WARNING: Inline literal start-string without end-string.
Documentation/security/keys/core.rst:1666: WARNING: Inline emphasis start-string without end-string.
Documentation/security/keys/core.rst:1666: WARNING: Inline emphasis start-string without end-string.
Documentation/security/keys/core.rst:1666: WARNING: Inline emphasis start-string without end-string.
Documentation/security/keys/trusted-encrypted.rst:112: WARNING: Literal block expected; none found.
Documentation/security/keys/trusted-encrypted.rst:121: WARNING: Unexpected indentation.
Documentation/security/keys/trusted-encrypted.rst:122: WARNING: Block quote ends without a blank line; unexpected unindent.
Documentation/security/keys/trusted-encrypted.rst:123: WARNING: Block quote ends without a blank line; unexpected unindent.
Documentation/translations/it_IT/process/license-rules.rst:329: WARNING: Literal block expected; none found.
Documentation/translations/it_IT/process/license-rules.rst:332: WARNING: Unexpected indentation.
Documentation/translations/it_IT/process/license-rules.rst:339: WARNING: Block quote ends without a blank line; unexpected unindent.
Documentation/translations/it_IT/process/license-rules.rst:341: WARNING: Unexpected indentation.
Documentation/translations/it_IT/process/license-rules.rst:305: WARNING: Unknown target name: "metatags".
Documentation/translations/zh_CN/process/management-style.rst:34: WARNING: duplicate label decisions, other instance in Documentation/process/management-style.rst
Documentation/translations/zh_CN/process/programming-language.rst:34: WARNING: duplicate citation c-language, other instance in Documentation/process/programming-language.rst
Documentation/translations/zh_CN/process/programming-language.rst:35: WARNING: duplicate citation gcc, other instance in Documentation/process/programming-language.rst
Documentation/translations/zh_CN/process/programming-language.rst:36: WARNING: duplicate citation clang, other instance in Documentation/process/programming-language.rst
Documentation/translations/zh_CN/process/programming-language.rst:37: WARNING: duplicate citation icc, other instance in Documentation/process/programming-language.rst
Documentation/translations/zh_CN/process/programming-language.rst:38: WARNING: duplicate citation gcc-c-dialect-options, other instance in Documentation/process/programming-language.rst
Documentation/translations/zh_CN/process/programming-language.rst:39: WARNING: duplicate citation gnu-extensions, other instance in Documentation/process/programming-language.rst
Documentation/translations/zh_CN/process/programming-language.rst:40: WARNING: duplicate citation gcc-attribute-syntax, other instance in Documentation/process/programming-language.rst
Documentation/translations/zh_CN/process/programming-language.rst:41: WARNING: duplicate citation n2049, other instance in Documentation/process/programming-language.rst
Documentation/accelerators/ocxl.rst: WARNING: document isn't included in any toctree
Documentation/arm/stm32/overview.rst: WARNING: document isn't included in any toctree
Documentation/arm/stm32/stm32f429-overview.rst: WARNING: document isn't included in any toctree
Documentation/arm/stm32/stm32f746-overview.rst: WARNING: document isn't included in any toctree
Documentation/arm/stm32/stm32f769-overview.rst: WARNING: document isn't included in any toctree
Documentation/arm/stm32/stm32h743-overview.rst: WARNING: document isn't included in any toctree
Documentation/arm/stm32/stm32mp157-overview.rst: WARNING: document isn't included in any toctree
Documentation/gpu/msm-crash-dump.rst: WARNING: document isn't included in any toctree
Documentation/interconnect/interconnect.rst: WARNING: document isn't included in any toctree
Documentation/laptops/lg-laptop.rst: WARNING: document isn't included in any toctree
Documentation/powerpc/isa-versions.rst: WARNING: document isn't included in any toctree
Documentation/virtual/kvm/amd-memory-encryption.rst: WARNING: document isn't included in any toctree
Documentation/virtual/kvm/vcpu-requests.rst: WARNING: document isn't included in any toctree
Documentation/virtual/kvm/amd-memory-encryption.rst:244: WARNING: Citation [white-paper] is not referenced.
Documentation/virtual/kvm/amd-memory-encryption.rst:246: WARNING: Citation [amd-apm] is not referenced.
Documentation/virtual/kvm/amd-memory-encryption.rst:247: WARNING: Citation [kvm-forum] is not referenced.
vim +1812 include/linux/lsm_hooks.h
3c4ed7bdf5997d Casey Schaufler 2015-05-02 @1812
:::::: The code at line 1812 was first introduced by commit
:::::: 3c4ed7bdf5997d8020cbb8d4abbef2fcfb9f1284 LSM: Split security.h
:::::: TO: Casey Schaufler <casey@schaufler-ca.com>
:::::: CC: James Morris <james.l.morris@oracle.com>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 7246 bytes --]
^ permalink raw reply
* [security:next-lockdown 8/29] arch/s390/kernel/kexec_elf.c:134:3: error: 'const struct kexec_file_ops' has no member named 'verify_sig'
From: kbuild test robot @ 2019-08-10 8:27 UTC (permalink / raw)
To: Jiri Bohac
Cc: kbuild-all, linux-security-module, James Morris, David Howells,
Matthew Garrett, Dave Young
[-- Attachment #1: Type: text/plain, Size: 2790 bytes --]
tree: https://kernel.googlesource.com/pub/scm/linux/kernel/git/jmorris/linux-security.git next-lockdown
head: 05ef41e93e1a40d6b2d9846284824ec6f67fe422
commit: 47b888368923ec6ccd96c6a654250867def4a2c4 [8/29] kexec_file: split KEXEC_VERIFY_SIG into KEXEC_SIG and KEXEC_SIG_FORCE
config: s390-allmodconfig (attached as .config)
compiler: s390-linux-gcc (GCC) 7.4.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
git checkout 47b888368923ec6ccd96c6a654250867def4a2c4
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=s390
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> arch/s390/kernel/kexec_elf.c:134:3: error: 'const struct kexec_file_ops' has no member named 'verify_sig'
.verify_sig = s390_verify_sig,
^~~~~~~~~~
>> arch/s390/kernel/kexec_elf.c:134:16: error: positional initialization of field in 'struct' declared with 'designated_init' attribute [-Werror=designated-init]
.verify_sig = s390_verify_sig,
^~~~~~~~~~~~~~~
arch/s390/kernel/kexec_elf.c:134:16: note: (near initialization for 's390_kexec_elf_ops')
cc1: some warnings being treated as errors
--
>> arch/s390/kernel/kexec_image.c:63:3: error: 'const struct kexec_file_ops' has no member named 'verify_sig'
.verify_sig = s390_verify_sig,
^~~~~~~~~~
>> arch/s390/kernel/kexec_image.c:63:16: error: positional initialization of field in 'struct' declared with 'designated_init' attribute [-Werror=designated-init]
.verify_sig = s390_verify_sig,
^~~~~~~~~~~~~~~
arch/s390/kernel/kexec_image.c:63:16: note: (near initialization for 's390_kexec_image_ops')
cc1: some warnings being treated as errors
vim +134 arch/s390/kernel/kexec_elf.c
8be01882715466 Philipp Rudo 2017-09-11 129
8be01882715466 Philipp Rudo 2017-09-11 130 const struct kexec_file_ops s390_kexec_elf_ops = {
8be01882715466 Philipp Rudo 2017-09-11 131 .probe = s390_elf_probe,
8be01882715466 Philipp Rudo 2017-09-11 132 .load = s390_elf_load,
e23a8020ce4e09 Philipp Rudo 2019-02-26 133 #ifdef CONFIG_KEXEC_VERIFY_SIG
e23a8020ce4e09 Philipp Rudo 2019-02-26 @134 .verify_sig = s390_verify_sig,
:::::: The code at line 134 was first introduced by commit
:::::: e23a8020ce4e094e10d717d39a8ce799243bf8c1 s390/kexec_file: Signature verification prototype
:::::: TO: Philipp Rudo <prudo@linux.ibm.com>
:::::: CC: Martin Schwidefsky <schwidefsky@de.ibm.com>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 55330 bytes --]
^ permalink raw reply
* Re: [PATCH] fanotify, inotify, dnotify, security: add security hook for fs notifications
From: Amir Goldstein @ 2019-08-10 10:05 UTC (permalink / raw)
To: Paul Moore
Cc: Aaron Goidel, selinux, LSM List, linux-fsdevel, David Howells,
Jan Kara, James Morris, Stephen Smalley, linux-kernel
In-Reply-To: <CAHC9VhRpTuL2Lj1VFwHW4YLpx0hJVSxMnXefooHqsxpEUg6-0A@mail.gmail.com>
> > > Other than Casey's comments, and ACK, I'm not seeing much commentary
> > > on this patch so FS and LSM folks consider this your last chance - if
> > > I don't hear any objections by the end of this week I'll plan on
> > > merging this into selinux/next next week.
> >
> > Please consider it is summer time so people may be on vacation like I was...
>
> This is one of the reasons why I was speaking to the mailing list and
> not a particular individual :)
>
Jan is fsnotify maintainer, so I think you should wait for an explicit ACK
from Jan or just merge the hook definition and ask Jan to merge to
fsnotify security hooks.
Thanks,
Amir.
^ permalink raw reply
* Re: [PATCH] fanotify, inotify, dnotify, security: add security hook for fs notifications
From: Paul Moore @ 2019-08-10 15:01 UTC (permalink / raw)
To: Amir Goldstein
Cc: Aaron Goidel, selinux, LSM List, linux-fsdevel, David Howells,
Jan Kara, James Morris, Stephen Smalley, linux-kernel
In-Reply-To: <CAOQ4uxiGNXbZ-DWeXTkNM4ySFbBbo1XOF1=3pjknsf+EjbNuOw@mail.gmail.com>
On August 10, 2019 6:05:27 AM Amir Goldstein <amir73il@gmail.com> wrote:
>>>> Other than Casey's comments, and ACK, I'm not seeing much commentary
>>>> on this patch so FS and LSM folks consider this your last chance - if
>>>> I don't hear any objections by the end of this week I'll plan on
>>>> merging this into selinux/next next week.
>>>
>>> Please consider it is summer time so people may be on vacation like I was...
>>
>> This is one of the reasons why I was speaking to the mailing list and
>> not a particular individual :)
>
> Jan is fsnotify maintainer, so I think you should wait for an explicit ACK
> from Jan or just merge the hook definition and ask Jan to merge to
> fsnotify security hooks.
Aaron posted his first patch a month ago in the beginning of July and I don't recall seeing any comments from Jan on any of the patch revisions. I would feel much better with an ACK/Reviewed-by from Jan, or you - which is why I sent that email - but I'm not going to wait forever and I'd like to get this into -next soon so we can get some testing.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* [PATCH][next] ima: ima_modsig: Fix use-after-free bug in ima_read_modsig
From: Gustavo A. R. Silva @ 2019-08-11 23:55 UTC (permalink / raw)
To: Mimi Zohar, Dmitry Kasatkin, James Morris, Serge E. Hallyn,
Thiago Jung Bauermann
Cc: linux-integrity, linux-security-module, linux-kernel,
Gustavo A. R. Silva
hdr is being freed and then dereferenced by accessing hdr->pkcs7_msg
Fix this by copying the value returned by PTR_ERR(hdr->pkcs7_msg) into
automatic variable err for its safe use after freeing hdr.
Addresses-Coverity-ID: 1485813 ("Read from pointer after free")
Fixes: 39b07096364a ("ima: Implement support for module-style appended signatures")
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
security/integrity/ima/ima_modsig.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/security/integrity/ima/ima_modsig.c b/security/integrity/ima/ima_modsig.c
index c412e31d1714..e681d4326145 100644
--- a/security/integrity/ima/ima_modsig.c
+++ b/security/integrity/ima/ima_modsig.c
@@ -91,8 +91,9 @@ int ima_read_modsig(enum ima_hooks func, const void *buf, loff_t buf_len,
hdr->pkcs7_msg = pkcs7_parse_message(buf + buf_len, sig_len);
if (IS_ERR(hdr->pkcs7_msg)) {
+ int err = PTR_ERR(hdr->pkcs7_msg);
kfree(hdr);
- return PTR_ERR(hdr->pkcs7_msg);
+ return err;
}
memcpy(hdr->raw_pkcs7, buf + buf_len, sig_len);
--
2.22.0
^ permalink raw reply related
* [PATCH] tracefs: Fix potential null dereference in default_file_open()
From: Ben Hutchings @ 2019-08-12 0:28 UTC (permalink / raw)
To: jmorris
Cc: linux-security-module, Matthew Garrett, Steven Rostedt,
Reinhard Karcher, 934304
[-- Attachment #1: Type: text/plain, Size: 856 bytes --]
The "open" operation in struct file_operations is optional, and
ftrace_event_id_fops does not set it. In default_file_open(), after
all other checks have passed, return 0 if the underlying struct
file_operations does not implement open.
Fixes: 757ff7244358 ("tracefs: Restrict tracefs when the kernel is …")
References: https://bugs.debian.org/934304
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
fs/tracefs/inode.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c
index 34da48036e08..761af8ce4015 100644
--- a/fs/tracefs/inode.c
+++ b/fs/tracefs/inode.c
@@ -42,6 +42,8 @@ static int default_open_file(struct inode *inode, struct file *filp)
return ret;
real_fops = dentry->d_fsdata;
+ if (!real_fops->open)
+ return 0;
return real_fops->open(inode, filp);
}
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* WARNING in aa_sock_msg_perm
From: syzbot @ 2019-08-12 12:30 UTC (permalink / raw)
To: jmorris, john.johansen, linux-kernel, linux-security-module,
netdev, serge, syzkaller-bugs
Hello,
syzbot found the following crash on:
HEAD commit: fcc32a21 liquidio: Use pcie_flr() instead of reimplementin..
git tree: net-next
console output: https://syzkaller.appspot.com/x/log.txt?x=11233726600000
kernel config: https://syzkaller.appspot.com/x/.config?x=d4cf1ffb87d590d7
dashboard link: https://syzkaller.appspot.com/bug?extid=cda1ac91660a61b51495
compiler: gcc (GCC) 9.0.0 20181231 (experimental)
Unfortunately, I don't have any reproducer for this crash yet.
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+cda1ac91660a61b51495@syzkaller.appspotmail.com
------------[ cut here ]------------
AppArmor WARN aa_sock_msg_perm: ((!sock)):
WARNING: CPU: 0 PID: 11187 at security/apparmor/lsm.c:920
aa_sock_msg_perm.isra.0+0xdd/0x170 security/apparmor/lsm.c:920
Kernel panic - not syncing: panic_on_warn set ...
CPU: 0 PID: 11187 Comm: kworker/0:5 Not tainted 5.3.0-rc3+ #124
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Workqueue: krxrpcd rxrpc_peer_keepalive_worker
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x172/0x1f0 lib/dump_stack.c:113
panic+0x2dc/0x755 kernel/panic.c:219
__warn.cold+0x20/0x4c kernel/panic.c:576
report_bug+0x263/0x2b0 lib/bug.c:186
fixup_bug arch/x86/kernel/traps.c:179 [inline]
fixup_bug arch/x86/kernel/traps.c:174 [inline]
do_error_trap+0x11b/0x200 arch/x86/kernel/traps.c:272
do_invalid_op+0x37/0x50 arch/x86/kernel/traps.c:291
invalid_op+0x23/0x30 arch/x86/entry/entry_64.S:1028
RIP: 0010:aa_sock_msg_perm.isra.0+0xdd/0x170 security/apparmor/lsm.c:920
Code: 89 ef e8 66 e6 02 00 5b 41 5c 41 5d 41 5e 41 5f 5d c3 e8 16 25 68 fe
48 c7 c6 a0 8f c0 87 48 c7 c7 a0 7a c0 87 e8 db 97 39 fe <0f> 0b e9 43 ff
ff ff e8 f7 24 68 fe 48 c7 c6 a0 8f c0 87 48 c7 c7
RSP: 0018:ffff8880689f79b0 EFLAGS: 00010286
RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffffffff815c3ba6 RDI: ffffed100d13ef28
RBP: ffff8880689f79d8 R08: ffff88806916e300 R09: fffffbfff11b42c5
R10: fffffbfff11b42c4 R11: ffffffff88da1623 R12: ffff8880689f7b20
R13: ffffffff87c07ee0 R14: 0000000000000002 R15: 000000000000001d
apparmor_socket_sendmsg+0x2a/0x30 security/apparmor/lsm.c:936
security_socket_sendmsg+0x77/0xc0 security/security.c:1973
sock_sendmsg+0x45/0x130 net/socket.c:654
kernel_sendmsg+0x44/0x50 net/socket.c:677
rxrpc_send_keepalive+0x1ff/0x940 net/rxrpc/output.c:656
rxrpc_peer_keepalive_dispatch net/rxrpc/peer_event.c:369 [inline]
rxrpc_peer_keepalive_worker+0x7be/0xd02 net/rxrpc/peer_event.c:430
process_one_work+0x9af/0x1740 kernel/workqueue.c:2269
worker_thread+0x98/0xe40 kernel/workqueue.c:2415
kthread+0x361/0x430 kernel/kthread.c:255
ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352
Kernel Offset: disabled
Rebooting in 86400 seconds..
---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
^ permalink raw reply
* Re: [PATCH] fanotify, inotify, dnotify, security: add security hook for fs notifications
From: Jan Kara @ 2019-08-12 13:41 UTC (permalink / raw)
To: Paul Moore
Cc: Amir Goldstein, Aaron Goidel, selinux, LSM List, linux-fsdevel,
David Howells, Jan Kara, James Morris, Stephen Smalley,
linux-kernel
In-Reply-To: <16c7c0c4a60.280e.85c95baa4474aabc7814e68940a78392@paul-moore.com>
On Sat 10-08-19 11:01:16, Paul Moore wrote:
> On August 10, 2019 6:05:27 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> >>>> Other than Casey's comments, and ACK, I'm not seeing much commentary
> >>>> on this patch so FS and LSM folks consider this your last chance - if
> >>>> I don't hear any objections by the end of this week I'll plan on
> >>>> merging this into selinux/next next week.
> >>>
> >>> Please consider it is summer time so people may be on vacation like I was...
> >>
> >> This is one of the reasons why I was speaking to the mailing list and
> >> not a particular individual :)
> >
> > Jan is fsnotify maintainer, so I think you should wait for an explicit ACK
> > from Jan or just merge the hook definition and ask Jan to merge to
> > fsnotify security hooks.
>
> Aaron posted his first patch a month ago in the beginning of July and I
> don't recall seeing any comments from Jan on any of the patch revisions.
> I would feel much better with an ACK/Reviewed-by from Jan, or you - which
> is why I sent that email - but I'm not going to wait forever and I'd like
> to get this into -next soon so we can get some testing.
Yeah, sorry for the delays. I'm aware of the patch but I was also on
vacation and pretty busy at work so Amir always beat me in commenting on
the patch and I didn't have much to add. Once Aaron fixes the latest
comments from Amir, I'll give the patch the final look and give my ack.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* Re: [Non-DoD Source] Re: [PATCH] fanotify, inotify, dnotify, security: add security hook for fs notifications
From: Aaron Goidel @ 2019-08-12 13:49 UTC (permalink / raw)
To: Jan Kara, Paul Moore
Cc: Amir Goldstein, selinux, LSM List, linux-fsdevel, David Howells,
James Morris, Stephen Smalley, linux-kernel
In-Reply-To: <20190812134145.GA11343@quack2.suse.cz>
On 8/12/19 9:41 AM, Jan Kara wrote:
> On Sat 10-08-19 11:01:16, Paul Moore wrote:
>> On August 10, 2019 6:05:27 AM Amir Goldstein <amir73il@gmail.com> wrote:
>>
>>>>>> Other than Casey's comments, and ACK, I'm not seeing much commentary
>>>>>> on this patch so FS and LSM folks consider this your last chance - if
>>>>>> I don't hear any objections by the end of this week I'll plan on
>>>>>> merging this into selinux/next next week.
>>>>>
>>>>> Please consider it is summer time so people may be on vacation like I was...
>>>>
>>>> This is one of the reasons why I was speaking to the mailing list and
>>>> not a particular individual :)
>>>
>>> Jan is fsnotify maintainer, so I think you should wait for an explicit ACK
>>> from Jan or just merge the hook definition and ask Jan to merge to
>>> fsnotify security hooks.
>>
>> Aaron posted his first patch a month ago in the beginning of July and I
>> don't recall seeing any comments from Jan on any of the patch revisions.
>> I would feel much better with an ACK/Reviewed-by from Jan, or you - which
>> is why I sent that email - but I'm not going to wait forever and I'd like
>> to get this into -next soon so we can get some testing.
>
> Yeah, sorry for the delays. I'm aware of the patch but I was also on
> vacation and pretty busy at work so Amir always beat me in commenting on
> the patch and I didn't have much to add. Once Aaron fixes the latest
> comments from Amir, I'll give the patch the final look and give my ack.
>
> Honza
>
I already re-spun the patch with the changes Amir and I agreed to. There
was an email with the PATCH v2. It may have flown under the radar a bit,
so just wanted to point that out.
--
Aaron
^ permalink raw reply
* Re: [PATCH v2] fanotify, inotify, dnotify, security: add security hook for fs notifications
From: Jan Kara @ 2019-08-12 14:06 UTC (permalink / raw)
To: Aaron Goidel
Cc: paul, selinux, linux-security-module, linux-fsdevel, dhowells,
jack, amir73il, jmorris, sds, linux-kernel, Casey Schaufler
In-Reply-To: <20190809181401.7086-1-acgoide@tycho.nsa.gov>
On Fri 09-08-19 14:14:01, Aaron Goidel wrote:
> As of now, setting watches on filesystem objects has, at most, applied a
> check for read access to the inode, and in the case of fanotify, requires
> CAP_SYS_ADMIN. No specific security hook or permission check has been
> provided to control the setting of watches. Using any of inotify, dnotify,
> or fanotify, it is possible to observe, not only write-like operations, but
> even read access to a file. Modeling the watch as being merely a read from
> the file is insufficient for the needs of SELinux. This is due to the fact
> that read access should not necessarily imply access to information about
> when another process reads from a file. Furthermore, fanotify watches grant
> more power to an application in the form of permission events. While
> notification events are solely, unidirectional (i.e. they only pass
> information to the receiving application), permission events are blocking.
> Permission events make a request to the receiving application which will
> then reply with a decision as to whether or not that action may be
> completed. This causes the issue of the watching application having the
> ability to exercise control over the triggering process. Without drawing a
> distinction within the permission check, the ability to read would imply
> the greater ability to control an application. Additionally, mount and
> superblock watches apply to all files within the same mount or superblock.
> Read access to one file should not necessarily imply the ability to watch
> all files accessed within a given mount or superblock.
>
> In order to solve these issues, a new LSM hook is implemented and has been
> placed within the system calls for marking filesystem objects with inotify,
> fanotify, and dnotify watches. These calls to the hook are placed at the
> point at which the target path has been resolved and are provided with the
> path struct, the mask of requested notification events, and the type of
> object on which the mark is being set (inode, superblock, or mount). The
> mask and obj_type have already been translated into common FS_* values
> shared by the entirety of the fs notification infrastructure. The path
> struct is passed rather than just the inode so that the mount is available,
> particularly for mount watches. This also allows for use of the hook by
> pathname-based security modules. However, since the hook is intended for
> use even by inode based security modules, it is not placed under the
> CONFIG_SECURITY_PATH conditional. Otherwise, the inode-based security
> modules would need to enable all of the path hooks, even though they do not
> use any of them.
>
> This only provides a hook at the point of setting a watch, and presumes
> that permission to set a particular watch implies the ability to receive
> all notification about that object which match the mask. This is all that
> is required for SELinux. If other security modules require additional hooks
> or infrastructure to control delivery of notification, these can be added
> by them. It does not make sense for us to propose hooks for which we have
> no implementation. The understanding that all notifications received by the
> requesting application are all strictly of a type for which the application
> has been granted permission shows that this implementation is sufficient in
> its coverage.
>
> Security modules wishing to provide complete control over fanotify must
> also implement a security_file_open hook that validates that the access
> requested by the watching application is authorized. Fanotify has the issue
> that it returns a file descriptor with the file mode specified during
> fanotify_init() to the watching process on event. This is already covered
> by the LSM security_file_open hook if the security module implements
> checking of the requested file mode there. Otherwise, a watching process
> can obtain escalated access to a file for which it has not been authorized.
>
> The selinux_path_notify hook implementation works by adding five new file
> permissions: watch, watch_mount, watch_sb, watch_reads, and watch_with_perm
> (descriptions about which will follow), and one new filesystem permission:
> watch (which is applied to superblock checks). The hook then decides which
> subset of these permissions must be held by the requesting application
> based on the contents of the provided mask and the obj_type. The
> selinux_file_open hook already checks the requested file mode and therefore
> ensures that a watching process cannot escalate its access through
> fanotify.
>
> The watch, watch_mount, and watch_sb permissions are the baseline
> permissions for setting a watch on an object and each are a requirement for
> any watch to be set on a file, mount, or superblock respectively. It should
> be noted that having either of the other two permissions (watch_reads and
> watch_with_perm) does not imply the watch, watch_mount, or watch_sb
> permission. Superblock watches further require the filesystem watch
> permission to the superblock. As there is no labeled object in view for
> mounts, there is no specific check for mount watches beyond watch_mount to
> the inode. Such a check could be added in the future, if a suitable labeled
> object existed representing the mount.
>
> The watch_reads permission is required to receive notifications from
> read-exclusive events on filesystem objects. These events include accessing
> a file for the purpose of reading and closing a file which has been opened
> read-only. This distinction has been drawn in order to provide a direct
> indication in the policy for this otherwise not obvious capability. Read
> access to a file should not necessarily imply the ability to observe read
> events on a file.
>
> Finally, watch_with_perm only applies to fanotify masks since it is the
> only way to set a mask which allows for the blocking, permission event.
> This permission is needed for any watch which is of this type. Though
> fanotify requires CAP_SYS_ADMIN, this is insufficient as it gives implicit
> trust to root, which we do not do, and does not support least privilege.
>
> Signed-off-by: Aaron Goidel <acgoide@tycho.nsa.gov>
> Acked-by: Casey Schaufler <casey@schaufler-ca.com>
The fsnotify bits look good to me in this patch so feel free to add:
Acked-by: Jan Kara <jack@suse.cz>
Honza
> ---
> v2:
> - move initialization of obj_type up to remove duplicate work
> - convert inotify and fanotify flags to common FS_* flags
> fs/notify/dnotify/dnotify.c | 15 +++++++--
> fs/notify/fanotify/fanotify_user.c | 19 ++++++++++--
> fs/notify/inotify/inotify_user.c | 14 +++++++--
> include/linux/lsm_hooks.h | 9 +++++-
> include/linux/security.h | 10 ++++--
> security/security.c | 6 ++++
> security/selinux/hooks.c | 47 +++++++++++++++++++++++++++++
> security/selinux/include/classmap.h | 5 +--
> 8 files changed, 113 insertions(+), 12 deletions(-)
>
> diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
> index 250369d6901d..87a7f61bc91c 100644
> --- a/fs/notify/dnotify/dnotify.c
> +++ b/fs/notify/dnotify/dnotify.c
> @@ -22,6 +22,7 @@
> #include <linux/sched/signal.h>
> #include <linux/dnotify.h>
> #include <linux/init.h>
> +#include <linux/security.h>
> #include <linux/spinlock.h>
> #include <linux/slab.h>
> #include <linux/fdtable.h>
> @@ -288,6 +289,17 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
> goto out_err;
> }
>
> + /*
> + * convert the userspace DN_* "arg" to the internal FS_*
> + * defined in fsnotify
> + */
> + mask = convert_arg(arg);
> +
> + error = security_path_notify(&filp->f_path, mask,
> + FSNOTIFY_OBJ_TYPE_INODE);
> + if (error)
> + goto out_err;
> +
> /* expect most fcntl to add new rather than augment old */
> dn = kmem_cache_alloc(dnotify_struct_cache, GFP_KERNEL);
> if (!dn) {
> @@ -302,9 +314,6 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
> goto out_err;
> }
>
> - /* convert the userspace DN_* "arg" to the internal FS_* defines in fsnotify */
> - mask = convert_arg(arg);
> -
> /* set up the new_fsn_mark and new_dn_mark */
> new_fsn_mark = &new_dn_mark->fsn_mark;
> fsnotify_init_mark(new_fsn_mark, dnotify_group);
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index a90bb19dcfa2..8b4e2ad6d208 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -528,7 +528,8 @@ static const struct file_operations fanotify_fops = {
> };
>
> static int fanotify_find_path(int dfd, const char __user *filename,
> - struct path *path, unsigned int flags)
> + struct path *path, unsigned int flags, __u64 mask,
> + unsigned int obj_type)
> {
> int ret;
>
> @@ -567,8 +568,15 @@ static int fanotify_find_path(int dfd, const char __user *filename,
>
> /* you can only watch an inode if you have read permissions on it */
> ret = inode_permission(path->dentry->d_inode, MAY_READ);
> + if (ret) {
> + path_put(path);
> + goto out;
> + }
> +
> + ret = security_path_notify(path, mask, obj_type);
> if (ret)
> path_put(path);
> +
> out:
> return ret;
> }
> @@ -931,6 +939,7 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
> __kernel_fsid_t __fsid, *fsid = NULL;
> u32 valid_mask = FANOTIFY_EVENTS | FANOTIFY_EVENT_FLAGS;
> unsigned int mark_type = flags & FANOTIFY_MARK_TYPE_BITS;
> + unsigned int obj_type;
> int ret;
>
> pr_debug("%s: fanotify_fd=%d flags=%x dfd=%d pathname=%p mask=%llx\n",
> @@ -945,8 +954,13 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
>
> switch (mark_type) {
> case FAN_MARK_INODE:
> + obj_type = FSNOTIFY_OBJ_TYPE_INODE;
> + break;
> case FAN_MARK_MOUNT:
> + obj_type = FSNOTIFY_OBJ_TYPE_VFSMOUNT;
> + break;
> case FAN_MARK_FILESYSTEM:
> + obj_type = FSNOTIFY_OBJ_TYPE_SB;
> break;
> default:
> return -EINVAL;
> @@ -1014,7 +1028,8 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
> goto fput_and_out;
> }
>
> - ret = fanotify_find_path(dfd, pathname, &path, flags);
> + ret = fanotify_find_path(dfd, pathname, &path, flags,
> + (mask & ALL_FSNOTIFY_EVENTS), obj_type);
> if (ret)
> goto fput_and_out;
>
> diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
> index 7b53598c8804..408e9917ed42 100644
> --- a/fs/notify/inotify/inotify_user.c
> +++ b/fs/notify/inotify/inotify_user.c
> @@ -39,6 +39,7 @@
> #include <linux/poll.h>
> #include <linux/wait.h>
> #include <linux/memcontrol.h>
> +#include <linux/security.h>
>
> #include "inotify.h"
> #include "../fdinfo.h"
> @@ -342,7 +343,8 @@ static const struct file_operations inotify_fops = {
> /*
> * find_inode - resolve a user-given path to a specific inode
> */
> -static int inotify_find_inode(const char __user *dirname, struct path *path, unsigned flags)
> +static int inotify_find_inode(const char __user *dirname, struct path *path,
> + unsigned int flags, __u64 mask)
> {
> int error;
>
> @@ -351,8 +353,15 @@ static int inotify_find_inode(const char __user *dirname, struct path *path, uns
> return error;
> /* you can only watch an inode if you have read permissions on it */
> error = inode_permission(path->dentry->d_inode, MAY_READ);
> + if (error) {
> + path_put(path);
> + return error;
> + }
> + error = security_path_notify(path, mask,
> + FSNOTIFY_OBJ_TYPE_INODE);
> if (error)
> path_put(path);
> +
> return error;
> }
>
> @@ -744,7 +753,8 @@ SYSCALL_DEFINE3(inotify_add_watch, int, fd, const char __user *, pathname,
> if (mask & IN_ONLYDIR)
> flags |= LOOKUP_DIRECTORY;
>
> - ret = inotify_find_inode(pathname, &path, flags);
> + ret = inotify_find_inode(pathname, &path, flags,
> + (mask & IN_ALL_EVENTS));
> if (ret)
> goto fput_and_out;
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 47f58cfb6a19..ead98af9c602 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -339,6 +339,9 @@
> * Check for permission to change root directory.
> * @path contains the path structure.
> * Return 0 if permission is granted.
> + * @path_notify:
> + * Check permissions before setting a watch on events as defined by @mask,
> + * on an object at @path, whose type is defined by @obj_type.
> * @inode_readlink:
> * Check the permission to read the symbolic link.
> * @dentry contains the dentry structure for the file link.
> @@ -1535,7 +1538,9 @@ union security_list_options {
> int (*path_chown)(const struct path *path, kuid_t uid, kgid_t gid);
> int (*path_chroot)(const struct path *path);
> #endif
> -
> + /* Needed for inode based security check */
> + int (*path_notify)(const struct path *path, u64 mask,
> + unsigned int obj_type);
> int (*inode_alloc_security)(struct inode *inode);
> void (*inode_free_security)(struct inode *inode);
> int (*inode_init_security)(struct inode *inode, struct inode *dir,
> @@ -1860,6 +1865,8 @@ struct security_hook_heads {
> struct hlist_head path_chown;
> struct hlist_head path_chroot;
> #endif
> + /* Needed for inode based modules as well */
> + struct hlist_head path_notify;
> struct hlist_head inode_alloc_security;
> struct hlist_head inode_free_security;
> struct hlist_head inode_init_security;
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 659071c2e57c..7d9c1da1f659 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -259,7 +259,8 @@ int security_dentry_create_files_as(struct dentry *dentry, int mode,
> struct qstr *name,
> const struct cred *old,
> struct cred *new);
> -
> +int security_path_notify(const struct path *path, u64 mask,
> + unsigned int obj_type);
> int security_inode_alloc(struct inode *inode);
> void security_inode_free(struct inode *inode);
> int security_inode_init_security(struct inode *inode, struct inode *dir,
> @@ -387,7 +388,6 @@ int security_ismaclabel(const char *name);
> int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen);
> int security_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid);
> void security_release_secctx(char *secdata, u32 seclen);
> -
> void security_inode_invalidate_secctx(struct inode *inode);
> int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
> int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
> @@ -621,6 +621,12 @@ static inline int security_move_mount(const struct path *from_path,
> return 0;
> }
>
> +static inline int security_path_notify(const struct path *path, u64 mask,
> + unsigned int obj_type)
> +{
> + return 0;
> +}
> +
> static inline int security_inode_alloc(struct inode *inode)
> {
> return 0;
> diff --git a/security/security.c b/security/security.c
> index 613a5c00e602..30687e1366b7 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -871,6 +871,12 @@ int security_move_mount(const struct path *from_path, const struct path *to_path
> return call_int_hook(move_mount, 0, from_path, to_path);
> }
>
> +int security_path_notify(const struct path *path, u64 mask,
> + unsigned int obj_type)
> +{
> + return call_int_hook(path_notify, 0, path, mask, obj_type);
> +}
> +
> int security_inode_alloc(struct inode *inode)
> {
> int rc = lsm_inode_alloc(inode);
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index f77b314d0575..a47376d1c924 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -92,6 +92,8 @@
> #include <linux/kernfs.h>
> #include <linux/stringhash.h> /* for hashlen_string() */
> #include <uapi/linux/mount.h>
> +#include <linux/fsnotify.h>
> +#include <linux/fanotify.h>
>
> #include "avc.h"
> #include "objsec.h"
> @@ -3261,6 +3263,50 @@ static int selinux_inode_removexattr(struct dentry *dentry, const char *name)
> return -EACCES;
> }
>
> +static int selinux_path_notify(const struct path *path, u64 mask,
> + unsigned int obj_type)
> +{
> + int ret;
> + u32 perm;
> +
> + struct common_audit_data ad;
> +
> + ad.type = LSM_AUDIT_DATA_PATH;
> + ad.u.path = *path;
> +
> + /*
> + * Set permission needed based on the type of mark being set.
> + * Performs an additional check for sb watches.
> + */
> + switch (obj_type) {
> + case FSNOTIFY_OBJ_TYPE_VFSMOUNT:
> + perm = FILE__WATCH_MOUNT;
> + break;
> + case FSNOTIFY_OBJ_TYPE_SB:
> + perm = FILE__WATCH_SB;
> + ret = superblock_has_perm(current_cred(), path->dentry->d_sb,
> + FILESYSTEM__WATCH, &ad);
> + if (ret)
> + return ret;
> + break;
> + case FSNOTIFY_OBJ_TYPE_INODE:
> + perm = FILE__WATCH;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + // check if the mask is requesting ability to set a blocking watch
> + if (mask & (ALL_FSNOTIFY_PERM_EVENTS))
> + perm |= FILE__WATCH_WITH_PERM; // if so, check that permission
> +
> + // is the mask asking to watch file reads?
> + if (mask & (FS_ACCESS | FS_ACCESS_PERM | FS_CLOSE_NOWRITE))
> + perm |= FILE__WATCH_READS; // check that permission as well
> +
> + return path_has_perm(current_cred(), path, perm);
> +}
> +
> /*
> * Copy the inode security context value to the user.
> *
> @@ -6798,6 +6844,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
> LSM_HOOK_INIT(inode_getsecid, selinux_inode_getsecid),
> LSM_HOOK_INIT(inode_copy_up, selinux_inode_copy_up),
> LSM_HOOK_INIT(inode_copy_up_xattr, selinux_inode_copy_up_xattr),
> + LSM_HOOK_INIT(path_notify, selinux_path_notify),
>
> LSM_HOOK_INIT(kernfs_init_security, selinux_kernfs_init_security),
>
> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
> index 201f7e588a29..32e9b03be3dd 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -7,7 +7,8 @@
>
> #define COMMON_FILE_PERMS COMMON_FILE_SOCK_PERMS, "unlink", "link", \
> "rename", "execute", "quotaon", "mounton", "audit_access", \
> - "open", "execmod"
> + "open", "execmod", "watch", "watch_mount", "watch_sb", \
> + "watch_with_perm", "watch_reads"
>
> #define COMMON_SOCK_PERMS COMMON_FILE_SOCK_PERMS, "bind", "connect", \
> "listen", "accept", "getopt", "setopt", "shutdown", "recvfrom", \
> @@ -60,7 +61,7 @@ struct security_class_mapping secclass_map[] = {
> { "filesystem",
> { "mount", "remount", "unmount", "getattr",
> "relabelfrom", "relabelto", "associate", "quotamod",
> - "quotaget", NULL } },
> + "quotaget", "watch", NULL } },
> { "file",
> { COMMON_FILE_PERMS,
> "execute_no_trans", "entrypoint", NULL } },
> --
> 2.21.0
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* Re: [PATCH] fanotify, inotify, dnotify, security: add security hook for fs notifications
From: Paul Moore @ 2019-08-12 14:45 UTC (permalink / raw)
To: Jan Kara
Cc: Amir Goldstein, Aaron Goidel, selinux, LSM List, linux-fsdevel,
David Howells, James Morris, Stephen Smalley, linux-kernel
In-Reply-To: <20190812134145.GA11343@quack2.suse.cz>
On Mon, Aug 12, 2019 at 9:41 AM Jan Kara <jack@suse.cz> wrote:
> On Sat 10-08-19 11:01:16, Paul Moore wrote:
> > On August 10, 2019 6:05:27 AM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > >>>> Other than Casey's comments, and ACK, I'm not seeing much commentary
> > >>>> on this patch so FS and LSM folks consider this your last chance - if
> > >>>> I don't hear any objections by the end of this week I'll plan on
> > >>>> merging this into selinux/next next week.
> > >>>
> > >>> Please consider it is summer time so people may be on vacation like I was...
> > >>
> > >> This is one of the reasons why I was speaking to the mailing list and
> > >> not a particular individual :)
> > >
> > > Jan is fsnotify maintainer, so I think you should wait for an explicit ACK
> > > from Jan or just merge the hook definition and ask Jan to merge to
> > > fsnotify security hooks.
> >
> > Aaron posted his first patch a month ago in the beginning of July and I
> > don't recall seeing any comments from Jan on any of the patch revisions.
> > I would feel much better with an ACK/Reviewed-by from Jan, or you - which
> > is why I sent that email - but I'm not going to wait forever and I'd like
> > to get this into -next soon so we can get some testing.
>
> Yeah, sorry for the delays. I'm aware of the patch but I was also on
> vacation and pretty busy at work so Amir always beat me in commenting on
> the patch and I didn't have much to add. Once Aaron fixes the latest
> comments from Amir, I'll give the patch the final look and give my ack.
That is prefect, thanks.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH v2] fanotify, inotify, dnotify, security: add security hook for fs notifications
From: Paul Moore @ 2019-08-12 15:06 UTC (permalink / raw)
To: Aaron Goidel, Stephen Smalley
Cc: selinux, linux-security-module, linux-fsdevel, dhowells, jack,
amir73il, James Morris, linux-kernel, Casey Schaufler
In-Reply-To: <20190809181401.7086-1-acgoide@tycho.nsa.gov>
On Fri, Aug 9, 2019 at 2:14 PM Aaron Goidel <acgoide@tycho.nsa.gov> wrote:
> As of now, setting watches on filesystem objects has, at most, applied a
> check for read access to the inode, and in the case of fanotify, requires
> CAP_SYS_ADMIN. No specific security hook or permission check has been
> provided to control the setting of watches. Using any of inotify, dnotify,
> or fanotify, it is possible to observe, not only write-like operations, but
> even read access to a file. Modeling the watch as being merely a read from
> the file is insufficient for the needs of SELinux. This is due to the fact
> that read access should not necessarily imply access to information about
> when another process reads from a file. Furthermore, fanotify watches grant
> more power to an application in the form of permission events. While
> notification events are solely, unidirectional (i.e. they only pass
> information to the receiving application), permission events are blocking.
> Permission events make a request to the receiving application which will
> then reply with a decision as to whether or not that action may be
> completed. This causes the issue of the watching application having the
> ability to exercise control over the triggering process. Without drawing a
> distinction within the permission check, the ability to read would imply
> the greater ability to control an application. Additionally, mount and
> superblock watches apply to all files within the same mount or superblock.
> Read access to one file should not necessarily imply the ability to watch
> all files accessed within a given mount or superblock.
>
> In order to solve these issues, a new LSM hook is implemented and has been
> placed within the system calls for marking filesystem objects with inotify,
> fanotify, and dnotify watches. These calls to the hook are placed at the
> point at which the target path has been resolved and are provided with the
> path struct, the mask of requested notification events, and the type of
> object on which the mark is being set (inode, superblock, or mount). The
> mask and obj_type have already been translated into common FS_* values
> shared by the entirety of the fs notification infrastructure. The path
> struct is passed rather than just the inode so that the mount is available,
> particularly for mount watches. This also allows for use of the hook by
> pathname-based security modules. However, since the hook is intended for
> use even by inode based security modules, it is not placed under the
> CONFIG_SECURITY_PATH conditional. Otherwise, the inode-based security
> modules would need to enable all of the path hooks, even though they do not
> use any of them.
>
> This only provides a hook at the point of setting a watch, and presumes
> that permission to set a particular watch implies the ability to receive
> all notification about that object which match the mask. This is all that
> is required for SELinux. If other security modules require additional hooks
> or infrastructure to control delivery of notification, these can be added
> by them. It does not make sense for us to propose hooks for which we have
> no implementation. The understanding that all notifications received by the
> requesting application are all strictly of a type for which the application
> has been granted permission shows that this implementation is sufficient in
> its coverage.
>
> Security modules wishing to provide complete control over fanotify must
> also implement a security_file_open hook that validates that the access
> requested by the watching application is authorized. Fanotify has the issue
> that it returns a file descriptor with the file mode specified during
> fanotify_init() to the watching process on event. This is already covered
> by the LSM security_file_open hook if the security module implements
> checking of the requested file mode there. Otherwise, a watching process
> can obtain escalated access to a file for which it has not been authorized.
>
> The selinux_path_notify hook implementation works by adding five new file
> permissions: watch, watch_mount, watch_sb, watch_reads, and watch_with_perm
> (descriptions about which will follow), and one new filesystem permission:
> watch (which is applied to superblock checks). The hook then decides which
> subset of these permissions must be held by the requesting application
> based on the contents of the provided mask and the obj_type. The
> selinux_file_open hook already checks the requested file mode and therefore
> ensures that a watching process cannot escalate its access through
> fanotify.
>
> The watch, watch_mount, and watch_sb permissions are the baseline
> permissions for setting a watch on an object and each are a requirement for
> any watch to be set on a file, mount, or superblock respectively. It should
> be noted that having either of the other two permissions (watch_reads and
> watch_with_perm) does not imply the watch, watch_mount, or watch_sb
> permission. Superblock watches further require the filesystem watch
> permission to the superblock. As there is no labeled object in view for
> mounts, there is no specific check for mount watches beyond watch_mount to
> the inode. Such a check could be added in the future, if a suitable labeled
> object existed representing the mount.
>
> The watch_reads permission is required to receive notifications from
> read-exclusive events on filesystem objects. These events include accessing
> a file for the purpose of reading and closing a file which has been opened
> read-only. This distinction has been drawn in order to provide a direct
> indication in the policy for this otherwise not obvious capability. Read
> access to a file should not necessarily imply the ability to observe read
> events on a file.
>
> Finally, watch_with_perm only applies to fanotify masks since it is the
> only way to set a mask which allows for the blocking, permission event.
> This permission is needed for any watch which is of this type. Though
> fanotify requires CAP_SYS_ADMIN, this is insufficient as it gives implicit
> trust to root, which we do not do, and does not support least privilege.
>
> Signed-off-by: Aaron Goidel <acgoide@tycho.nsa.gov>
> Acked-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> v2:
> - move initialization of obj_type up to remove duplicate work
> - convert inotify and fanotify flags to common FS_* flags
> fs/notify/dnotify/dnotify.c | 15 +++++++--
> fs/notify/fanotify/fanotify_user.c | 19 ++++++++++--
> fs/notify/inotify/inotify_user.c | 14 +++++++--
> include/linux/lsm_hooks.h | 9 +++++-
> include/linux/security.h | 10 ++++--
> security/security.c | 6 ++++
> security/selinux/hooks.c | 47 +++++++++++++++++++++++++++++
> security/selinux/include/classmap.h | 5 +--
> 8 files changed, 113 insertions(+), 12 deletions(-)
...
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index f77b314d0575..a47376d1c924 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3261,6 +3263,50 @@ static int selinux_inode_removexattr(struct dentry *dentry, const char *name)
> return -EACCES;
> }
>
> +static int selinux_path_notify(const struct path *path, u64 mask,
> + unsigned int obj_type)
> +{
> + int ret;
> + u32 perm;
> +
> + struct common_audit_data ad;
> +
> + ad.type = LSM_AUDIT_DATA_PATH;
> + ad.u.path = *path;
> +
> + /*
> + * Set permission needed based on the type of mark being set.
> + * Performs an additional check for sb watches.
> + */
> + switch (obj_type) {
> + case FSNOTIFY_OBJ_TYPE_VFSMOUNT:
> + perm = FILE__WATCH_MOUNT;
> + break;
> + case FSNOTIFY_OBJ_TYPE_SB:
> + perm = FILE__WATCH_SB;
> + ret = superblock_has_perm(current_cred(), path->dentry->d_sb,
> + FILESYSTEM__WATCH, &ad);
> + if (ret)
> + return ret;
> + break;
> + case FSNOTIFY_OBJ_TYPE_INODE:
> + perm = FILE__WATCH;
> + break;
> + default:
> + return -EINVAL;
> + }
Sigh.
Remember when I said "Don't respin the patch just for this, but if you
have to do it for some other reason please fix the C++ style
comments."? In this particular case it is a small thing, but a
failure to incorporate all the feedback is one of the things that
really annoys me (mostly because it makes me worry about other things
that may have been missed). It isn't as bad as submitting code which
doesn't compile, but it's a close second.
At this point I'm going to ask you to respin the patch to get rid of
those C++ style comments. I'm also going to get a bit more nitpicky
about those comments too (more comments below).
> + // check if the mask is requesting ability to set a blocking watch
> + if (mask & (ALL_FSNOTIFY_PERM_EVENTS))
> + perm |= FILE__WATCH_WITH_PERM; // if so, check that permission
What is the point of that trailing comment "if so, check that
permission"? Given the code, and the comment two lines above this
seems obvious, does it not? If you want to keep it, that's fine with
me, but let's combine the two comments so they read a bit better, for
example:
/* blocking watches require the file:watch_with_perm permission */
if (...)
perm |= FILE__WATCH_WITH_PERM;
> + // is the mask asking to watch file reads?
> + if (mask & (FS_ACCESS | FS_ACCESS_PERM | FS_CLOSE_NOWRITE))
> + perm |= FILE__WATCH_READS; // check that permission as well
Here the "check that permission as well" adds no additional useful
information, it's just noise in the code, drop it from the patch.
I am a believer in the old advice that good comments explain *why* the
code is doing something, where bad comments explain *what* the code is
doing. I would kindly ask that you keep that in mind when submitting
future SELinux patches.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [Non-DoD Source] Re: [PATCH v2] fanotify, inotify, dnotify, security: add security hook for fs notifications
From: Aaron Goidel @ 2019-08-12 15:16 UTC (permalink / raw)
To: Paul Moore, Stephen Smalley
Cc: selinux, linux-security-module, linux-fsdevel, dhowells, jack,
amir73il, James Morris, linux-kernel, Casey Schaufler
In-Reply-To: <CAHC9VhTXjvUHQsZT9Fd6m=yzdBTDAzf1SpfMxQ_qwjy6zbJZLg@mail.gmail.com>
On 8/12/19 11:06 AM, Paul Moore wrote:
> On Fri, Aug 9, 2019 at 2:14 PM Aaron Goidel <acgoide@tycho.nsa.gov> wrote:
>> As of now, setting watches on filesystem objects has, at most, applied a
>> check for read access to the inode, and in the case of fanotify, requires
>> CAP_SYS_ADMIN. No specific security hook or permission check has been
>> provided to control the setting of watches. Using any of inotify, dnotify,
>> or fanotify, it is possible to observe, not only write-like operations, but
>> even read access to a file. Modeling the watch as being merely a read from
>> the file is insufficient for the needs of SELinux. This is due to the fact
>> that read access should not necessarily imply access to information about
>> when another process reads from a file. Furthermore, fanotify watches grant
>> more power to an application in the form of permission events. While
>> notification events are solely, unidirectional (i.e. they only pass
>> information to the receiving application), permission events are blocking.
>> Permission events make a request to the receiving application which will
>> then reply with a decision as to whether or not that action may be
>> completed. This causes the issue of the watching application having the
>> ability to exercise control over the triggering process. Without drawing a
>> distinction within the permission check, the ability to read would imply
>> the greater ability to control an application. Additionally, mount and
>> superblock watches apply to all files within the same mount or superblock.
>> Read access to one file should not necessarily imply the ability to watch
>> all files accessed within a given mount or superblock.
>>
>> In order to solve these issues, a new LSM hook is implemented and has been
>> placed within the system calls for marking filesystem objects with inotify,
>> fanotify, and dnotify watches. These calls to the hook are placed at the
>> point at which the target path has been resolved and are provided with the
>> path struct, the mask of requested notification events, and the type of
>> object on which the mark is being set (inode, superblock, or mount). The
>> mask and obj_type have already been translated into common FS_* values
>> shared by the entirety of the fs notification infrastructure. The path
>> struct is passed rather than just the inode so that the mount is available,
>> particularly for mount watches. This also allows for use of the hook by
>> pathname-based security modules. However, since the hook is intended for
>> use even by inode based security modules, it is not placed under the
>> CONFIG_SECURITY_PATH conditional. Otherwise, the inode-based security
>> modules would need to enable all of the path hooks, even though they do not
>> use any of them.
>>
>> This only provides a hook at the point of setting a watch, and presumes
>> that permission to set a particular watch implies the ability to receive
>> all notification about that object which match the mask. This is all that
>> is required for SELinux. If other security modules require additional hooks
>> or infrastructure to control delivery of notification, these can be added
>> by them. It does not make sense for us to propose hooks for which we have
>> no implementation. The understanding that all notifications received by the
>> requesting application are all strictly of a type for which the application
>> has been granted permission shows that this implementation is sufficient in
>> its coverage.
>>
>> Security modules wishing to provide complete control over fanotify must
>> also implement a security_file_open hook that validates that the access
>> requested by the watching application is authorized. Fanotify has the issue
>> that it returns a file descriptor with the file mode specified during
>> fanotify_init() to the watching process on event. This is already covered
>> by the LSM security_file_open hook if the security module implements
>> checking of the requested file mode there. Otherwise, a watching process
>> can obtain escalated access to a file for which it has not been authorized.
>>
>> The selinux_path_notify hook implementation works by adding five new file
>> permissions: watch, watch_mount, watch_sb, watch_reads, and watch_with_perm
>> (descriptions about which will follow), and one new filesystem permission:
>> watch (which is applied to superblock checks). The hook then decides which
>> subset of these permissions must be held by the requesting application
>> based on the contents of the provided mask and the obj_type. The
>> selinux_file_open hook already checks the requested file mode and therefore
>> ensures that a watching process cannot escalate its access through
>> fanotify.
>>
>> The watch, watch_mount, and watch_sb permissions are the baseline
>> permissions for setting a watch on an object and each are a requirement for
>> any watch to be set on a file, mount, or superblock respectively. It should
>> be noted that having either of the other two permissions (watch_reads and
>> watch_with_perm) does not imply the watch, watch_mount, or watch_sb
>> permission. Superblock watches further require the filesystem watch
>> permission to the superblock. As there is no labeled object in view for
>> mounts, there is no specific check for mount watches beyond watch_mount to
>> the inode. Such a check could be added in the future, if a suitable labeled
>> object existed representing the mount.
>>
>> The watch_reads permission is required to receive notifications from
>> read-exclusive events on filesystem objects. These events include accessing
>> a file for the purpose of reading and closing a file which has been opened
>> read-only. This distinction has been drawn in order to provide a direct
>> indication in the policy for this otherwise not obvious capability. Read
>> access to a file should not necessarily imply the ability to observe read
>> events on a file.
>>
>> Finally, watch_with_perm only applies to fanotify masks since it is the
>> only way to set a mask which allows for the blocking, permission event.
>> This permission is needed for any watch which is of this type. Though
>> fanotify requires CAP_SYS_ADMIN, this is insufficient as it gives implicit
>> trust to root, which we do not do, and does not support least privilege.
>>
>> Signed-off-by: Aaron Goidel <acgoide@tycho.nsa.gov>
>> Acked-by: Casey Schaufler <casey@schaufler-ca.com>
>> ---
>> v2:
>> - move initialization of obj_type up to remove duplicate work
>> - convert inotify and fanotify flags to common FS_* flags
>> fs/notify/dnotify/dnotify.c | 15 +++++++--
>> fs/notify/fanotify/fanotify_user.c | 19 ++++++++++--
>> fs/notify/inotify/inotify_user.c | 14 +++++++--
>> include/linux/lsm_hooks.h | 9 +++++-
>> include/linux/security.h | 10 ++++--
>> security/security.c | 6 ++++
>> security/selinux/hooks.c | 47 +++++++++++++++++++++++++++++
>> security/selinux/include/classmap.h | 5 +--
>> 8 files changed, 113 insertions(+), 12 deletions(-)
>
> ...
>
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index f77b314d0575..a47376d1c924 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -3261,6 +3263,50 @@ static int selinux_inode_removexattr(struct dentry *dentry, const char *name)
>> return -EACCES;
>> }
>>
>> +static int selinux_path_notify(const struct path *path, u64 mask,
>> + unsigned int obj_type)
>> +{
>> + int ret;
>> + u32 perm;
>> +
>> + struct common_audit_data ad;
>> +
>> + ad.type = LSM_AUDIT_DATA_PATH;
>> + ad.u.path = *path;
>> +
>> + /*
>> + * Set permission needed based on the type of mark being set.
>> + * Performs an additional check for sb watches.
>> + */
>> + switch (obj_type) {
>> + case FSNOTIFY_OBJ_TYPE_VFSMOUNT:
>> + perm = FILE__WATCH_MOUNT;
>> + break;
>> + case FSNOTIFY_OBJ_TYPE_SB:
>> + perm = FILE__WATCH_SB;
>> + ret = superblock_has_perm(current_cred(), path->dentry->d_sb,
>> + FILESYSTEM__WATCH, &ad);
>> + if (ret)
>> + return ret;
>> + break;
>> + case FSNOTIFY_OBJ_TYPE_INODE:
>> + perm = FILE__WATCH;
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>
> Sigh.
>
> Remember when I said "Don't respin the patch just for this, but if you
> have to do it for some other reason please fix the C++ style
> comments."? In this particular case it is a small thing, but a
> failure to incorporate all the feedback is one of the things that
> really annoys me (mostly because it makes me worry about other things
> that may have been missed). It isn't as bad as submitting code which
> doesn't compile, but it's a close second.
>
> At this point I'm going to ask you to respin the patch to get rid of
> those C++ style comments. I'm also going to get a bit more nitpicky
> about those comments too (more comments below).
>
>> + // check if the mask is requesting ability to set a blocking watch
>> + if (mask & (ALL_FSNOTIFY_PERM_EVENTS))
>> + perm |= FILE__WATCH_WITH_PERM; // if so, check that permission
>
> What is the point of that trailing comment "if so, check that
> permission"? Given the code, and the comment two lines above this
> seems obvious, does it not? If you want to keep it, that's fine with
> me, but let's combine the two comments so they read a bit better, for
> example:
>
> /* blocking watches require the file:watch_with_perm permission */
> if (...)
> perm |= FILE__WATCH_WITH_PERM;
>
>> + // is the mask asking to watch file reads?
>> + if (mask & (FS_ACCESS | FS_ACCESS_PERM | FS_CLOSE_NOWRITE))
>> + perm |= FILE__WATCH_READS; // check that permission as well
>
> Here the "check that permission as well" adds no additional useful
> information, it's just noise in the code, drop it from the patch.
>
> I am a believer in the old advice that good comments explain *why* the
> code is doing something, where bad comments explain *what* the code is
> doing. I would kindly ask that you keep that in mind when submitting
> future SELinux patches.
>
Paul,
I was so focused on Amir's comments that it didn't occur to me to
include the comment changes in that patch, expect a new version to
follow very shortly. My bad.
--
Aaron
^ permalink raw reply
* [PATCH v3] fanotify, inotify, dnotify, security: add security hook for fs notifications
From: Aaron Goidel @ 2019-08-12 15:20 UTC (permalink / raw)
To: paul
Cc: selinux, linux-security-module, linux-fsdevel, dhowells, jack,
amir73il, jmorris, sds, linux-kernel, Aaron Goidel,
Casey Schaufler
As of now, setting watches on filesystem objects has, at most, applied a
check for read access to the inode, and in the case of fanotify, requires
CAP_SYS_ADMIN. No specific security hook or permission check has been
provided to control the setting of watches. Using any of inotify, dnotify,
or fanotify, it is possible to observe, not only write-like operations, but
even read access to a file. Modeling the watch as being merely a read from
the file is insufficient for the needs of SELinux. This is due to the fact
that read access should not necessarily imply access to information about
when another process reads from a file. Furthermore, fanotify watches grant
more power to an application in the form of permission events. While
notification events are solely, unidirectional (i.e. they only pass
information to the receiving application), permission events are blocking.
Permission events make a request to the receiving application which will
then reply with a decision as to whether or not that action may be
completed. This causes the issue of the watching application having the
ability to exercise control over the triggering process. Without drawing a
distinction within the permission check, the ability to read would imply
the greater ability to control an application. Additionally, mount and
superblock watches apply to all files within the same mount or superblock.
Read access to one file should not necessarily imply the ability to watch
all files accessed within a given mount or superblock.
In order to solve these issues, a new LSM hook is implemented and has been
placed within the system calls for marking filesystem objects with inotify,
fanotify, and dnotify watches. These calls to the hook are placed at the
point at which the target path has been resolved and are provided with the
path struct, the mask of requested notification events, and the type of
object on which the mark is being set (inode, superblock, or mount). The
mask and obj_type have already been translated into common FS_* values
shared by the entirety of the fs notification infrastructure. The path
struct is passed rather than just the inode so that the mount is available,
particularly for mount watches. This also allows for use of the hook by
pathname-based security modules. However, since the hook is intended for
use even by inode based security modules, it is not placed under the
CONFIG_SECURITY_PATH conditional. Otherwise, the inode-based security
modules would need to enable all of the path hooks, even though they do not
use any of them.
This only provides a hook at the point of setting a watch, and presumes
that permission to set a particular watch implies the ability to receive
all notification about that object which match the mask. This is all that
is required for SELinux. If other security modules require additional hooks
or infrastructure to control delivery of notification, these can be added
by them. It does not make sense for us to propose hooks for which we have
no implementation. The understanding that all notifications received by the
requesting application are all strictly of a type for which the application
has been granted permission shows that this implementation is sufficient in
its coverage.
Security modules wishing to provide complete control over fanotify must
also implement a security_file_open hook that validates that the access
requested by the watching application is authorized. Fanotify has the issue
that it returns a file descriptor with the file mode specified during
fanotify_init() to the watching process on event. This is already covered
by the LSM security_file_open hook if the security module implements
checking of the requested file mode there. Otherwise, a watching process
can obtain escalated access to a file for which it has not been authorized.
The selinux_path_notify hook implementation works by adding five new file
permissions: watch, watch_mount, watch_sb, watch_reads, and watch_with_perm
(descriptions about which will follow), and one new filesystem permission:
watch (which is applied to superblock checks). The hook then decides which
subset of these permissions must be held by the requesting application
based on the contents of the provided mask and the obj_type. The
selinux_file_open hook already checks the requested file mode and therefore
ensures that a watching process cannot escalate its access through
fanotify.
The watch, watch_mount, and watch_sb permissions are the baseline
permissions for setting a watch on an object and each are a requirement for
any watch to be set on a file, mount, or superblock respectively. It should
be noted that having either of the other two permissions (watch_reads and
watch_with_perm) does not imply the watch, watch_mount, or watch_sb
permission. Superblock watches further require the filesystem watch
permission to the superblock. As there is no labeled object in view for
mounts, there is no specific check for mount watches beyond watch_mount to
the inode. Such a check could be added in the future, if a suitable labeled
object existed representing the mount.
The watch_reads permission is required to receive notifications from
read-exclusive events on filesystem objects. These events include accessing
a file for the purpose of reading and closing a file which has been opened
read-only. This distinction has been drawn in order to provide a direct
indication in the policy for this otherwise not obvious capability. Read
access to a file should not necessarily imply the ability to observe read
events on a file.
Finally, watch_with_perm only applies to fanotify masks since it is the
only way to set a mask which allows for the blocking, permission event.
This permission is needed for any watch which is of this type. Though
fanotify requires CAP_SYS_ADMIN, this is insufficient as it gives implicit
trust to root, which we do not do, and does not support least privilege.
Signed-off-by: Aaron Goidel <acgoide@tycho.nsa.gov>
Acked-by: Casey Schaufler <casey@schaufler-ca.com>
Acked-by: Jan Kara <jack@suse.cz>
---
v3:
- fixed comment style in security hook
v2:
- move initialization of obj_type up to remove duplicate work
- convert inotify and fanotify flags to common FS_* flags
---
fs/notify/dnotify/dnotify.c | 15 +++++++--
fs/notify/fanotify/fanotify_user.c | 19 ++++++++++--
fs/notify/inotify/inotify_user.c | 14 +++++++--
include/linux/lsm_hooks.h | 9 +++++-
include/linux/security.h | 10 ++++--
security/security.c | 6 ++++
security/selinux/hooks.c | 47 +++++++++++++++++++++++++++++
security/selinux/include/classmap.h | 5 +--
8 files changed, 113 insertions(+), 12 deletions(-)
diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
index 250369d6901d..87a7f61bc91c 100644
--- a/fs/notify/dnotify/dnotify.c
+++ b/fs/notify/dnotify/dnotify.c
@@ -22,6 +22,7 @@
#include <linux/sched/signal.h>
#include <linux/dnotify.h>
#include <linux/init.h>
+#include <linux/security.h>
#include <linux/spinlock.h>
#include <linux/slab.h>
#include <linux/fdtable.h>
@@ -288,6 +289,17 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
goto out_err;
}
+ /*
+ * convert the userspace DN_* "arg" to the internal FS_*
+ * defined in fsnotify
+ */
+ mask = convert_arg(arg);
+
+ error = security_path_notify(&filp->f_path, mask,
+ FSNOTIFY_OBJ_TYPE_INODE);
+ if (error)
+ goto out_err;
+
/* expect most fcntl to add new rather than augment old */
dn = kmem_cache_alloc(dnotify_struct_cache, GFP_KERNEL);
if (!dn) {
@@ -302,9 +314,6 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
goto out_err;
}
- /* convert the userspace DN_* "arg" to the internal FS_* defines in fsnotify */
- mask = convert_arg(arg);
-
/* set up the new_fsn_mark and new_dn_mark */
new_fsn_mark = &new_dn_mark->fsn_mark;
fsnotify_init_mark(new_fsn_mark, dnotify_group);
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index a90bb19dcfa2..8b4e2ad6d208 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -528,7 +528,8 @@ static const struct file_operations fanotify_fops = {
};
static int fanotify_find_path(int dfd, const char __user *filename,
- struct path *path, unsigned int flags)
+ struct path *path, unsigned int flags, __u64 mask,
+ unsigned int obj_type)
{
int ret;
@@ -567,8 +568,15 @@ static int fanotify_find_path(int dfd, const char __user *filename,
/* you can only watch an inode if you have read permissions on it */
ret = inode_permission(path->dentry->d_inode, MAY_READ);
+ if (ret) {
+ path_put(path);
+ goto out;
+ }
+
+ ret = security_path_notify(path, mask, obj_type);
if (ret)
path_put(path);
+
out:
return ret;
}
@@ -931,6 +939,7 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
__kernel_fsid_t __fsid, *fsid = NULL;
u32 valid_mask = FANOTIFY_EVENTS | FANOTIFY_EVENT_FLAGS;
unsigned int mark_type = flags & FANOTIFY_MARK_TYPE_BITS;
+ unsigned int obj_type;
int ret;
pr_debug("%s: fanotify_fd=%d flags=%x dfd=%d pathname=%p mask=%llx\n",
@@ -945,8 +954,13 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
switch (mark_type) {
case FAN_MARK_INODE:
+ obj_type = FSNOTIFY_OBJ_TYPE_INODE;
+ break;
case FAN_MARK_MOUNT:
+ obj_type = FSNOTIFY_OBJ_TYPE_VFSMOUNT;
+ break;
case FAN_MARK_FILESYSTEM:
+ obj_type = FSNOTIFY_OBJ_TYPE_SB;
break;
default:
return -EINVAL;
@@ -1014,7 +1028,8 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
goto fput_and_out;
}
- ret = fanotify_find_path(dfd, pathname, &path, flags);
+ ret = fanotify_find_path(dfd, pathname, &path, flags,
+ (mask & ALL_FSNOTIFY_EVENTS), obj_type);
if (ret)
goto fput_and_out;
diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index 7b53598c8804..408e9917ed42 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -39,6 +39,7 @@
#include <linux/poll.h>
#include <linux/wait.h>
#include <linux/memcontrol.h>
+#include <linux/security.h>
#include "inotify.h"
#include "../fdinfo.h"
@@ -342,7 +343,8 @@ static const struct file_operations inotify_fops = {
/*
* find_inode - resolve a user-given path to a specific inode
*/
-static int inotify_find_inode(const char __user *dirname, struct path *path, unsigned flags)
+static int inotify_find_inode(const char __user *dirname, struct path *path,
+ unsigned int flags, __u64 mask)
{
int error;
@@ -351,8 +353,15 @@ static int inotify_find_inode(const char __user *dirname, struct path *path, uns
return error;
/* you can only watch an inode if you have read permissions on it */
error = inode_permission(path->dentry->d_inode, MAY_READ);
+ if (error) {
+ path_put(path);
+ return error;
+ }
+ error = security_path_notify(path, mask,
+ FSNOTIFY_OBJ_TYPE_INODE);
if (error)
path_put(path);
+
return error;
}
@@ -744,7 +753,8 @@ SYSCALL_DEFINE3(inotify_add_watch, int, fd, const char __user *, pathname,
if (mask & IN_ONLYDIR)
flags |= LOOKUP_DIRECTORY;
- ret = inotify_find_inode(pathname, &path, flags);
+ ret = inotify_find_inode(pathname, &path, flags,
+ (mask & IN_ALL_EVENTS));
if (ret)
goto fput_and_out;
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 47f58cfb6a19..ead98af9c602 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -339,6 +339,9 @@
* Check for permission to change root directory.
* @path contains the path structure.
* Return 0 if permission is granted.
+ * @path_notify:
+ * Check permissions before setting a watch on events as defined by @mask,
+ * on an object at @path, whose type is defined by @obj_type.
* @inode_readlink:
* Check the permission to read the symbolic link.
* @dentry contains the dentry structure for the file link.
@@ -1535,7 +1538,9 @@ union security_list_options {
int (*path_chown)(const struct path *path, kuid_t uid, kgid_t gid);
int (*path_chroot)(const struct path *path);
#endif
-
+ /* Needed for inode based security check */
+ int (*path_notify)(const struct path *path, u64 mask,
+ unsigned int obj_type);
int (*inode_alloc_security)(struct inode *inode);
void (*inode_free_security)(struct inode *inode);
int (*inode_init_security)(struct inode *inode, struct inode *dir,
@@ -1860,6 +1865,8 @@ struct security_hook_heads {
struct hlist_head path_chown;
struct hlist_head path_chroot;
#endif
+ /* Needed for inode based modules as well */
+ struct hlist_head path_notify;
struct hlist_head inode_alloc_security;
struct hlist_head inode_free_security;
struct hlist_head inode_init_security;
diff --git a/include/linux/security.h b/include/linux/security.h
index 659071c2e57c..7d9c1da1f659 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -259,7 +259,8 @@ int security_dentry_create_files_as(struct dentry *dentry, int mode,
struct qstr *name,
const struct cred *old,
struct cred *new);
-
+int security_path_notify(const struct path *path, u64 mask,
+ unsigned int obj_type);
int security_inode_alloc(struct inode *inode);
void security_inode_free(struct inode *inode);
int security_inode_init_security(struct inode *inode, struct inode *dir,
@@ -387,7 +388,6 @@ int security_ismaclabel(const char *name);
int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen);
int security_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid);
void security_release_secctx(char *secdata, u32 seclen);
-
void security_inode_invalidate_secctx(struct inode *inode);
int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
@@ -621,6 +621,12 @@ static inline int security_move_mount(const struct path *from_path,
return 0;
}
+static inline int security_path_notify(const struct path *path, u64 mask,
+ unsigned int obj_type)
+{
+ return 0;
+}
+
static inline int security_inode_alloc(struct inode *inode)
{
return 0;
diff --git a/security/security.c b/security/security.c
index 613a5c00e602..30687e1366b7 100644
--- a/security/security.c
+++ b/security/security.c
@@ -871,6 +871,12 @@ int security_move_mount(const struct path *from_path, const struct path *to_path
return call_int_hook(move_mount, 0, from_path, to_path);
}
+int security_path_notify(const struct path *path, u64 mask,
+ unsigned int obj_type)
+{
+ return call_int_hook(path_notify, 0, path, mask, obj_type);
+}
+
int security_inode_alloc(struct inode *inode)
{
int rc = lsm_inode_alloc(inode);
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index f77b314d0575..d55571c585ff 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -92,6 +92,8 @@
#include <linux/kernfs.h>
#include <linux/stringhash.h> /* for hashlen_string() */
#include <uapi/linux/mount.h>
+#include <linux/fsnotify.h>
+#include <linux/fanotify.h>
#include "avc.h"
#include "objsec.h"
@@ -3261,6 +3263,50 @@ static int selinux_inode_removexattr(struct dentry *dentry, const char *name)
return -EACCES;
}
+static int selinux_path_notify(const struct path *path, u64 mask,
+ unsigned int obj_type)
+{
+ int ret;
+ u32 perm;
+
+ struct common_audit_data ad;
+
+ ad.type = LSM_AUDIT_DATA_PATH;
+ ad.u.path = *path;
+
+ /*
+ * Set permission needed based on the type of mark being set.
+ * Performs an additional check for sb watches.
+ */
+ switch (obj_type) {
+ case FSNOTIFY_OBJ_TYPE_VFSMOUNT:
+ perm = FILE__WATCH_MOUNT;
+ break;
+ case FSNOTIFY_OBJ_TYPE_SB:
+ perm = FILE__WATCH_SB;
+ ret = superblock_has_perm(current_cred(), path->dentry->d_sb,
+ FILESYSTEM__WATCH, &ad);
+ if (ret)
+ return ret;
+ break;
+ case FSNOTIFY_OBJ_TYPE_INODE:
+ perm = FILE__WATCH;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ /* blocking watches require the file:watch_with_perm permission */
+ if (mask & (ALL_FSNOTIFY_PERM_EVENTS))
+ perm |= FILE__WATCH_WITH_PERM;
+
+ /* watches on read-like events need the file:watch_reads permission */
+ if (mask & (FS_ACCESS | FS_ACCESS_PERM | FS_CLOSE_NOWRITE))
+ perm |= FILE__WATCH_READS;
+
+ return path_has_perm(current_cred(), path, perm);
+}
+
/*
* Copy the inode security context value to the user.
*
@@ -6798,6 +6844,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
LSM_HOOK_INIT(inode_getsecid, selinux_inode_getsecid),
LSM_HOOK_INIT(inode_copy_up, selinux_inode_copy_up),
LSM_HOOK_INIT(inode_copy_up_xattr, selinux_inode_copy_up_xattr),
+ LSM_HOOK_INIT(path_notify, selinux_path_notify),
LSM_HOOK_INIT(kernfs_init_security, selinux_kernfs_init_security),
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index 201f7e588a29..32e9b03be3dd 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -7,7 +7,8 @@
#define COMMON_FILE_PERMS COMMON_FILE_SOCK_PERMS, "unlink", "link", \
"rename", "execute", "quotaon", "mounton", "audit_access", \
- "open", "execmod"
+ "open", "execmod", "watch", "watch_mount", "watch_sb", \
+ "watch_with_perm", "watch_reads"
#define COMMON_SOCK_PERMS COMMON_FILE_SOCK_PERMS, "bind", "connect", \
"listen", "accept", "getopt", "setopt", "shutdown", "recvfrom", \
@@ -60,7 +61,7 @@ struct security_class_mapping secclass_map[] = {
{ "filesystem",
{ "mount", "remount", "unmount", "getattr",
"relabelfrom", "relabelto", "associate", "quotamod",
- "quotaget", NULL } },
+ "quotaget", "watch", NULL } },
{ "file",
{ COMMON_FILE_PERMS,
"execute_no_trans", "entrypoint", NULL } },
--
2.21.0
^ permalink raw reply related
* Re: [PATCH V38 00/29] security: Add support for locking down the kernel
From: Matthew Garrett @ 2019-08-12 17:06 UTC (permalink / raw)
To: James Morris; +Cc: LSM List, Linux Kernel Mailing List, Linux API
In-Reply-To: <alpine.LRH.2.21.1908101608260.25186@namei.org>
On Fri, Aug 9, 2019 at 11:08 PM James Morris <jmorris@namei.org> wrote:
> Please verify and test, as I had to make a few minor fixups for my v5.2
> base.
Thanks James - there's a few small fixups required, would you like
those as separate patches or should I just send you a fixed copy of
the original patchset?
^ 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