* 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
* [PATCH v2] tomoyo: Don't check open/getattr permission on sockets.
From: Tetsuo Handa @ 2019-08-09 15:51 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-security-module, Tetsuo Handa, syzbot
In-Reply-To: <alpine.LRH.2.21.1907061949040.2662@namei.org>
syzbot is reporting that use of SOCKET_I()->sk from open() can result in
use after free problem [1], for socket's inode is still reachable via
/proc/pid/fd/n despite destruction of SOCKET_I()->sk already completed.
But there is no point with calling security_file_open() on sockets
because open("/proc/pid/fd/n", !O_PATH) on sockets fails with -ENXIO.
There is some point with calling security_inode_getattr() on sockets
because stat("/proc/pid/fd/n") and fstat(open("/proc/pid/fd/n", O_PATH))
are valid. If we want to access "struct sock"->sk_{family,type,protocol}
fields, we will need to use security_socket_post_create() hook and
security_inode_free() hook in order to remember these fields because
security_sk_free() hook is called before the inode is destructed. But
since information which can be protected by checking
security_inode_getattr() on sockets is trivial, let's not be bothered by
"struct inode"->i_security management.
There is point with calling security_file_ioctl() on sockets. Since
ioctl(open("/proc/pid/fd/n", O_PATH)) is invalid, security_file_ioctl()
on sockets should remain safe.
[1] https://syzkaller.appspot.com/bug?id=73d590010454403d55164cca23bd0565b1eb3b74
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Reported-by: syzbot <syzbot+0341f6a4d729d4e0acf1@syzkaller.appspotmail.com>
---
security/tomoyo/tomoyo.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
index 716c92e..8ea3f5d 100644
--- a/security/tomoyo/tomoyo.c
+++ b/security/tomoyo/tomoyo.c
@@ -126,6 +126,9 @@ static int tomoyo_bprm_check_security(struct linux_binprm *bprm)
*/
static int tomoyo_inode_getattr(const struct path *path)
{
+ /* It is not safe to call tomoyo_get_socket_name(). */
+ if (S_ISSOCK(d_inode(path->dentry)->i_mode))
+ return 0;
return tomoyo_path_perm(TOMOYO_TYPE_GETATTR, path, NULL);
}
@@ -316,6 +319,9 @@ static int tomoyo_file_open(struct file *f)
/* Don't check read permission here if called from do_execve(). */
if (current->in_execve)
return 0;
+ /* Sockets can't be opened by open(). */
+ if (S_ISSOCK(file_inode(f)->i_mode))
+ return 0;
return tomoyo_check_open_permission(tomoyo_domain(), &f->f_path,
f->f_flags);
}
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH v3] KEYS: trusted: allow module init if TPM is inactive or deactivated
From: Jarkko Sakkinen @ 2019-08-09 15:50 UTC (permalink / raw)
To: Roberto Sassu
Cc: jejb, zohar, jgg, tyhicks, linux-integrity, linux-security-module,
keyrings, linux-kernel, crazyt2019+lml, nayna, silviu.vlasceanu
In-Reply-To: <20190805164427.17408-1-roberto.sassu@huawei.com>
On Mon, Aug 05, 2019 at 06:44:27PM +0200, Roberto Sassu wrote:
> Commit c78719203fc6 ("KEYS: trusted: allow trusted.ko to initialize w/o a
> TPM") allows the trusted module to be loaded even if a TPM is not found, to
> avoid module dependency problems.
>
> However, trusted module initialization can still fail if the TPM is
> inactive or deactivated. tpm_get_random() returns an error.
>
> This patch removes the call to tpm_get_random() and instead extends the PCR
> specified by the user with zeros. The security of this alternative is
> equivalent to the previous one, as either option prevents with a PCR update
> unsealing and misuse of sealed data by a user space process.
>
> Even if a PCR is extended with zeros, instead of random data, it is still
> computationally infeasible to find a value as input for a new PCR extend
> operation, to obtain again the PCR value that would allow unsealing.
>
> Fixes: 240730437deb ("KEYS: trusted: explicitly use tpm_chip structure...")
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> Reviewed-by: Tyler Hicks <tyhicks@canonical.com>
> Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
OK so this has been now applied. I'm going to do a PR over
the weekend, which means that the fix will land to 5.3.
/Jarkko
^ permalink raw reply
* Re: [Non-DoD Source] Re: [PATCH] fanotify, inotify, dnotify, security: add security hook for fs notifications
From: Aaron Goidel @ 2019-08-09 15:44 UTC (permalink / raw)
To: Paul Moore, Amir Goldstein
Cc: selinux, LSM List, linux-fsdevel, David Howells, Jan Kara,
James Morris, Stephen Smalley, linux-kernel
In-Reply-To: <CAHC9VhRpTuL2Lj1VFwHW4YLpx0hJVSxMnXefooHqsxpEUg6-0A@mail.gmail.com>
On 8/9/19 8:55 AM, Paul Moore wrote:
> On Fri, Aug 9, 2019 at 5:06 AM Amir Goldstein <amir73il@gmail.com> 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...
>
> This is one of the reasons why I was speaking to the mailing list and
> not a particular individual :)
>
>> 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(...).
>
While I'm not necessarily attached to the name, I feel as though
"misleading" is too strong a word here. 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.
--
Aaron
^ permalink raw reply
* [PATCH] LSM: Disable move_mount() syscall when TOMOYO or AppArmor is enabled.
From: Tetsuo Handa @ 2019-08-09 15:44 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-security-module, Tetsuo Handa, John Johansen
In-Reply-To: <16ae946d-dbbe-9be9-9b22-866b3cd1cd7e@i-love.sakura.ne.jp>
Commit 2db154b3ea8e14b0 ("vfs: syscall: Add move_mount(2) to move mounts
around") introduced security_move_mount() LSM hook, but we missed that
TOMOYO and AppArmor did not implement hooks for checking move_mount(2).
For pathname based access controls like TOMOYO and AppArmor, unchecked
mount manipulation is not acceptable. Therefore, until TOMOYO and AppArmor
implement hooks, in order to avoid unchecked mount manipulation, pretend
as if move_mount(2) is unavailable when either TOMOYO or AppArmor is
enabled.
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Acked-by: John Johansen <john.johansen@canonical.com>
Fixes: 2db154b3ea8e14b0 ("vfs: syscall: Add move_mount(2) to move mounts around")
Cc: stable@vger.kernel.org # 5.2
---
include/linux/lsm_hooks.h | 6 ++++++
security/apparmor/lsm.c | 1 +
security/tomoyo/tomoyo.c | 1 +
3 files changed, 8 insertions(+)
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 47f58cf..cd411b7 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -2142,4 +2142,10 @@ static inline void security_delete_hooks(struct security_hook_list *hooks,
extern int lsm_inode_alloc(struct inode *inode);
+static inline int no_move_mount(const struct path *from_path,
+ const struct path *to_path)
+{
+ return -ENOSYS;
+}
+
#endif /* ! __LINUX_LSM_HOOKS_H */
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index ec3a928..5cdf63b 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -1158,6 +1158,7 @@ struct lsm_blob_sizes apparmor_blob_sizes __lsm_ro_after_init = {
LSM_HOOK_INIT(capable, apparmor_capable),
LSM_HOOK_INIT(sb_mount, apparmor_sb_mount),
+ LSM_HOOK_INIT(move_mount, no_move_mount),
LSM_HOOK_INIT(sb_umount, apparmor_sb_umount),
LSM_HOOK_INIT(sb_pivotroot, apparmor_sb_pivotroot),
diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
index 716c92e..be1b1a1 100644
--- a/security/tomoyo/tomoyo.c
+++ b/security/tomoyo/tomoyo.c
@@ -558,6 +558,7 @@ static void tomoyo_task_free(struct task_struct *task)
LSM_HOOK_INIT(path_chown, tomoyo_path_chown),
LSM_HOOK_INIT(path_chroot, tomoyo_path_chroot),
LSM_HOOK_INIT(sb_mount, tomoyo_sb_mount),
+ LSM_HOOK_INIT(move_mount, no_move_mount),
LSM_HOOK_INIT(sb_umount, tomoyo_sb_umount),
LSM_HOOK_INIT(sb_pivotroot, tomoyo_sb_pivotroot),
LSM_HOOK_INIT(socket_bind, tomoyo_socket_bind),
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH] fanotify, inotify, dnotify, security: add security hook for fs notifications
From: Paul Moore @ 2019-08-09 12:55 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: <CAOQ4uxigYZunXgq0BubRFNM51Kh_g3wrtyNH77PozUX+3sM=aQ@mail.gmail.com>
On Fri, Aug 9, 2019 at 5:06 AM Amir Goldstein <amir73il@gmail.com> 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...
This is one of the reasons why I was speaking to the mailing list and
not a particular individual :)
> 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(...).
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH] fanotify, inotify, dnotify, security: add security hook for fs notifications
From: Amir Goldstein @ 2019-08-09 9:06 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: <CAHC9VhQUoDwBiLi+BiW=_Px18v3xMhhGYDD2mLdu9YZJDWw1yg@mail.gmail.com>
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.
> > +
> > + 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.
> > +
> > 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).
Thanks,
Amir.
^ permalink raw reply
* Re: [Tee-dev] [RFC v2 2/6] tee: enable support to register kernel memory
From: Sumit Garg @ 2019-08-09 5:36 UTC (permalink / raw)
To: Stuart Yoder
Cc: keyrings, linux-integrity, linux-security-module,
tee-dev @ lists . linaro . org, Daniel Thompson, Jonathan Corbet,
jejb, Ard Biesheuvel, Linux Doc Mailing List, Mimi Zohar,
Linux Kernel Mailing List, dhowells, Jarkko Sakkinen,
Casey Schaufler, linux-arm-kernel, Serge E. Hallyn
In-Reply-To: <99777010-db74-096a-ce1a-da30539d6fb5@arm.com>
On Fri, 9 Aug 2019 at 03:57, Stuart Yoder <stuart.yoder@arm.com> wrote:
>
>
>
> On 7/30/19 7:23 AM, Sumit Garg wrote:
>
> > @@ -264,7 +266,17 @@ struct tee_shm *tee_shm_register(struct tee_context *ctx, unsigned long addr,
> > goto err;
> > }
> >
> > - rc = get_user_pages_fast(start, num_pages, FOLL_WRITE, shm->pages);
> > + if (flags & TEE_SHM_USER_MAPPED) {
> > + rc = get_user_pages_fast(start, num_pages, FOLL_WRITE,
> > + shm->pages);
> > + } else {
> > + const struct kvec kiov = {
> > + .iov_base = (void *)start,
> > + .iov_len = PAGE_SIZE
> > + };
> > +
> > + rc = get_kernel_pages(&kiov, num_pages, 0, shm->pages);
>
> Passing a single kvec struct is temporary I assume? Because as currently
> written this will only work with num_pages==1.
>
Ah, thanks Stuart for pointing this out. It should rather be an array
of kvec struct. Will fix it in next version.
-Sumit
> Stuart
^ permalink raw reply
* Re: [PATCH v7 22/28] SELinux: Verify LSM display sanity in binder
From: Casey Schaufler @ 2019-08-09 0:56 UTC (permalink / raw)
To: Kees Cook
Cc: casey.schaufler, jmorris, linux-security-module, selinux,
john.johansen, penguin-kernel, paul, sds
In-Reply-To: <201908081454.FF7420D8D@keescook>
On 8/8/2019 2:55 PM, Kees Cook wrote:
> On Wed, Aug 07, 2019 at 12:44:04PM -0700, Casey Schaufler wrote:
>> Verify that the tasks on the ends of a binder transaction
>> use LSM display values that don't cause SELinux contexts
>> to be interpreted by another LSM or another LSM's context
>> to be interpreted by SELinux. No judgement is made in cases
>> that where SELinux contexts are not used in the binder
>> transaction.
>>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>> ---
>> security/selinux/hooks.c | 34 ++++++++++++++++++++++++++++++++++
>> 1 file changed, 34 insertions(+)
>>
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index 352be16a887d..fcad2e3432d2 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -2009,6 +2009,28 @@ static inline u32 open_file_to_av(struct file *file)
>> return av;
>> }
>>
>> +/*
>> + * Verify that if the "display" LSM is SELinux for either task
>> + * that it is for both tasks.
>> + */
>> +static inline bool compatible_task_displays(struct task_struct *here,
>> + struct task_struct *there)
>> +{
>> + int h = lsm_task_display(here);
>> + int t = lsm_task_display(there);
>> +
>> + if (h == t)
>> + return true;
>> +
>> + /* unspecified is only ok if SELinux isn't going to be involved */
>> + if (selinux_lsmid.slot == 0)
>> + return ((h == 0 && t == LSMBLOB_INVALID) ||
>> + (t == 0 && h == LSMBLOB_INVALID));
> What is "0" here? Doesn't that just mean the first LSM. I though only -1
> had a special meaning (and had a #define name for it).
I try not to write obscure code, but I seem to have done so here.
The lsm in slot 0 (the first registered "display" lsm) will
get used if the display value is LSMBLOB_INVALID. We've already
checked to see if the display values are the same, and they're not.
If selinux is in slot 0, one of the display values is 0 and the
other is LSMBLOB_INVALID, the displays are compatible. Otherwise,
they're not. If selinux is not in slot 0 and either of the displays
slots is selinux's slot, it's not compatible.
Simple, no?
I'll have a go at making the code more obvious or, failing
that, better documented.
>
> -Kees
>
>> +
>> + /* it's ok only if neither display is SELinux */
>> + return (h != selinux_lsmid.slot && t != selinux_lsmid.slot);
>> +}
>> +
>> /* Hook functions begin here. */
>>
>> static int selinux_binder_set_context_mgr(struct task_struct *mgr)
>> @@ -2016,6 +2038,9 @@ static int selinux_binder_set_context_mgr(struct task_struct *mgr)
>> u32 mysid = current_sid();
>> u32 mgrsid = task_sid(mgr);
>>
>> + if (!compatible_task_displays(current, mgr))
>> + return -EINVAL;
>> +
>> return avc_has_perm(&selinux_state,
>> mysid, mgrsid, SECCLASS_BINDER,
>> BINDER__SET_CONTEXT_MGR, NULL);
>> @@ -2029,6 +2054,9 @@ static int selinux_binder_transaction(struct task_struct *from,
>> u32 tosid = task_sid(to);
>> int rc;
>>
>> + if (!compatible_task_displays(from, to))
>> + return -EINVAL;
>> +
>> if (mysid != fromsid) {
>> rc = avc_has_perm(&selinux_state,
>> mysid, fromsid, SECCLASS_BINDER,
>> @@ -2048,6 +2076,9 @@ static int selinux_binder_transfer_binder(struct task_struct *from,
>> u32 fromsid = task_sid(from);
>> u32 tosid = task_sid(to);
>>
>> + if (!compatible_task_displays(from, to))
>> + return -EINVAL;
>> +
>> return avc_has_perm(&selinux_state,
>> fromsid, tosid, SECCLASS_BINDER, BINDER__TRANSFER,
>> NULL);
>> @@ -2064,6 +2095,9 @@ static int selinux_binder_transfer_file(struct task_struct *from,
>> struct common_audit_data ad;
>> int rc;
>>
>> + if (!compatible_task_displays(from, to))
>> + return -EINVAL;
>> +
>> ad.type = LSM_AUDIT_DATA_PATH;
>> ad.u.path = file->f_path;
>>
>> --
>> 2.20.1
>>
^ permalink raw reply
* Re: [PATCH v7 27/28] LSM: Add /proc attr entry for full LSM context
From: Casey Schaufler @ 2019-08-09 0:23 UTC (permalink / raw)
To: Kees Cook
Cc: casey.schaufler, jmorris, linux-security-module, selinux,
john.johansen, penguin-kernel, paul, sds
In-Reply-To: <201908081521.E0E7CC8F6@keescook>
On 8/8/2019 3:22 PM, Kees Cook wrote:
> On Wed, Aug 07, 2019 at 12:44:09PM -0700, Casey Schaufler wrote:
>> Add an entry /proc/.../attr/context which displays the full
>> process security "context" in compound format:'
>> lsm1\0value\0lsm2\0value\0...
>> This entry is not writable.
>>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>> ---
>> fs/proc/base.c | 1 +
>> security/security.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 46 insertions(+)
>>
>> diff --git a/fs/proc/base.c b/fs/proc/base.c
>> index 7bf70e041315..79600df5f7a2 100644
>> --- a/fs/proc/base.c
>> +++ b/fs/proc/base.c
>> @@ -2619,6 +2619,7 @@ static const struct pid_entry attr_dir_stuff[] = {
>> ATTR(NULL, "keycreate", 0666),
>> ATTR(NULL, "sockcreate", 0666),
>> ATTR(NULL, "display", 0666),
>> + ATTR(NULL, "context", 0666),
>> #ifdef CONFIG_SECURITY_SMACK
>> DIR("smack", 0555,
>> proc_smack_attr_dir_inode_ops, proc_smack_attr_dir_ops),
>> diff --git a/security/security.c b/security/security.c
>> index 0ea7ee27e331..e9f579483d12 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -2046,6 +2046,14 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
>> char **value)
>> {
>> struct security_hook_list *hp;
>> + char *final = NULL;
>> + char *cp;
>> + char *tp;
>> + int rc = 0;
>> + int finallen = 0;
>> + int llen;
>> + int clen;
>> + int tlen;
>> int display = lsm_task_display(current);
>> int slot = 0;
>>
>> @@ -2063,6 +2071,43 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
>> return -ENOMEM;
>> }
>>
>> + if (!strcmp(name, "context")) {
>> + hlist_for_each_entry(hp, &security_hook_heads.getprocattr,
>> + list) {
>> + rc = hp->hook.getprocattr(p, "current", &cp);
>> + if (rc == -EINVAL || rc == -ENOPROTOOPT)
>> + continue;
>> + if (rc < 0) {
>> + kfree(final);
>> + return rc;
>> + }
>> + llen = strlen(hp->lsmid->lsm) + 1;
>> + clen = strlen(cp) + 1;
>> + tlen = llen + clen;
>> + if (final)
>> + tlen += finallen;
>> + tp = kzalloc(tlen, GFP_KERNEL);
>> + if (tp == NULL) {
>> + kfree(cp);
>> + kfree(final);
>> + return -ENOMEM;
>> + }
>> + if (final)
>> + memcpy(tp, final, finallen);
>> + memcpy(tp + finallen, hp->lsmid->lsm, llen);
>> + memcpy(tp + finallen + llen, cp, clen);
>> + kfree(cp);
>> + if (final)
>> + kfree(final);
>> + final = tp;
>> + finallen = tlen;
>> + }
>> + if (final == NULL)
>> + return -EINVAL;
>> + *value = final;
>> + return finallen;
>> + }
> Instead of a copy-paste here, please add a helper for use in both
> places...
I added an append_ctx() helper in the "full" series. I can pull it
back to here.
>
>> +
>> hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) {
>> if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm))
>> continue;
>> --
>> 2.20.1
>>
^ permalink raw reply
* Re: [PATCH v7 26/28] NET: Add SO_PEERCONTEXT for multiple LSMs
From: Casey Schaufler @ 2019-08-09 0:18 UTC (permalink / raw)
To: Kees Cook
Cc: casey.schaufler, jmorris, linux-security-module, selinux,
john.johansen, penguin-kernel, paul, sds
In-Reply-To: <201908081500.992E5330@keescook>
On 8/8/2019 3:21 PM, Kees Cook wrote:
> On Wed, Aug 07, 2019 at 12:44:08PM -0700, Casey Schaufler wrote:
>> The getsockopt SO_PEERSEC provides the LSM based security
>> information for a single module, but for reasons of backward
>> compatibility cannot include the information for multiple
>> modules. A new option SO_PEERCONTEXT is added to report the
>> security "context" of multiple modules using a "compound" format
>>
>> lsm1\0value\0lsm2\0value\0
>>
>> This is expected to be used by system services, including dbus-daemon.
>> The exact format of a compound context has been the subject of
>> considerable debate. This format was suggested by Simon McVittie,
>> a dbus maintainer with a significant stake in the format being
>> uasable.
>>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>> ---
>> [...]
>> diff --git a/security/security.c b/security/security.c
>> index 2f4a430a1126..0ea7ee27e331 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -2117,8 +2117,8 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
>> hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) {
>> if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm))
>> continue;
>> - if (lsm == NULL && *display != LSMBLOB_INVALID &&
>> - *display != hp->lsmid->slot)
>> + if (lsm == NULL && display != NULL &&
>> + *display != LSMBLOB_INVALID && *display != hp->lsmid->slot)
>> continue;
>> return hp->hook.setprocattr(name, value, size);
>> }
>> @@ -2342,17 +2342,91 @@ int security_sock_rcv_skb(struct sock *sk, struct sk_buff *skb)
>> EXPORT_SYMBOL(security_sock_rcv_skb);
>>
>> int security_socket_getpeersec_stream(struct socket *sock, char __user *optval,
>> - int __user *optlen, unsigned len)
>> + int __user *optlen, unsigned len,
>> + int display)
>> {
>> - int display = lsm_task_display(current);
>> struct security_hook_list *hp;
>> + char *final = NULL;
>> + char *cp;
>> + char *tp;
>> + int rc = 0;
>> + unsigned finallen = 0;
>> + unsigned llen;
>> + unsigned clen = 0;
>> + unsigned tlen;
> Please move the case-specific variables into the case scope, like (and
> expand type names):
>
> case LSMBLOB_COMPOUND: {
> unsigned int clen ...;
I've never been a fan of that style. I'll probably make the
special cases into functions.
>
>> +
>> + switch (display) {
>> + case LSMBLOB_DISPLAY:
>> + rc = -ENOPROTOOPT;
>> + display = lsm_task_display(current);
>> + hlist_for_each_entry(hp,
>> + &security_hook_heads.socket_getpeersec_stream,
>> + list)
>> + if (display == LSMBLOB_INVALID ||
>> + display == hp->lsmid->slot) {
>> + rc = hp->hook.socket_getpeersec_stream(sock,
>> + &final, &finallen, len);
>> + break;
>> + }
>> + break;
>> + case LSMBLOB_COMPOUND:
>> + /*
>> + * A compound context, in the form [lsm\0value\0]...
>> + */
>> + hlist_for_each_entry(hp,
>> + &security_hook_heads.socket_getpeersec_stream,
>> + list) {
>> + rc = hp->hook.socket_getpeersec_stream(sock, &cp, &clen,
>> + len);
> Is passing "len" here useful at all? It's kind of a lie, but nothing
> else wouldn't also be a lie. :)
I could be reducing the value of len for each component gathered.
It requires yet another temporary length variable, and exceeding the
len passed has to get checked in here anyway. I may be able to make
this more sensible.
>
>> + if (rc == -EINVAL || rc == -ENOPROTOOPT) {
>> + rc = 0;
>> + continue;
>> + }
>> + if (rc) {
>> + kfree(final);
>> + return rc;
>> + }
>> + /*
>> + * Don't propogate trailing nul bytes.
> typo: propagate
Not a typo, a spelling error. I'm real good at spelling errors.
>
>> + */
>> + clen = strnlen(cp, clen) + 1;
>> + llen = strlen(hp->lsmid->lsm) + 1;
>> + tlen = llen + clen;
>> + if (final)
>> + tlen += finallen;
> You can drop the "if (final)" since finallen is initialized to 0.
Right you are.
>
>> + tp = kzalloc(tlen, GFP_KERNEL);
> I'm not a huge fan of "c", "l", and "t" prefixes -- can you just make
> these a little more self-documenting? cp and clen could be value and
> value_len. llen could be lsm_name_len. tp and tlen could be tuple and
> tuple_len. (And maybe final and finallen could be compound and
> compound_len?)
You didn't spend much time using TTY33s, did you? Or 80 column punch cards
where the last 8 are reserved for collation numbers, I'd bet. Kids. OK,
I'll see what I can do since I will be fiddling about anyway.
>
>> + if (tp == NULL) {
>> + kfree(cp);
>> + kfree(final);
>> + return -ENOMEM;
>> + }
>> + if (final)
>> + memcpy(tp, final, finallen);
>> + memcpy(tp + finallen, hp->lsmid->lsm, llen);
>> + memcpy(tp + finallen + llen, cp, clen);
>> + kfree(cp);
>> + if (final)
>> + kfree(final);
> Just kfree(final) is safe here -- kfree(NULL) is valid.
Just so. At some point there was more in the if condition.
>
>> + final = tp;
>> + finallen = tlen;
>> + }
>> + if (final == NULL)
>> + return -EINVAL;
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>>
>> - hlist_for_each_entry(hp, &security_hook_heads.socket_getpeersec_stream,
>> - list)
>> - if (display == LSMBLOB_INVALID || display == hp->lsmid->slot)
>> - return hp->hook.socket_getpeersec_stream(sock, optval,
>> - optlen, len);
>> - return -ENOPROTOOPT;
>> + if (finallen > len)
>> + rc = -ERANGE;
>> + else if (copy_to_user(optval, final, finallen))
>> + rc = -EFAULT;
>> +
>> + if (put_user(finallen, optlen))
>> + rc = -EFAULT;
>> +
>> + kfree(final);
>> + return rc;
>> }
> Otherwise, looks good.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
>
> -Kees
>
>>
>> int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb,
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index fcad2e3432d2..5e7d61754798 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -4923,10 +4923,8 @@ static int selinux_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb)
>> return err;
>> }
>>
>> -static int selinux_socket_getpeersec_stream(struct socket *sock,
>> - char __user *optval,
>> - int __user *optlen,
>> - unsigned int len)
>> +static int selinux_socket_getpeersec_stream(struct socket *sock, char **optval,
>> + int *optlen, unsigned int len)
>> {
>> int err = 0;
>> char *scontext;
>> @@ -4946,18 +4944,12 @@ static int selinux_socket_getpeersec_stream(struct socket *sock,
>> if (err)
>> return err;
>>
>> - if (scontext_len > len) {
>> + if (scontext_len > len)
>> err = -ERANGE;
>> - goto out_len;
>> - }
>> -
>> - if (copy_to_user(optval, scontext, scontext_len))
>> - err = -EFAULT;
>> + else
>> + *optval = scontext;
>>
>> -out_len:
>> - if (put_user(scontext_len, optlen))
>> - err = -EFAULT;
>> - kfree(scontext);
>> + *optlen = scontext_len;
>> return err;
>> }
>>
>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>> index 7a30b8692b1e..40c75205a914 100644
>> --- a/security/smack/smack_lsm.c
>> +++ b/security/smack/smack_lsm.c
>> @@ -3919,28 +3919,29 @@ static int smack_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb)
>> *
>> * returns zero on success, an error code otherwise
>> */
>> -static int smack_socket_getpeersec_stream(struct socket *sock,
>> - char __user *optval,
>> - int __user *optlen, unsigned len)
>> +static int smack_socket_getpeersec_stream(struct socket *sock, char **optval,
>> + int *optlen, unsigned len)
>> {
>> - struct socket_smack *ssp;
>> - char *rcp = "";
>> - int slen = 1;
>> + struct socket_smack *ssp = smack_sock(sock->sk);
>> + char *rcp;
>> + int slen;
>> int rc = 0;
>>
>> - ssp = smack_sock(sock->sk);
>> - if (ssp->smk_packet != NULL) {
>> - rcp = ssp->smk_packet->smk_known;
>> - slen = strlen(rcp) + 1;
>> + if (ssp->smk_packet == NULL) {
>> + *optlen = 0;
>> + return -EINVAL;
>> }
>>
>> + rcp = ssp->smk_packet->smk_known;
>> + slen = strlen(rcp) + 1;
>> if (slen > len)
>> rc = -ERANGE;
>> - else if (copy_to_user(optval, rcp, slen) != 0)
>> - rc = -EFAULT;
>> -
>> - if (put_user(slen, optlen) != 0)
>> - rc = -EFAULT;
>> + else {
>> + *optval = kstrdup(rcp, GFP_KERNEL);
>> + if (*optval == NULL)
>> + rc = -ENOMEM;
>> + }
>> + *optlen = slen;
>>
>> return rc;
>> }
>> --
>> 2.20.1
>>
^ permalink raw reply
* Re: [PATCH v7 15/28] LSM: Specify which LSM to display
From: Casey Schaufler @ 2019-08-08 23:38 UTC (permalink / raw)
To: Kees Cook
Cc: casey.schaufler, jmorris, linux-security-module, selinux,
john.johansen, penguin-kernel, paul, sds
In-Reply-To: <201908081424.21002A3@keescook>
On 8/8/2019 2:39 PM, Kees Cook wrote:
> On Wed, Aug 07, 2019 at 12:43:57PM -0700, Casey Schaufler wrote:
>> @@ -1980,10 +2033,48 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
>> size_t size)
>> {
>> struct security_hook_list *hp;
>> + char *term;
>> + char *cp;
>> + int *display = current->security;
> So I went down a rat hole looking at setprocattr vs current. It looks
> like everything ignores the $pid part of /proc/$pid/attr/$name and only
> ever operates on "current". Is that the expected interface here?
Yes. procfs enforces the policy that writes can only affect "self".
>
>> + int rc = -EINVAL;
>> + int slot = 0;
>> +
>> + if (!strcmp(name, "display")) {
>> + if (!capable(CAP_MAC_ADMIN))
>> + return -EPERM;
>> + /*
>> + * lsm_slot will be 0 if there are no displaying modules.
>> + */
>> + if (lsm_slot == 0 || size == 0)
>> + return -EINVAL;
> ...
>
>> + cp = kzalloc(size + 1, GFP_KERNEL);
>> + if (cp == NULL)
>> + return -ENOMEM;
>> + memcpy(cp, value, size);
> Saving one line, the above can be:
>
> cp = kmemdup_nul(value, size, GFP_KERNEL);
> if (cp == NULL)
> return -ENOMEM;
Thanks. That would be better.
>
>> + term = strchr(cp, ' ');
>> + if (term == NULL)
>> + term = strchr(cp, '\n');
> "foo\n " will result in "foo\n". I think you want strsep() instead of
> the above three lines:
>
> term = strsep(cp, " \n");
That would be better.
>
>> + if (term != NULL)
>> + *term = '\0';
>> +
>> + for (slot = 0; slot < lsm_slot; slot++)
>> + if (!strcmp(cp, lsm_slotlist[slot]->lsm)) {
>> + *display = lsm_slotlist[slot]->slot;
>> + rc = size;
>> + break;
>> + }
>> +
>> + kfree(cp);
>> + return rc;
>> + }
>>
>> hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) {
>> if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm))
>> continue;
>> + if (lsm == NULL && *display != LSMBLOB_INVALID &&
>> + *display != hp->lsmid->slot)
>> + continue;
>> return hp->hook.setprocattr(name, value, size);
>> }
>> return -EINVAL;
> Otherwise, yeah, seems good.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
>
^ permalink raw reply
* Re: [PATCH V37 04/29] Enforce module signatures if the kernel is locked down
From: James Morris @ 2019-08-08 22:43 UTC (permalink / raw)
To: Matthew Garrett
Cc: Jessica Yu, LSM List, Linux Kernel Mailing List, Linux API,
David Howells, Kees Cook
In-Reply-To: <CACdnJuu6gFQUQQOVj2MwL5+dx+XsBKY=Uq58r7F8Lr2C0Gi_TA@mail.gmail.com>
On Thu, 8 Aug 2019, Matthew Garrett wrote:
> On Thu, Aug 8, 2019 at 3:01 AM Jessica Yu <jeyu@kernel.org> wrote:
> > If you're confident that a hard dependency is not the right approach,
> > then perhaps we could add a comment in the Kconfig (You could take a
> > look at the comment under MODULE_SIG_ALL in init/Kconfig for an
> > example)? If someone is configuring the kernel on their own then it'd
> > be nice to let them know, otherwise having a lockdown kernel without
> > module signatures would defeat the purpose of lockdown no? :-)
>
> James, what would your preference be here? Jessica is right that not
> having CONFIG_MODULE_SIG enabled means lockdown probably doesn't work
> as expected, but tying it to the lockdown LSM seems inappropriate when
> another LSM could be providing lockdown policy and run into the same
> issue. Should this just be mentioned in the CONFIG_MODULE_SIG Kconfig
> help?
I agree and yes mention it in the help. A respin of just this patch is
fine.
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [Tee-dev] [RFC v2 2/6] tee: enable support to register kernel memory
From: Stuart Yoder @ 2019-08-08 22:26 UTC (permalink / raw)
To: Sumit Garg, keyrings, linux-integrity, linux-security-module
Cc: tee-dev, daniel.thompson, corbet, jejb, ard.biesheuvel, linux-doc,
zohar, linux-kernel, dhowells, jarkko.sakkinen, casey,
linux-arm-kernel, serge
In-Reply-To: <1564489420-677-3-git-send-email-sumit.garg@linaro.org>
On 7/30/19 7:23 AM, Sumit Garg wrote:
> @@ -264,7 +266,17 @@ struct tee_shm *tee_shm_register(struct tee_context *ctx, unsigned long addr,
> goto err;
> }
>
> - rc = get_user_pages_fast(start, num_pages, FOLL_WRITE, shm->pages);
> + if (flags & TEE_SHM_USER_MAPPED) {
> + rc = get_user_pages_fast(start, num_pages, FOLL_WRITE,
> + shm->pages);
> + } else {
> + const struct kvec kiov = {
> + .iov_base = (void *)start,
> + .iov_len = PAGE_SIZE
> + };
> +
> + rc = get_kernel_pages(&kiov, num_pages, 0, shm->pages);
Passing a single kvec struct is temporary I assume? Because as currently
written this will only work with num_pages==1.
Stuart
^ permalink raw reply
* Re: [PATCH v7 27/28] LSM: Add /proc attr entry for full LSM context
From: Kees Cook @ 2019-08-08 22:22 UTC (permalink / raw)
To: Casey Schaufler
Cc: casey.schaufler, jmorris, linux-security-module, selinux,
john.johansen, penguin-kernel, paul, sds
In-Reply-To: <20190807194410.9762-28-casey@schaufler-ca.com>
On Wed, Aug 07, 2019 at 12:44:09PM -0700, Casey Schaufler wrote:
> Add an entry /proc/.../attr/context which displays the full
> process security "context" in compound format:'
> lsm1\0value\0lsm2\0value\0...
> This entry is not writable.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> fs/proc/base.c | 1 +
> security/security.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 46 insertions(+)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 7bf70e041315..79600df5f7a2 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2619,6 +2619,7 @@ static const struct pid_entry attr_dir_stuff[] = {
> ATTR(NULL, "keycreate", 0666),
> ATTR(NULL, "sockcreate", 0666),
> ATTR(NULL, "display", 0666),
> + ATTR(NULL, "context", 0666),
> #ifdef CONFIG_SECURITY_SMACK
> DIR("smack", 0555,
> proc_smack_attr_dir_inode_ops, proc_smack_attr_dir_ops),
> diff --git a/security/security.c b/security/security.c
> index 0ea7ee27e331..e9f579483d12 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2046,6 +2046,14 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
> char **value)
> {
> struct security_hook_list *hp;
> + char *final = NULL;
> + char *cp;
> + char *tp;
> + int rc = 0;
> + int finallen = 0;
> + int llen;
> + int clen;
> + int tlen;
> int display = lsm_task_display(current);
> int slot = 0;
>
> @@ -2063,6 +2071,43 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
> return -ENOMEM;
> }
>
> + if (!strcmp(name, "context")) {
> + hlist_for_each_entry(hp, &security_hook_heads.getprocattr,
> + list) {
> + rc = hp->hook.getprocattr(p, "current", &cp);
> + if (rc == -EINVAL || rc == -ENOPROTOOPT)
> + continue;
> + if (rc < 0) {
> + kfree(final);
> + return rc;
> + }
> + llen = strlen(hp->lsmid->lsm) + 1;
> + clen = strlen(cp) + 1;
> + tlen = llen + clen;
> + if (final)
> + tlen += finallen;
> + tp = kzalloc(tlen, GFP_KERNEL);
> + if (tp == NULL) {
> + kfree(cp);
> + kfree(final);
> + return -ENOMEM;
> + }
> + if (final)
> + memcpy(tp, final, finallen);
> + memcpy(tp + finallen, hp->lsmid->lsm, llen);
> + memcpy(tp + finallen + llen, cp, clen);
> + kfree(cp);
> + if (final)
> + kfree(final);
> + final = tp;
> + finallen = tlen;
> + }
> + if (final == NULL)
> + return -EINVAL;
> + *value = final;
> + return finallen;
> + }
Instead of a copy-paste here, please add a helper for use in both
places...
> +
> hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) {
> if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm))
> continue;
> --
> 2.20.1
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v7 26/28] NET: Add SO_PEERCONTEXT for multiple LSMs
From: Kees Cook @ 2019-08-08 22:21 UTC (permalink / raw)
To: Casey Schaufler
Cc: casey.schaufler, jmorris, linux-security-module, selinux,
john.johansen, penguin-kernel, paul, sds
In-Reply-To: <20190807194410.9762-27-casey@schaufler-ca.com>
On Wed, Aug 07, 2019 at 12:44:08PM -0700, Casey Schaufler wrote:
> The getsockopt SO_PEERSEC provides the LSM based security
> information for a single module, but for reasons of backward
> compatibility cannot include the information for multiple
> modules. A new option SO_PEERCONTEXT is added to report the
> security "context" of multiple modules using a "compound" format
>
> lsm1\0value\0lsm2\0value\0
>
> This is expected to be used by system services, including dbus-daemon.
> The exact format of a compound context has been the subject of
> considerable debate. This format was suggested by Simon McVittie,
> a dbus maintainer with a significant stake in the format being
> uasable.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> [...]
> diff --git a/security/security.c b/security/security.c
> index 2f4a430a1126..0ea7ee27e331 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2117,8 +2117,8 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
> hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) {
> if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm))
> continue;
> - if (lsm == NULL && *display != LSMBLOB_INVALID &&
> - *display != hp->lsmid->slot)
> + if (lsm == NULL && display != NULL &&
> + *display != LSMBLOB_INVALID && *display != hp->lsmid->slot)
> continue;
> return hp->hook.setprocattr(name, value, size);
> }
> @@ -2342,17 +2342,91 @@ int security_sock_rcv_skb(struct sock *sk, struct sk_buff *skb)
> EXPORT_SYMBOL(security_sock_rcv_skb);
>
> int security_socket_getpeersec_stream(struct socket *sock, char __user *optval,
> - int __user *optlen, unsigned len)
> + int __user *optlen, unsigned len,
> + int display)
> {
> - int display = lsm_task_display(current);
> struct security_hook_list *hp;
> + char *final = NULL;
> + char *cp;
> + char *tp;
> + int rc = 0;
> + unsigned finallen = 0;
> + unsigned llen;
> + unsigned clen = 0;
> + unsigned tlen;
Please move the case-specific variables into the case scope, like (and
expand type names):
case LSMBLOB_COMPOUND: {
unsigned int clen ...;
> +
> + switch (display) {
> + case LSMBLOB_DISPLAY:
> + rc = -ENOPROTOOPT;
> + display = lsm_task_display(current);
> + hlist_for_each_entry(hp,
> + &security_hook_heads.socket_getpeersec_stream,
> + list)
> + if (display == LSMBLOB_INVALID ||
> + display == hp->lsmid->slot) {
> + rc = hp->hook.socket_getpeersec_stream(sock,
> + &final, &finallen, len);
> + break;
> + }
> + break;
> + case LSMBLOB_COMPOUND:
> + /*
> + * A compound context, in the form [lsm\0value\0]...
> + */
> + hlist_for_each_entry(hp,
> + &security_hook_heads.socket_getpeersec_stream,
> + list) {
> + rc = hp->hook.socket_getpeersec_stream(sock, &cp, &clen,
> + len);
Is passing "len" here useful at all? It's kind of a lie, but nothing
else wouldn't also be a lie. :)
> + if (rc == -EINVAL || rc == -ENOPROTOOPT) {
> + rc = 0;
> + continue;
> + }
> + if (rc) {
> + kfree(final);
> + return rc;
> + }
> + /*
> + * Don't propogate trailing nul bytes.
typo: propagate
> + */
> + clen = strnlen(cp, clen) + 1;
> + llen = strlen(hp->lsmid->lsm) + 1;
> + tlen = llen + clen;
> + if (final)
> + tlen += finallen;
You can drop the "if (final)" since finallen is initialized to 0.
> + tp = kzalloc(tlen, GFP_KERNEL);
I'm not a huge fan of "c", "l", and "t" prefixes -- can you just make
these a little more self-documenting? cp and clen could be value and
value_len. llen could be lsm_name_len. tp and tlen could be tuple and
tuple_len. (And maybe final and finallen could be compound and
compound_len?)
> + if (tp == NULL) {
> + kfree(cp);
> + kfree(final);
> + return -ENOMEM;
> + }
> + if (final)
> + memcpy(tp, final, finallen);
> + memcpy(tp + finallen, hp->lsmid->lsm, llen);
> + memcpy(tp + finallen + llen, cp, clen);
> + kfree(cp);
> + if (final)
> + kfree(final);
Just kfree(final) is safe here -- kfree(NULL) is valid.
> + final = tp;
> + finallen = tlen;
> + }
> + if (final == NULL)
> + return -EINVAL;
> + break;
> + default:
> + return -EINVAL;
> + }
>
> - hlist_for_each_entry(hp, &security_hook_heads.socket_getpeersec_stream,
> - list)
> - if (display == LSMBLOB_INVALID || display == hp->lsmid->slot)
> - return hp->hook.socket_getpeersec_stream(sock, optval,
> - optlen, len);
> - return -ENOPROTOOPT;
> + if (finallen > len)
> + rc = -ERANGE;
> + else if (copy_to_user(optval, final, finallen))
> + rc = -EFAULT;
> +
> + if (put_user(finallen, optlen))
> + rc = -EFAULT;
> +
> + kfree(final);
> + return rc;
> }
Otherwise, looks good.
Reviewed-by: Kees Cook <keescook@chromium.org>
-Kees
>
> int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb,
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index fcad2e3432d2..5e7d61754798 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -4923,10 +4923,8 @@ static int selinux_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb)
> return err;
> }
>
> -static int selinux_socket_getpeersec_stream(struct socket *sock,
> - char __user *optval,
> - int __user *optlen,
> - unsigned int len)
> +static int selinux_socket_getpeersec_stream(struct socket *sock, char **optval,
> + int *optlen, unsigned int len)
> {
> int err = 0;
> char *scontext;
> @@ -4946,18 +4944,12 @@ static int selinux_socket_getpeersec_stream(struct socket *sock,
> if (err)
> return err;
>
> - if (scontext_len > len) {
> + if (scontext_len > len)
> err = -ERANGE;
> - goto out_len;
> - }
> -
> - if (copy_to_user(optval, scontext, scontext_len))
> - err = -EFAULT;
> + else
> + *optval = scontext;
>
> -out_len:
> - if (put_user(scontext_len, optlen))
> - err = -EFAULT;
> - kfree(scontext);
> + *optlen = scontext_len;
> return err;
> }
>
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 7a30b8692b1e..40c75205a914 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -3919,28 +3919,29 @@ static int smack_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb)
> *
> * returns zero on success, an error code otherwise
> */
> -static int smack_socket_getpeersec_stream(struct socket *sock,
> - char __user *optval,
> - int __user *optlen, unsigned len)
> +static int smack_socket_getpeersec_stream(struct socket *sock, char **optval,
> + int *optlen, unsigned len)
> {
> - struct socket_smack *ssp;
> - char *rcp = "";
> - int slen = 1;
> + struct socket_smack *ssp = smack_sock(sock->sk);
> + char *rcp;
> + int slen;
> int rc = 0;
>
> - ssp = smack_sock(sock->sk);
> - if (ssp->smk_packet != NULL) {
> - rcp = ssp->smk_packet->smk_known;
> - slen = strlen(rcp) + 1;
> + if (ssp->smk_packet == NULL) {
> + *optlen = 0;
> + return -EINVAL;
> }
>
> + rcp = ssp->smk_packet->smk_known;
> + slen = strlen(rcp) + 1;
> if (slen > len)
> rc = -ERANGE;
> - else if (copy_to_user(optval, rcp, slen) != 0)
> - rc = -EFAULT;
> -
> - if (put_user(slen, optlen) != 0)
> - rc = -EFAULT;
> + else {
> + *optval = kstrdup(rcp, GFP_KERNEL);
> + if (*optval == NULL)
> + rc = -ENOMEM;
> + }
> + *optlen = slen;
>
> return rc;
> }
> --
> 2.20.1
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v7 22/28] SELinux: Verify LSM display sanity in binder
From: Kees Cook @ 2019-08-08 21:55 UTC (permalink / raw)
To: Casey Schaufler
Cc: casey.schaufler, jmorris, linux-security-module, selinux,
john.johansen, penguin-kernel, paul, sds
In-Reply-To: <20190807194410.9762-23-casey@schaufler-ca.com>
On Wed, Aug 07, 2019 at 12:44:04PM -0700, Casey Schaufler wrote:
> Verify that the tasks on the ends of a binder transaction
> use LSM display values that don't cause SELinux contexts
> to be interpreted by another LSM or another LSM's context
> to be interpreted by SELinux. No judgement is made in cases
> that where SELinux contexts are not used in the binder
> transaction.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> security/selinux/hooks.c | 34 ++++++++++++++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 352be16a887d..fcad2e3432d2 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2009,6 +2009,28 @@ static inline u32 open_file_to_av(struct file *file)
> return av;
> }
>
> +/*
> + * Verify that if the "display" LSM is SELinux for either task
> + * that it is for both tasks.
> + */
> +static inline bool compatible_task_displays(struct task_struct *here,
> + struct task_struct *there)
> +{
> + int h = lsm_task_display(here);
> + int t = lsm_task_display(there);
> +
> + if (h == t)
> + return true;
> +
> + /* unspecified is only ok if SELinux isn't going to be involved */
> + if (selinux_lsmid.slot == 0)
> + return ((h == 0 && t == LSMBLOB_INVALID) ||
> + (t == 0 && h == LSMBLOB_INVALID));
What is "0" here? Doesn't that just mean the first LSM. I though only -1
had a special meaning (and had a #define name for it).
-Kees
> +
> + /* it's ok only if neither display is SELinux */
> + return (h != selinux_lsmid.slot && t != selinux_lsmid.slot);
> +}
> +
> /* Hook functions begin here. */
>
> static int selinux_binder_set_context_mgr(struct task_struct *mgr)
> @@ -2016,6 +2038,9 @@ static int selinux_binder_set_context_mgr(struct task_struct *mgr)
> u32 mysid = current_sid();
> u32 mgrsid = task_sid(mgr);
>
> + if (!compatible_task_displays(current, mgr))
> + return -EINVAL;
> +
> return avc_has_perm(&selinux_state,
> mysid, mgrsid, SECCLASS_BINDER,
> BINDER__SET_CONTEXT_MGR, NULL);
> @@ -2029,6 +2054,9 @@ static int selinux_binder_transaction(struct task_struct *from,
> u32 tosid = task_sid(to);
> int rc;
>
> + if (!compatible_task_displays(from, to))
> + return -EINVAL;
> +
> if (mysid != fromsid) {
> rc = avc_has_perm(&selinux_state,
> mysid, fromsid, SECCLASS_BINDER,
> @@ -2048,6 +2076,9 @@ static int selinux_binder_transfer_binder(struct task_struct *from,
> u32 fromsid = task_sid(from);
> u32 tosid = task_sid(to);
>
> + if (!compatible_task_displays(from, to))
> + return -EINVAL;
> +
> return avc_has_perm(&selinux_state,
> fromsid, tosid, SECCLASS_BINDER, BINDER__TRANSFER,
> NULL);
> @@ -2064,6 +2095,9 @@ static int selinux_binder_transfer_file(struct task_struct *from,
> struct common_audit_data ad;
> int rc;
>
> + if (!compatible_task_displays(from, to))
> + return -EINVAL;
> +
> ad.type = LSM_AUDIT_DATA_PATH;
> ad.u.path = file->f_path;
>
> --
> 2.20.1
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v7 15/28] LSM: Specify which LSM to display
From: Kees Cook @ 2019-08-08 21:39 UTC (permalink / raw)
To: Casey Schaufler
Cc: casey.schaufler, jmorris, linux-security-module, selinux,
john.johansen, penguin-kernel, paul, sds
In-Reply-To: <20190807194410.9762-16-casey@schaufler-ca.com>
On Wed, Aug 07, 2019 at 12:43:57PM -0700, Casey Schaufler wrote:
> @@ -1980,10 +2033,48 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
> size_t size)
> {
> struct security_hook_list *hp;
> + char *term;
> + char *cp;
> + int *display = current->security;
So I went down a rat hole looking at setprocattr vs current. It looks
like everything ignores the $pid part of /proc/$pid/attr/$name and only
ever operates on "current". Is that the expected interface here?
> + int rc = -EINVAL;
> + int slot = 0;
> +
> + if (!strcmp(name, "display")) {
> + if (!capable(CAP_MAC_ADMIN))
> + return -EPERM;
> + /*
> + * lsm_slot will be 0 if there are no displaying modules.
> + */
> + if (lsm_slot == 0 || size == 0)
> + return -EINVAL;
...
> + cp = kzalloc(size + 1, GFP_KERNEL);
> + if (cp == NULL)
> + return -ENOMEM;
> + memcpy(cp, value, size);
Saving one line, the above can be:
cp = kmemdup_nul(value, size, GFP_KERNEL);
if (cp == NULL)
return -ENOMEM;
> + term = strchr(cp, ' ');
> + if (term == NULL)
> + term = strchr(cp, '\n');
"foo\n " will result in "foo\n". I think you want strsep() instead of
the above three lines:
term = strsep(cp, " \n");
> + if (term != NULL)
> + *term = '\0';
> +
> + for (slot = 0; slot < lsm_slot; slot++)
> + if (!strcmp(cp, lsm_slotlist[slot]->lsm)) {
> + *display = lsm_slotlist[slot]->slot;
> + rc = size;
> + break;
> + }
> +
> + kfree(cp);
> + return rc;
> + }
>
> hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) {
> if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm))
> continue;
> + if (lsm == NULL && *display != LSMBLOB_INVALID &&
> + *display != hp->lsmid->slot)
> + continue;
> return hp->hook.setprocattr(name, value, size);
> }
> return -EINVAL;
Otherwise, yeah, seems good.
Reviewed-by: Kees Cook <keescook@chromium.org>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH] fanotify, inotify, dnotify, security: add security hook for fs notifications
From: Paul Moore @ 2019-08-08 18:33 UTC (permalink / raw)
To: Aaron Goidel, selinux, linux-security-module, linux-fsdevel
Cc: dhowells, jack, amir73il, James Morris, Stephen Smalley,
linux-kernel
In-Reply-To: <20190731153443.4984-1-acgoide@tycho.nsa.gov>
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.
> 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;
> + }
> +
> + ret = security_path_notify(path, mask, obj_type);
> if (ret)
> path_put(path);
> +
> 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);
> 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 c61787b15f27..a1aaf79421df 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.
> *
> @@ -6797,6 +6843,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
>
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH V37 04/29] Enforce module signatures if the kernel is locked down
From: Matthew Garrett @ 2019-08-08 18:31 UTC (permalink / raw)
To: Jessica Yu
Cc: James Morris, LSM List, Linux Kernel Mailing List, Linux API,
David Howells, Kees Cook
In-Reply-To: <20190808100059.GA30260@linux-8ccs>
On Thu, Aug 8, 2019 at 3:01 AM Jessica Yu <jeyu@kernel.org> wrote:
> If you're confident that a hard dependency is not the right approach,
> then perhaps we could add a comment in the Kconfig (You could take a
> look at the comment under MODULE_SIG_ALL in init/Kconfig for an
> example)? If someone is configuring the kernel on their own then it'd
> be nice to let them know, otherwise having a lockdown kernel without
> module signatures would defeat the purpose of lockdown no? :-)
James, what would your preference be here? Jessica is right that not
having CONFIG_MODULE_SIG enabled means lockdown probably doesn't work
as expected, but tying it to the lockdown LSM seems inappropriate when
another LSM could be providing lockdown policy and run into the same
issue. Should this just be mentioned in the CONFIG_MODULE_SIG Kconfig
help?
^ permalink raw reply
* KASAN: use-after-free Read in tomoyo_socket_sendmsg_permission
From: syzbot @ 2019-08-08 16:45 UTC (permalink / raw)
To: jmorris, linux-kernel, linux-security-module, netdev, serge,
syzkaller-bugs, takedakn
Hello,
syzbot found the following crash on:
HEAD commit: 107e47cc vrf: make sure skb->data contains ip header to ma..
git tree: net
console output: https://syzkaller.appspot.com/x/log.txt?x=139506d8600000
kernel config: https://syzkaller.appspot.com/x/.config?x=4dba67bf8b8c9ad7
dashboard link: https://syzkaller.appspot.com/bug?extid=b91501546ab4037f685f
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+b91501546ab4037f685f@syzkaller.appspotmail.com
==================================================================
BUG: KASAN: use-after-free in tomoyo_sock_family
security/tomoyo/network.c:632 [inline]
BUG: KASAN: use-after-free in tomoyo_socket_sendmsg_permission+0x37e/0x3cb
security/tomoyo/network.c:762
Read of size 2 at addr ffff8880a066f410 by task syz-executor.3/2063
CPU: 1 PID: 2063 Comm: syz-executor.3 Not tainted 5.2.0+ #97
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x172/0x1f0 lib/dump_stack.c:113
print_address_description.cold+0xd4/0x306 mm/kasan/report.c:351
__kasan_report.cold+0x1b/0x36 mm/kasan/report.c:482
kasan_report+0x12/0x17 mm/kasan/common.c:612
__asan_report_load2_noabort+0x14/0x20 mm/kasan/generic_report.c:130
tomoyo_sock_family security/tomoyo/network.c:632 [inline]
tomoyo_socket_sendmsg_permission+0x37e/0x3cb security/tomoyo/network.c:762
tomoyo_socket_sendmsg+0x26/0x30 security/tomoyo/tomoyo.c:486
security_socket_sendmsg+0x77/0xc0 security/security.c:1973
sock_sendmsg+0x45/0x130 net/socket.c:654
__sys_sendto+0x262/0x380 net/socket.c:1952
__do_sys_sendto net/socket.c:1964 [inline]
__se_sys_sendto net/socket.c:1960 [inline]
__x64_sys_sendto+0xe1/0x1a0 net/socket.c:1960
do_syscall_64+0xfd/0x6a0 arch/x86/entry/common.c:296
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x459829
Code: fd b7 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7
48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
ff 0f 83 cb b7 fb ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007f8413a51c78 EFLAGS: 00000246 ORIG_RAX: 000000000000002c
RAX: ffffffffffffffda RBX: 0000000000000006 RCX: 0000000000459829
RDX: 00000000000000ed RSI: 0000000020000280 RDI: 0000000000000005
RBP: 000000000075bfc8 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00007f8413a526d4
R13: 00000000004c77f1 R14: 00000000004dcfc0 R15: 00000000ffffffff
Allocated by task 0:
save_stack+0x23/0x90 mm/kasan/common.c:69
set_track mm/kasan/common.c:77 [inline]
__kasan_kmalloc mm/kasan/common.c:487 [inline]
__kasan_kmalloc.constprop.0+0xcf/0xe0 mm/kasan/common.c:460
kasan_kmalloc+0x9/0x10 mm/kasan/common.c:501
__do_kmalloc mm/slab.c:3655 [inline]
__kmalloc+0x163/0x770 mm/slab.c:3664
kmalloc include/linux/slab.h:557 [inline]
sk_prot_alloc+0x23a/0x310 net/core/sock.c:1603
sk_alloc+0x39/0xf70 net/core/sock.c:1657
nr_make_new net/netrom/af_netrom.c:476 [inline]
nr_rx_frame+0x733/0x1e73 net/netrom/af_netrom.c:959
nr_loopback_timer+0x7b/0x170 net/netrom/nr_loopback.c:59
call_timer_fn+0x1ac/0x780 kernel/time/timer.c:1322
expire_timers kernel/time/timer.c:1366 [inline]
__run_timers kernel/time/timer.c:1685 [inline]
__run_timers kernel/time/timer.c:1653 [inline]
run_timer_softirq+0x697/0x17a0 kernel/time/timer.c:1698
__do_softirq+0x262/0x98c kernel/softirq.c:292
Freed by task 2063:
save_stack+0x23/0x90 mm/kasan/common.c:69
set_track mm/kasan/common.c:77 [inline]
__kasan_slab_free+0x102/0x150 mm/kasan/common.c:449
kasan_slab_free+0xe/0x10 mm/kasan/common.c:457
__cache_free mm/slab.c:3425 [inline]
kfree+0x10a/0x2c0 mm/slab.c:3756
sk_prot_free net/core/sock.c:1640 [inline]
__sk_destruct+0x4f7/0x6e0 net/core/sock.c:1726
sk_destruct+0x86/0xa0 net/core/sock.c:1734
__sk_free+0xfb/0x360 net/core/sock.c:1745
sk_free+0x42/0x50 net/core/sock.c:1756
sock_put include/net/sock.h:1725 [inline]
sock_efree+0x61/0x80 net/core/sock.c:2042
skb_release_head_state+0xeb/0x250 net/core/skbuff.c:652
skb_release_all+0x16/0x60 net/core/skbuff.c:663
__kfree_skb net/core/skbuff.c:679 [inline]
kfree_skb net/core/skbuff.c:697 [inline]
kfree_skb+0x101/0x3c0 net/core/skbuff.c:691
nr_accept+0x56e/0x700 net/netrom/af_netrom.c:819
__sys_accept4+0x34e/0x6a0 net/socket.c:1754
__do_sys_accept net/socket.c:1795 [inline]
__se_sys_accept net/socket.c:1792 [inline]
__x64_sys_accept+0x75/0xb0 net/socket.c:1792
do_syscall_64+0xfd/0x6a0 arch/x86/entry/common.c:296
entry_SYSCALL_64_after_hwframe+0x49/0xbe
The buggy address belongs to the object at ffff8880a066f400
which belongs to the cache kmalloc-2k of size 2048
The buggy address is located 16 bytes inside of
2048-byte region [ffff8880a066f400, ffff8880a066fc00)
The buggy address belongs to the page:
page:ffffea0002819b80 refcount:1 mapcount:0 mapping:ffff8880aa400e00
index:0x0 compound_mapcount: 0
flags: 0x1fffc0000010200(slab|head)
raw: 01fffc0000010200 ffffea0001849388 ffffea000235a788 ffff8880aa400e00
raw: 0000000000000000 ffff8880a066e300 0000000100000003 0000000000000000
page dumped because: kasan: bad access detected
Memory state around the buggy address:
ffff8880a066f300: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
ffff8880a066f380: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> ffff8880a066f400: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
^
ffff8880a066f480: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
ffff8880a066f500: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
==================================================================
------------[ cut here ]------------
refcount_t: addition on 0; use-after-free.
WARNING: CPU: 1 PID: 2063 at lib/refcount.c:105 refcount_add_checked
lib/refcount.c:105 [inline]
WARNING: CPU: 1 PID: 2063 at lib/refcount.c:105
refcount_add_checked+0x6b/0x70 lib/refcount.c:103
Modules linked in:
CPU: 1 PID: 2063 Comm: syz-executor.3 Tainted: G B 5.2.0+ #97
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
RIP: 0010:refcount_add_checked lib/refcount.c:105 [inline]
RIP: 0010:refcount_add_checked+0x6b/0x70 lib/refcount.c:103
Code: 1d e7 77 64 06 31 ff 89 de e8 71 dc 35 fe 84 db 75 db e8 28 db 35 fe
48 c7 c7 20 08 c6 87 c6 05 c7 77 64 06 01 e8 1d 43 07 fe <0f> 0b eb bf 90
48 b8 00 00 00 00 00 fc ff df 55 48 89 e5 41 54 49
RSP: 0018:ffff88805d55f9f8 EFLAGS: 00010286
RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
RDX: 0000000000040000 RSI: ffffffff815c3a26 RDI: ffffed100baabf31
RBP: ffff88805d55fa10 R08: ffff888098124480 R09: fffffbfff1349ef0
R10: fffffbfff1349eef R11: ffffffff89a4f77f R12: 0000000000000500
R13: ffff8880a066f644 R14: ffff88808f271e60 R15: 00000000000000ed
FS: 00007f8413a52700(0000) GS:ffff8880ae900000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000000000075c000 CR3: 0000000097173000 CR4: 00000000001406e0
Call Trace:
skb_set_owner_w+0x216/0x320 net/core/sock.c:1991
sock_alloc_send_pskb+0x7c9/0x920 net/core/sock.c:2226
sock_alloc_send_skb+0x32/0x40 net/core/sock.c:2240
nr_sendmsg+0x557/0xb00 net/netrom/af_netrom.c:1094
sock_sendmsg_nosec net/socket.c:637 [inline]
sock_sendmsg+0xd7/0x130 net/socket.c:657
__sys_sendto+0x262/0x380 net/socket.c:1952
__do_sys_sendto net/socket.c:1964 [inline]
__se_sys_sendto net/socket.c:1960 [inline]
__x64_sys_sendto+0xe1/0x1a0 net/socket.c:1960
do_syscall_64+0xfd/0x6a0 arch/x86/entry/common.c:296
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x459829
Code: fd b7 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7
48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
ff 0f 83 cb b7 fb ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007f8413a51c78 EFLAGS: 00000246 ORIG_RAX: 000000000000002c
RAX: ffffffffffffffda RBX: 0000000000000006 RCX: 0000000000459829
RDX: 00000000000000ed RSI: 0000000020000280 RDI: 0000000000000005
RBP: 000000000075bfc8 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00007f8413a526d4
R13: 00000000004c77f1 R14: 00000000004dcfc0 R15: 00000000ffffffff
irq event stamp: 344
hardirqs last enabled at (343): [<ffffffff8100a206>]
do_syscall_64+0x26/0x6a0 arch/x86/entry/common.c:283
hardirqs last disabled at (344): [<ffffffff87391ddf>]
__raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:108 [inline]
hardirqs last disabled at (344): [<ffffffff87391ddf>]
_raw_spin_lock_irqsave+0x6f/0xcd kernel/locking/spinlock.c:159
softirqs last enabled at (314): [<ffffffff858fb1b6>] spin_unlock_bh
include/linux/spinlock.h:383 [inline]
softirqs last enabled at (314): [<ffffffff858fb1b6>]
release_sock+0x156/0x1c0 net/core/sock.c:2945
softirqs last disabled at (312): [<ffffffff858fb080>] spin_lock_bh
include/linux/spinlock.h:343 [inline]
softirqs last disabled at (312): [<ffffffff858fb080>]
release_sock+0x20/0x1c0 net/core/sock.c:2932
---[ end trace 2fe47c842e5d598a ]---
---
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 V38 19/29] Lock down module params that specify hardware parameters (eg. ioport)
From: James Morris @ 2019-08-08 16:33 UTC (permalink / raw)
To: Jessica Yu
Cc: Matthew Garrett, linux-security-module, linux-kernel, linux-api,
David Howells, Alan Cox, Matthew Garrett, Kees Cook
In-Reply-To: <20190808111246.GA29211@linux-8ccs>
On Thu, 8 Aug 2019, Jessica Yu wrote:
> > +#ifdef CONFIG_MODULES
> > +#define mod_name(mod) ((mod)->name)
> > +#else
> > +#define mod_name(mod) "unknown"
> > +#endif
> > +
>
> Hm, I don't think mod_name is used anywhere?
>
> But other than that:
>
> Acked-by: Jessica Yu <jeyu@kernel.org>
>
Matthew: no need to respin the patchset just for this.
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [RFC PATCH v2] security,capability: pass object information to security_capable
From: Paul Moore @ 2019-08-08 16:30 UTC (permalink / raw)
To: Aaron Goidel
Cc: selinux, linux-security-module, luto, James Morris,
Stephen Smalley, keescook, Serge Hallyn, john.johansen, casey,
mortonm, rgb, Nicholas Franck, linux-audit
In-Reply-To: <20190801144313.1014-1-acgoide@tycho.nsa.gov>
On Thu, Aug 1, 2019 at 10:43 AM Aaron Goidel <acgoide@tycho.nsa.gov> wrote:
> From: Nicholas Franck <nhfran2@tycho.nsa.gov>
>
> At present security_capable does not pass any object information
> and therefore can neither audit the particular object nor take it
> into account. Augment the security_capable interface to support
> passing supplementary data. Use this facility initially to convey
> the inode for capability checks relevant to inodes. This only
> addresses capable_wrt_inode_uidgid calls; other capability checks
> relevant to inodes will be addressed in subsequent changes. In the
> future, this will be further extended to pass object information for
> other capability checks such as the target task for CAP_KILL.
>
> In SELinux this new information is leveraged here to include the inode
> in the audit message. In the future, it could also be used to perform
> a per inode capability checks.
>
> It would be possible to fold the existing opts argument into this new
> supplementary data structure. This was omitted from this change to
> minimize changes.
>
> Signed-off-by: Nicholas Franck <nhfran2@tycho.nsa.gov>
> Signed-off-by: Aaron Goidel <acgoide@tycho.nsa.gov>
> ---
> v2:
> - Changed order of audit prints so optional information comes second
> ---
> include/linux/capability.h | 7 ++++++
> include/linux/lsm_audit.h | 5 +++-
> include/linux/lsm_hooks.h | 3 ++-
> include/linux/security.h | 23 +++++++++++++-----
> kernel/capability.c | 33 ++++++++++++++++++--------
> kernel/seccomp.c | 2 +-
> security/apparmor/capability.c | 8 ++++---
> security/apparmor/include/capability.h | 4 +++-
> security/apparmor/ipc.c | 2 +-
> security/apparmor/lsm.c | 5 ++--
> security/apparmor/resource.c | 2 +-
> security/commoncap.c | 11 +++++----
> security/lsm_audit.c | 21 ++++++++++++++--
> security/safesetid/lsm.c | 3 ++-
> security/security.c | 5 ++--
> security/selinux/hooks.c | 20 +++++++++-------
> security/smack/smack_access.c | 2 +-
> 17 files changed, 110 insertions(+), 46 deletions(-)
You should CC the linux-audit list, I've added them on this mail.
I had hoped to see some thought put into the idea of dynamically
emitting the proper audit records as I mentioned in the previous patch
set, but regardless there are some comments on this code as written
...
> diff --git a/security/lsm_audit.c b/security/lsm_audit.c
> index 33028c098ef3..18cc7c956b69 100644
> --- a/security/lsm_audit.c
> +++ b/security/lsm_audit.c
> @@ -229,9 +229,26 @@ static void dump_common_audit_data(struct audit_buffer *ab,
> case LSM_AUDIT_DATA_IPC:
> audit_log_format(ab, " key=%d ", a->u.ipc_id);
> break;
> - case LSM_AUDIT_DATA_CAP:
> - audit_log_format(ab, " capability=%d ", a->u.cap);
> + case LSM_AUDIT_DATA_CAP: {
> + const struct inode *inode;
> +
> + audit_log_format(ab, " capability=%d ", a->u.cap_struct.cap);
> + if (a->u.cap_struct.cad) {
> + switch (a->u.cap_struct.cad->type) {
> + case CAP_AUX_DATA_INODE: {
> + inode = a->u.cap_struct.cad->u.inode;
> +
> + audit_log_format(ab, " dev=");
> + audit_log_untrustedstring(ab,
> + inode->i_sb->s_id);
> + audit_log_format(ab, " ino=%lu",
> + inode->i_ino);
> + break;
> + }
Since you are declaring "inode" further up, there doesn't appear to be
any need for the CAP_AUX_DATA_INODE braces, please remove them.
The general recommended practice when it comes to "sometimes" fields
in an audit record, is to always record them in the record, but use a
value of "?" when there is nothing relevant to record. For example,
when *not* recording inode information you would do something like the
following:
audit_log_format(ab, " dev=? ino=?");
> + }
> + }
> break;
> + }
> case LSM_AUDIT_DATA_PATH: {
> struct inode *inode;
>
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH v13 2/5] Add flags option to get xattr method paired to __vfs_getxattr
From: Mark Salyzyn @ 2019-08-08 15:29 UTC (permalink / raw)
To: Stephen Smalley, Linux Security Module list, linux-integrity
In-Reply-To: <20190731165803.4755-3-salyzyn@android.com>
I have been told privately that this patch should be pulled out of the
overlayfs series, and independently approved. In effect by security
focus (the filesystem folks generally ignore patches that are related to
overlayfs or are security related).
Once accepted, we can _then_ get on with the fixes for overlayfs (or any
other unionfs that needs this) under separate cover.
Does the flags option to __vfs_getxattr deal with your (Stephen Smalley)
concerns about attack surface (the flag is passed
programmatically/internally and resides only on the randomized stack)?
Sincerely -- Mark
On 7/31/19 9:57 AM, Mark Salyzyn wrote:
> Add a flag option to get xattr method that could have a bit flag of
> XATTR_NOSECURITY passed to it. XATTR_NOSECURITY is generally then
> set in the __vfs_getxattr path.
>
> This handles the case of a union filesystem driver that is being
> requested by the security layer to report back the data that is the
> target label or context embedded into wrapped filesystem's xattr.
>
> For the use case where access is to be blocked by the security layer.
>
> The path then could be security(dentry) ->
> __vfs_getxattr(dentry...XATTR_NOSECUIRTY) ->
> handler->get(dentry...XATTR_NOSECURITY) ->
> __vfs_getxattr(lower_dentry...XATTR_NOSECUIRTY) ->
> lower_handler->get(lower_dentry...XATTR_NOSECUIRTY)
> which would report back through the chain data and success as
> expected, but the logging security layer at the top would have the
> data to determine the access permissions and report back the target
> context that was blocked.
>
> Without the get handler flag, the path on a union filesystem would be
> the errant security(dentry) -> __vfs_getxattr(dentry) ->
> handler->get(dentry) -> vfs_getxattr(lower_dentry) -> nested ->
> security(lower_dentry, log off) -> lower_handler->get(lower_dentry)
> which would report back through the chain no data, and -EACCES.
>
> For selinux for both cases, this would translate to a correctly
> determined blocked access. In the first corrected case a correct avc
> log would be reported, in the second legacy case an incorrect avc log
> would be reported against an uninitialized u:object_r:unlabeled:s0
> context making the logs cosmetically useless for audit2allow.
>
> This patch series is inert and is the wide-spread addition of the
> flags option for xattr functions, and a replacement of _vfs_getxattr
> with __vfs_getxattr(...XATTR_NOSECURITY).
>
> Signed-off-by: Mark Salyzyn <salyzyn@android.com>
> Cc: Miklos Szeredi <miklos@szeredi.hu>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Vivek Goyal <vgoyal@redhat.com>
> Cc: Eric W. Biederman <ebiederm@xmission.com>
> Cc: Amir Goldstein <amir73il@gmail.com>
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Cc: Stephen Smalley <sds@tycho.nsa.gov>
> Cc: linux-unionfs@vger.kernel.org
> Cc: linux-doc@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: kernel-team@android.com
> Cc: Eric Van Hensbergen <ericvh@gmail.com>
> Cc: Latchesar Ionkov <lucho@ionkov.net>
> Cc: Dominique Martinet <asmadeus@codewreck.org>
> Cc: David Howells <dhowells@redhat.com>
> Cc: Chris Mason <clm@fb.com>
> Cc: Josef Bacik <josef@toxicpanda.com>
> Cc: David Sterba <dsterba@suse.com>
> Cc: Jeff Layton <jlayton@kernel.org>
> Cc: Sage Weil <sage@redhat.com>
> Cc: Ilya Dryomov <idryomov@gmail.com>
> Cc: Steve French <sfrench@samba.org>
> Cc: Tyler Hicks <tyhicks@canonical.com>
> Cc: Jan Kara <jack@suse.com>
> Cc: Theodore Ts'o <tytso@mit.edu>
> Cc: Andreas Dilger <adilger.kernel@dilger.ca>
> Cc: Jaegeuk Kim <jaegeuk@kernel.org>
> Cc: Chao Yu <yuchao0@huawei.com>
> Cc: Bob Peterson <rpeterso@redhat.com>
> Cc: Andreas Gruenbacher <agruenba@redhat.com>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Richard Weinberger <richard@nod.at>
> Cc: Dave Kleikamp <shaggy@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Trond Myklebust <trond.myklebust@hammerspace.com>
> Cc: Anna Schumaker <anna.schumaker@netapp.com>
> Cc: Mark Fasheh <mark@fasheh.com>
> Cc: Joel Becker <jlbec@evilplan.org>
> Cc: Joseph Qi <joseph.qi@linux.alibaba.com>
> Cc: Mike Marshall <hubcap@omnibond.com>
> Cc: Martin Brandenburg <martin@omnibond.com>
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: Phillip Lougher <phillip@squashfs.org.uk>
> Cc: Darrick J. Wong <darrick.wong@oracle.com>
> Cc: linux-xfs@vger.kernel.org
> Cc: Hugh Dickins <hughd@google.com>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Mathieu Malaterre <malat@debian.org>
> Cc: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com>
> Cc: Vyacheslav Dubeyko <slava@dubeyko.com>
> Cc: v9fs-developer@lists.sourceforge.net
> Cc: linux-afs@lists.infradead.org
> Cc: linux-btrfs@vger.kernel.org
> Cc: ceph-devel@vger.kernel.org
> Cc: linux-cifs@vger.kernel.org
> Cc: samba-technical@lists.samba.org
> Cc: ecryptfs@vger.kernel.org
> Cc: linux-ext4@vger.kernel.org
> Cc: linux-f2fs-devel@lists.sourceforge.net
> Cc: linux-fsdevel@vger.kernel.org
> Cc: cluster-devel@redhat.com
> Cc: linux-mtd@lists.infradead.org
> Cc: jfs-discussion@lists.sourceforge.net
> Cc: linux-nfs@vger.kernel.org
> Cc: ocfs2-devel@oss.oracle.com
> Cc: devel@lists.orangefs.org
> Cc: reiserfs-devel@vger.kernel.org
> Cc: linux-mm@kvack.org
> Cc: netdev@vger.kernel.org
> Cc: linux-security-module@vger.kernel.org
> Cc: stable@vger.kernel.org # 4.4, 4.9, 4.14 & 4.19
> ---
> v13 - added flags to __vfs_getxattr call, and moved all the security
> code from vfs_getxattr into it.
>
> v12 - Added back to patch series as get xattr with flag option.
>
> v11 - Squashed out of patch series and replaced with per-thread flag
> solution.
>
> v10 - Added to patch series as __get xattr method.
> ---
> fs/9p/acl.c | 3 ++-
> fs/9p/xattr.c | 3 ++-
> fs/afs/xattr.c | 6 +++---
> fs/btrfs/xattr.c | 3 ++-
> fs/ceph/xattr.c | 3 ++-
> fs/cifs/xattr.c | 2 +-
> fs/ecryptfs/inode.c | 6 ++++--
> fs/ecryptfs/mmap.c | 2 +-
> fs/ext2/xattr_trusted.c | 2 +-
> fs/ext2/xattr_user.c | 2 +-
> fs/ext4/xattr_security.c | 2 +-
> fs/ext4/xattr_trusted.c | 2 +-
> fs/ext4/xattr_user.c | 2 +-
> fs/f2fs/xattr.c | 4 ++--
> fs/fuse/xattr.c | 4 ++--
> fs/gfs2/xattr.c | 3 ++-
> fs/hfs/attr.c | 2 +-
> fs/hfsplus/xattr.c | 3 ++-
> fs/hfsplus/xattr_trusted.c | 3 ++-
> fs/hfsplus/xattr_user.c | 3 ++-
> fs/jffs2/security.c | 3 ++-
> fs/jffs2/xattr_trusted.c | 3 ++-
> fs/jffs2/xattr_user.c | 3 ++-
> fs/jfs/xattr.c | 5 +++--
> fs/kernfs/inode.c | 3 ++-
> fs/nfs/nfs4proc.c | 6 ++++--
> fs/ocfs2/xattr.c | 9 +++++---
> fs/orangefs/xattr.c | 3 ++-
> fs/overlayfs/super.c | 8 ++++---
> fs/posix_acl.c | 2 +-
> fs/reiserfs/xattr_security.c | 3 ++-
> fs/reiserfs/xattr_trusted.c | 3 ++-
> fs/reiserfs/xattr_user.c | 3 ++-
> fs/squashfs/xattr.c | 2 +-
> fs/xattr.c | 36 +++++++++++++++----------------
> fs/xfs/xfs_xattr.c | 3 ++-
> include/linux/xattr.h | 9 ++++----
> include/uapi/linux/xattr.h | 5 +++--
> mm/shmem.c | 3 ++-
> net/socket.c | 3 ++-
> security/commoncap.c | 6 ++++--
> security/integrity/evm/evm_main.c | 3 ++-
> security/selinux/hooks.c | 11 ++++++----
> security/smack/smack_lsm.c | 5 +++--
> 44 files changed, 119 insertions(+), 81 deletions(-)
>
> diff --git a/fs/9p/acl.c b/fs/9p/acl.c
> index 6261719f6f2a..cb14e8b312bc 100644
> --- a/fs/9p/acl.c
> +++ b/fs/9p/acl.c
> @@ -214,7 +214,8 @@ int v9fs_acl_mode(struct inode *dir, umode_t *modep,
>
> static int v9fs_xattr_get_acl(const struct xattr_handler *handler,
> struct dentry *dentry, struct inode *inode,
> - const char *name, void *buffer, size_t size)
> + const char *name, void *buffer, size_t size,
> + int flags)
> {
> struct v9fs_session_info *v9ses;
> struct posix_acl *acl;
> diff --git a/fs/9p/xattr.c b/fs/9p/xattr.c
> index ac8ff8ca4c11..5cfa772452fd 100644
> --- a/fs/9p/xattr.c
> +++ b/fs/9p/xattr.c
> @@ -139,7 +139,8 @@ ssize_t v9fs_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size)
>
> static int v9fs_xattr_handler_get(const struct xattr_handler *handler,
> struct dentry *dentry, struct inode *inode,
> - const char *name, void *buffer, size_t size)
> + const char *name, void *buffer, size_t size,
> + int flags)
> {
> const char *full_name = xattr_full_name(handler, name);
>
> diff --git a/fs/afs/xattr.c b/fs/afs/xattr.c
> index 5552d034090a..e6509c21f08a 100644
> --- a/fs/afs/xattr.c
> +++ b/fs/afs/xattr.c
> @@ -334,7 +334,7 @@ static const struct xattr_handler afs_xattr_yfs_handler = {
> static int afs_xattr_get_cell(const struct xattr_handler *handler,
> struct dentry *dentry,
> struct inode *inode, const char *name,
> - void *buffer, size_t size)
> + void *buffer, size_t size, int flags)
> {
> struct afs_vnode *vnode = AFS_FS_I(inode);
> struct afs_cell *cell = vnode->volume->cell;
> @@ -361,7 +361,7 @@ static const struct xattr_handler afs_xattr_afs_cell_handler = {
> static int afs_xattr_get_fid(const struct xattr_handler *handler,
> struct dentry *dentry,
> struct inode *inode, const char *name,
> - void *buffer, size_t size)
> + void *buffer, size_t size, int flags)
> {
> struct afs_vnode *vnode = AFS_FS_I(inode);
> char text[16 + 1 + 24 + 1 + 8 + 1];
> @@ -397,7 +397,7 @@ static const struct xattr_handler afs_xattr_afs_fid_handler = {
> static int afs_xattr_get_volume(const struct xattr_handler *handler,
> struct dentry *dentry,
> struct inode *inode, const char *name,
> - void *buffer, size_t size)
> + void *buffer, size_t size, int flags)
> {
> struct afs_vnode *vnode = AFS_FS_I(inode);
> const char *volname = vnode->volume->name;
> diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c
> index 95d9aebff2c4..1e522e145344 100644
> --- a/fs/btrfs/xattr.c
> +++ b/fs/btrfs/xattr.c
> @@ -353,7 +353,8 @@ ssize_t btrfs_listxattr(struct dentry *dentry, char *buffer, size_t size)
>
> static int btrfs_xattr_handler_get(const struct xattr_handler *handler,
> struct dentry *unused, struct inode *inode,
> - const char *name, void *buffer, size_t size)
> + const char *name, void *buffer, size_t size,
> + int flags)
> {
> name = xattr_full_name(handler, name);
> return btrfs_getxattr(inode, name, buffer, size);
> diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
> index 37b458a9af3a..edb7eb9ae83e 100644
> --- a/fs/ceph/xattr.c
> +++ b/fs/ceph/xattr.c
> @@ -1171,7 +1171,8 @@ int __ceph_setxattr(struct inode *inode, const char *name,
>
> static int ceph_get_xattr_handler(const struct xattr_handler *handler,
> struct dentry *dentry, struct inode *inode,
> - const char *name, void *value, size_t size)
> + const char *name, void *value, size_t size,
> + int flags)
> {
> if (!ceph_is_valid_xattr(name))
> return -EOPNOTSUPP;
> diff --git a/fs/cifs/xattr.c b/fs/cifs/xattr.c
> index 9076150758d8..7f71c06ce631 100644
> --- a/fs/cifs/xattr.c
> +++ b/fs/cifs/xattr.c
> @@ -199,7 +199,7 @@ static int cifs_creation_time_get(struct dentry *dentry, struct inode *inode,
>
> static int cifs_xattr_get(const struct xattr_handler *handler,
> struct dentry *dentry, struct inode *inode,
> - const char *name, void *value, size_t size)
> + const char *name, void *value, size_t size, int flags)
> {
> ssize_t rc = -EOPNOTSUPP;
> unsigned int xid;
> diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
> index 18426f4855f1..c710c7533729 100644
> --- a/fs/ecryptfs/inode.c
> +++ b/fs/ecryptfs/inode.c
> @@ -1018,7 +1018,8 @@ ecryptfs_getxattr_lower(struct dentry *lower_dentry, struct inode *lower_inode,
> goto out;
> }
> inode_lock(lower_inode);
> - rc = __vfs_getxattr(lower_dentry, lower_inode, name, value, size);
> + rc = __vfs_getxattr(lower_dentry, lower_inode, name, value, size,
> + XATTR_NOSECURITY);
> inode_unlock(lower_inode);
> out:
> return rc;
> @@ -1103,7 +1104,8 @@ const struct inode_operations ecryptfs_main_iops = {
>
> static int ecryptfs_xattr_get(const struct xattr_handler *handler,
> struct dentry *dentry, struct inode *inode,
> - const char *name, void *buffer, size_t size)
> + const char *name, void *buffer, size_t size,
> + int flags)
> {
> return ecryptfs_getxattr(dentry, inode, name, buffer, size);
> }
> diff --git a/fs/ecryptfs/mmap.c b/fs/ecryptfs/mmap.c
> index cffa0c1ec829..2362be3e3b4d 100644
> --- a/fs/ecryptfs/mmap.c
> +++ b/fs/ecryptfs/mmap.c
> @@ -422,7 +422,7 @@ static int ecryptfs_write_inode_size_to_xattr(struct inode *ecryptfs_inode)
> }
> inode_lock(lower_inode);
> size = __vfs_getxattr(lower_dentry, lower_inode, ECRYPTFS_XATTR_NAME,
> - xattr_virt, PAGE_SIZE);
> + xattr_virt, PAGE_SIZE, XATTR_NOSECURITY);
> if (size < 0)
> size = 8;
> put_unaligned_be64(i_size_read(ecryptfs_inode), xattr_virt);
> diff --git a/fs/ext2/xattr_trusted.c b/fs/ext2/xattr_trusted.c
> index 49add1107850..8d313664f0fa 100644
> --- a/fs/ext2/xattr_trusted.c
> +++ b/fs/ext2/xattr_trusted.c
> @@ -18,7 +18,7 @@ ext2_xattr_trusted_list(struct dentry *dentry)
> static int
> ext2_xattr_trusted_get(const struct xattr_handler *handler,
> struct dentry *unused, struct inode *inode,
> - const char *name, void *buffer, size_t size)
> + const char *name, void *buffer, size_t size, int flags)
> {
> return ext2_xattr_get(inode, EXT2_XATTR_INDEX_TRUSTED, name,
> buffer, size);
> diff --git a/fs/ext2/xattr_user.c b/fs/ext2/xattr_user.c
> index c243a3b4d69d..712b7c95cc64 100644
> --- a/fs/ext2/xattr_user.c
> +++ b/fs/ext2/xattr_user.c
> @@ -20,7 +20,7 @@ ext2_xattr_user_list(struct dentry *dentry)
> static int
> ext2_xattr_user_get(const struct xattr_handler *handler,
> struct dentry *unused, struct inode *inode,
> - const char *name, void *buffer, size_t size)
> + const char *name, void *buffer, size_t size, int flags)
> {
> if (!test_opt(inode->i_sb, XATTR_USER))
> return -EOPNOTSUPP;
> diff --git a/fs/ext4/xattr_security.c b/fs/ext4/xattr_security.c
> index 197a9d8a15ef..50fb71393fb6 100644
> --- a/fs/ext4/xattr_security.c
> +++ b/fs/ext4/xattr_security.c
> @@ -15,7 +15,7 @@
> static int
> ext4_xattr_security_get(const struct xattr_handler *handler,
> struct dentry *unused, struct inode *inode,
> - const char *name, void *buffer, size_t size)
> + const char *name, void *buffer, size_t size, int flags)
> {
> return ext4_xattr_get(inode, EXT4_XATTR_INDEX_SECURITY,
> name, buffer, size);
> diff --git a/fs/ext4/xattr_trusted.c b/fs/ext4/xattr_trusted.c
> index e9389e5d75c3..64bd8f86c1f1 100644
> --- a/fs/ext4/xattr_trusted.c
> +++ b/fs/ext4/xattr_trusted.c
> @@ -22,7 +22,7 @@ ext4_xattr_trusted_list(struct dentry *dentry)
> static int
> ext4_xattr_trusted_get(const struct xattr_handler *handler,
> struct dentry *unused, struct inode *inode,
> - const char *name, void *buffer, size_t size)
> + const char *name, void *buffer, size_t size, int flags)
> {
> return ext4_xattr_get(inode, EXT4_XATTR_INDEX_TRUSTED,
> name, buffer, size);
> diff --git a/fs/ext4/xattr_user.c b/fs/ext4/xattr_user.c
> index d4546184b34b..b7301373820e 100644
> --- a/fs/ext4/xattr_user.c
> +++ b/fs/ext4/xattr_user.c
> @@ -21,7 +21,7 @@ ext4_xattr_user_list(struct dentry *dentry)
> static int
> ext4_xattr_user_get(const struct xattr_handler *handler,
> struct dentry *unused, struct inode *inode,
> - const char *name, void *buffer, size_t size)
> + const char *name, void *buffer, size_t size, int flags)
> {
> if (!test_opt(inode->i_sb, XATTR_USER))
> return -EOPNOTSUPP;
> diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
> index b32c45621679..76559da8dfba 100644
> --- a/fs/f2fs/xattr.c
> +++ b/fs/f2fs/xattr.c
> @@ -24,7 +24,7 @@
>
> static int f2fs_xattr_generic_get(const struct xattr_handler *handler,
> struct dentry *unused, struct inode *inode,
> - const char *name, void *buffer, size_t size)
> + const char *name, void *buffer, size_t size, int flags)
> {
> struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
>
> @@ -79,7 +79,7 @@ static bool f2fs_xattr_trusted_list(struct dentry *dentry)
>
> static int f2fs_xattr_advise_get(const struct xattr_handler *handler,
> struct dentry *unused, struct inode *inode,
> - const char *name, void *buffer, size_t size)
> + const char *name, void *buffer, size_t size, int flags)
> {
> if (buffer)
> *((char *)buffer) = F2FS_I(inode)->i_advise;
> diff --git a/fs/fuse/xattr.c b/fs/fuse/xattr.c
> index 433717640f78..d1ef7808304e 100644
> --- a/fs/fuse/xattr.c
> +++ b/fs/fuse/xattr.c
> @@ -176,7 +176,7 @@ int fuse_removexattr(struct inode *inode, const char *name)
>
> static int fuse_xattr_get(const struct xattr_handler *handler,
> struct dentry *dentry, struct inode *inode,
> - const char *name, void *value, size_t size)
> + const char *name, void *value, size_t size, int flags)
> {
> return fuse_getxattr(inode, name, value, size);
> }
> @@ -199,7 +199,7 @@ static bool no_xattr_list(struct dentry *dentry)
>
> static int no_xattr_get(const struct xattr_handler *handler,
> struct dentry *dentry, struct inode *inode,
> - const char *name, void *value, size_t size)
> + const char *name, void *value, size_t size, int flags)
> {
> return -EOPNOTSUPP;
> }
> diff --git a/fs/gfs2/xattr.c b/fs/gfs2/xattr.c
> index bbe593d16bea..a9db067a99c1 100644
> --- a/fs/gfs2/xattr.c
> +++ b/fs/gfs2/xattr.c
> @@ -588,7 +588,8 @@ static int __gfs2_xattr_get(struct inode *inode, const char *name,
>
> static int gfs2_xattr_get(const struct xattr_handler *handler,
> struct dentry *unused, struct inode *inode,
> - const char *name, void *buffer, size_t size)
> + const char *name, void *buffer, size_t size,
> + int flags)
> {
> struct gfs2_inode *ip = GFS2_I(inode);
> struct gfs2_holder gh;
> diff --git a/fs/hfs/attr.c b/fs/hfs/attr.c
> index 74fa62643136..08222a9c5d31 100644
> --- a/fs/hfs/attr.c
> +++ b/fs/hfs/attr.c
> @@ -115,7 +115,7 @@ static ssize_t __hfs_getxattr(struct inode *inode, enum hfs_xattr_type type,
>
> static int hfs_xattr_get(const struct xattr_handler *handler,
> struct dentry *unused, struct inode *inode,
> - const char *name, void *value, size_t size)
> + const char *name, void *value, size_t size, int flags)
> {
> return __hfs_getxattr(inode, handler->flags, value, size);
> }
> diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
> index bb0b27d88e50..381c2aaedbc8 100644
> --- a/fs/hfsplus/xattr.c
> +++ b/fs/hfsplus/xattr.c
> @@ -839,7 +839,8 @@ static int hfsplus_removexattr(struct inode *inode, const char *name)
>
> static int hfsplus_osx_getxattr(const struct xattr_handler *handler,
> struct dentry *unused, struct inode *inode,
> - const char *name, void *buffer, size_t size)
> + const char *name, void *buffer, size_t size,
> + int flags)
> {
> /*
> * Don't allow retrieving properly prefixed attributes
> diff --git a/fs/hfsplus/xattr_trusted.c b/fs/hfsplus/xattr_trusted.c
> index fbad91e1dada..54d926314f8c 100644
> --- a/fs/hfsplus/xattr_trusted.c
> +++ b/fs/hfsplus/xattr_trusted.c
> @@ -14,7 +14,8 @@
>
> static int hfsplus_trusted_getxattr(const struct xattr_handler *handler,
> struct dentry *unused, struct inode *inode,
> - const char *name, void *buffer, size_t size)
> + const char *name, void *buffer,
> + size_t size, int flags)
> {
> return hfsplus_getxattr(inode, name, buffer, size,
> XATTR_TRUSTED_PREFIX,
> diff --git a/fs/hfsplus/xattr_user.c b/fs/hfsplus/xattr_user.c
> index 74d19faf255e..4d2b1ffff887 100644
> --- a/fs/hfsplus/xattr_user.c
> +++ b/fs/hfsplus/xattr_user.c
> @@ -14,7 +14,8 @@
>
> static int hfsplus_user_getxattr(const struct xattr_handler *handler,
> struct dentry *unused, struct inode *inode,
> - const char *name, void *buffer, size_t size)
> + const char *name, void *buffer, size_t size,
> + int flags)
> {
>
> return hfsplus_getxattr(inode, name, buffer, size,
> diff --git a/fs/jffs2/security.c b/fs/jffs2/security.c
> index c2332e30f218..e6f42fe435af 100644
> --- a/fs/jffs2/security.c
> +++ b/fs/jffs2/security.c
> @@ -50,7 +50,8 @@ int jffs2_init_security(struct inode *inode, struct inode *dir,
> /* ---- XATTR Handler for "security.*" ----------------- */
> static int jffs2_security_getxattr(const struct xattr_handler *handler,
> struct dentry *unused, struct inode *inode,
> - const char *name, void *buffer, size_t size)
> + const char *name, void *buffer, size_t size,
> + int flags)
> {
> return do_jffs2_getxattr(inode, JFFS2_XPREFIX_SECURITY,
> name, buffer, size);
> diff --git a/fs/jffs2/xattr_trusted.c b/fs/jffs2/xattr_trusted.c
> index 5d6030826c52..9dccaae549f5 100644
> --- a/fs/jffs2/xattr_trusted.c
> +++ b/fs/jffs2/xattr_trusted.c
> @@ -18,7 +18,8 @@
>
> static int jffs2_trusted_getxattr(const struct xattr_handler *handler,
> struct dentry *unused, struct inode *inode,
> - const char *name, void *buffer, size_t size)
> + const char *name, void *buffer, size_t size,
> + int flags)
> {
> return do_jffs2_getxattr(inode, JFFS2_XPREFIX_TRUSTED,
> name, buffer, size);
> diff --git a/fs/jffs2/xattr_user.c b/fs/jffs2/xattr_user.c
> index 9d027b4abcf9..c0983a3e810b 100644
> --- a/fs/jffs2/xattr_user.c
> +++ b/fs/jffs2/xattr_user.c
> @@ -18,7 +18,8 @@
>
> static int jffs2_user_getxattr(const struct xattr_handler *handler,
> struct dentry *unused, struct inode *inode,
> - const char *name, void *buffer, size_t size)
> + const char *name, void *buffer, size_t size,
> + int flags)
> {
> return do_jffs2_getxattr(inode, JFFS2_XPREFIX_USER,
> name, buffer, size);
> diff --git a/fs/jfs/xattr.c b/fs/jfs/xattr.c
> index db41e7803163..5c79a35bf62f 100644
> --- a/fs/jfs/xattr.c
> +++ b/fs/jfs/xattr.c
> @@ -925,7 +925,7 @@ static int __jfs_xattr_set(struct inode *inode, const char *name,
>
> static int jfs_xattr_get(const struct xattr_handler *handler,
> struct dentry *unused, struct inode *inode,
> - const char *name, void *value, size_t size)
> + const char *name, void *value, size_t size, int flags)
> {
> name = xattr_full_name(handler, name);
> return __jfs_getxattr(inode, name, value, size);
> @@ -942,7 +942,8 @@ static int jfs_xattr_set(const struct xattr_handler *handler,
>
> static int jfs_xattr_get_os2(const struct xattr_handler *handler,
> struct dentry *unused, struct inode *inode,
> - const char *name, void *value, size_t size)
> + const char *name, void *value, size_t size,
> + int flags)
> {
> if (is_known_namespace(name))
> return -EOPNOTSUPP;
> diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c
> index f3f3984cce80..89db24ce644e 100644
> --- a/fs/kernfs/inode.c
> +++ b/fs/kernfs/inode.c
> @@ -309,7 +309,8 @@ int kernfs_xattr_set(struct kernfs_node *kn, const char *name,
>
> static int kernfs_vfs_xattr_get(const struct xattr_handler *handler,
> struct dentry *unused, struct inode *inode,
> - const char *suffix, void *value, size_t size)
> + const char *suffix, void *value, size_t size,
> + int flags)
> {
> const char *name = xattr_full_name(handler, suffix);
> struct kernfs_node *kn = inode->i_private;
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 39896afc6edf..5e6a58685cd0 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -7203,7 +7203,8 @@ static int nfs4_xattr_set_nfs4_acl(const struct xattr_handler *handler,
>
> static int nfs4_xattr_get_nfs4_acl(const struct xattr_handler *handler,
> struct dentry *unused, struct inode *inode,
> - const char *key, void *buf, size_t buflen)
> + const char *key, void *buf, size_t buflen,
> + int flags)
> {
> return nfs4_proc_get_acl(inode, buf, buflen);
> }
> @@ -7228,7 +7229,8 @@ static int nfs4_xattr_set_nfs4_label(const struct xattr_handler *handler,
>
> static int nfs4_xattr_get_nfs4_label(const struct xattr_handler *handler,
> struct dentry *unused, struct inode *inode,
> - const char *key, void *buf, size_t buflen)
> + const char *key, void *buf, size_t buflen,
> + int flags)
> {
> if (security_ismaclabel(key))
> return nfs4_get_security_label(inode, buf, buflen);
> diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
> index 385f3aaa2448..06e615642422 100644
> --- a/fs/ocfs2/xattr.c
> +++ b/fs/ocfs2/xattr.c
> @@ -7245,7 +7245,8 @@ int ocfs2_init_security_and_acl(struct inode *dir,
> */
> static int ocfs2_xattr_security_get(const struct xattr_handler *handler,
> struct dentry *unused, struct inode *inode,
> - const char *name, void *buffer, size_t size)
> + const char *name, void *buffer, size_t size,
> + int flags)
> {
> return ocfs2_xattr_get(inode, OCFS2_XATTR_INDEX_SECURITY,
> name, buffer, size);
> @@ -7317,7 +7318,8 @@ const struct xattr_handler ocfs2_xattr_security_handler = {
> */
> static int ocfs2_xattr_trusted_get(const struct xattr_handler *handler,
> struct dentry *unused, struct inode *inode,
> - const char *name, void *buffer, size_t size)
> + const char *name, void *buffer, size_t size,
> + int flags)
> {
> return ocfs2_xattr_get(inode, OCFS2_XATTR_INDEX_TRUSTED,
> name, buffer, size);
> @@ -7343,7 +7345,8 @@ const struct xattr_handler ocfs2_xattr_trusted_handler = {
> */
> static int ocfs2_xattr_user_get(const struct xattr_handler *handler,
> struct dentry *unused, struct inode *inode,
> - const char *name, void *buffer, size_t size)
> + const char *name, void *buffer, size_t size,
> + int flags)
> {
> struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
>
> diff --git a/fs/orangefs/xattr.c b/fs/orangefs/xattr.c
> index bdc285aea360..ef4180bff7bb 100644
> --- a/fs/orangefs/xattr.c
> +++ b/fs/orangefs/xattr.c
> @@ -541,7 +541,8 @@ static int orangefs_xattr_get_default(const struct xattr_handler *handler,
> struct inode *inode,
> const char *name,
> void *buffer,
> - size_t size)
> + size_t size,
> + int flags)
> {
> return orangefs_inode_getxattr(inode, name, buffer, size);
>
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index b368e2e102fa..a7b21f2ea2dd 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -854,7 +854,7 @@ static unsigned int ovl_split_lowerdirs(char *str)
> static int __maybe_unused
> ovl_posix_acl_xattr_get(const struct xattr_handler *handler,
> struct dentry *dentry, struct inode *inode,
> - const char *name, void *buffer, size_t size)
> + const char *name, void *buffer, size_t size, int flags)
> {
> return ovl_xattr_get(dentry, inode, handler->name, buffer, size);
> }
> @@ -919,7 +919,8 @@ ovl_posix_acl_xattr_set(const struct xattr_handler *handler,
>
> static int ovl_own_xattr_get(const struct xattr_handler *handler,
> struct dentry *dentry, struct inode *inode,
> - const char *name, void *buffer, size_t size)
> + const char *name, void *buffer, size_t size,
> + int flags)
> {
> return -EOPNOTSUPP;
> }
> @@ -934,7 +935,8 @@ static int ovl_own_xattr_set(const struct xattr_handler *handler,
>
> static int ovl_other_xattr_get(const struct xattr_handler *handler,
> struct dentry *dentry, struct inode *inode,
> - const char *name, void *buffer, size_t size)
> + const char *name, void *buffer, size_t size,
> + int flags)
> {
> return ovl_xattr_get(dentry, inode, name, buffer, size);
> }
> diff --git a/fs/posix_acl.c b/fs/posix_acl.c
> index 84ad1c90d535..cd55621e570b 100644
> --- a/fs/posix_acl.c
> +++ b/fs/posix_acl.c
> @@ -832,7 +832,7 @@ EXPORT_SYMBOL (posix_acl_to_xattr);
> static int
> posix_acl_xattr_get(const struct xattr_handler *handler,
> struct dentry *unused, struct inode *inode,
> - const char *name, void *value, size_t size)
> + const char *name, void *value, size_t size, int flags)
> {
> struct posix_acl *acl;
> int error;
> diff --git a/fs/reiserfs/xattr_security.c b/fs/reiserfs/xattr_security.c
> index 20be9a0e5870..eedfa07a4fd0 100644
> --- a/fs/reiserfs/xattr_security.c
> +++ b/fs/reiserfs/xattr_security.c
> @@ -11,7 +11,8 @@
>
> static int
> security_get(const struct xattr_handler *handler, struct dentry *unused,
> - struct inode *inode, const char *name, void *buffer, size_t size)
> + struct inode *inode, const char *name, void *buffer, size_t size,
> + int flags)
> {
> if (IS_PRIVATE(inode))
> return -EPERM;
> diff --git a/fs/reiserfs/xattr_trusted.c b/fs/reiserfs/xattr_trusted.c
> index 5ed48da3d02b..2d11d98605dd 100644
> --- a/fs/reiserfs/xattr_trusted.c
> +++ b/fs/reiserfs/xattr_trusted.c
> @@ -10,7 +10,8 @@
>
> static int
> trusted_get(const struct xattr_handler *handler, struct dentry *unused,
> - struct inode *inode, const char *name, void *buffer, size_t size)
> + struct inode *inode, const char *name, void *buffer, size_t size,
> + int flags)
> {
> if (!capable(CAP_SYS_ADMIN) || IS_PRIVATE(inode))
> return -EPERM;
> diff --git a/fs/reiserfs/xattr_user.c b/fs/reiserfs/xattr_user.c
> index a573ca45bacc..2a59d85c69c9 100644
> --- a/fs/reiserfs/xattr_user.c
> +++ b/fs/reiserfs/xattr_user.c
> @@ -9,7 +9,8 @@
>
> static int
> user_get(const struct xattr_handler *handler, struct dentry *unused,
> - struct inode *inode, const char *name, void *buffer, size_t size)
> + struct inode *inode, const char *name, void *buffer, size_t size,
> + int flags)
> {
> if (!reiserfs_xattrs_user(inode->i_sb))
> return -EOPNOTSUPP;
> diff --git a/fs/squashfs/xattr.c b/fs/squashfs/xattr.c
> index e1e3f3dd5a06..d8d58c990652 100644
> --- a/fs/squashfs/xattr.c
> +++ b/fs/squashfs/xattr.c
> @@ -204,7 +204,7 @@ static int squashfs_xattr_handler_get(const struct xattr_handler *handler,
> struct dentry *unused,
> struct inode *inode,
> const char *name,
> - void *buffer, size_t size)
> + void *buffer, size_t size, int flags)
> {
> return squashfs_xattr_get(inode, handler->flags, name,
> buffer, size);
> diff --git a/fs/xattr.c b/fs/xattr.c
> index 90dd78f0eb27..71f887518d6f 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -281,7 +281,7 @@ vfs_getxattr_alloc(struct dentry *dentry, const char *name, char **xattr_value,
> return PTR_ERR(handler);
> if (!handler->get)
> return -EOPNOTSUPP;
> - error = handler->get(handler, dentry, inode, name, NULL, 0);
> + error = handler->get(handler, dentry, inode, name, NULL, 0, 0);
> if (error < 0)
> return error;
>
> @@ -292,32 +292,20 @@ vfs_getxattr_alloc(struct dentry *dentry, const char *name, char **xattr_value,
> memset(value, 0, error + 1);
> }
>
> - error = handler->get(handler, dentry, inode, name, value, error);
> + error = handler->get(handler, dentry, inode, name, value, error, 0);
> *xattr_value = value;
> return error;
> }
>
> ssize_t
> __vfs_getxattr(struct dentry *dentry, struct inode *inode, const char *name,
> - void *value, size_t size)
> + void *value, size_t size, int flags)
> {
> const struct xattr_handler *handler;
> -
> - handler = xattr_resolve_name(inode, &name);
> - if (IS_ERR(handler))
> - return PTR_ERR(handler);
> - if (!handler->get)
> - return -EOPNOTSUPP;
> - return handler->get(handler, dentry, inode, name, value, size);
> -}
> -EXPORT_SYMBOL(__vfs_getxattr);
> -
> -ssize_t
> -vfs_getxattr(struct dentry *dentry, const char *name, void *value, size_t size)
> -{
> - struct inode *inode = dentry->d_inode;
> int error;
>
> + if (flags & XATTR_NOSECURITY)
> + goto nolsm;
> error = xattr_permission(inode, name, MAY_READ);
> if (error)
> return error;
> @@ -339,7 +327,19 @@ vfs_getxattr(struct dentry *dentry, const char *name, void *value, size_t size)
> return ret;
> }
> nolsm:
> - return __vfs_getxattr(dentry, inode, name, value, size);
> + handler = xattr_resolve_name(inode, &name);
> + if (IS_ERR(handler))
> + return PTR_ERR(handler);
> + if (!handler->get)
> + return -EOPNOTSUPP;
> + return handler->get(handler, dentry, inode, name, value, size, flags);
> +}
> +EXPORT_SYMBOL(__vfs_getxattr);
> +
> +ssize_t
> +vfs_getxattr(struct dentry *dentry, const char *name, void *value, size_t size)
> +{
> + return __vfs_getxattr(dentry, dentry->d_inode, name, value, size, 0);
> }
> EXPORT_SYMBOL_GPL(vfs_getxattr);
>
> diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c
> index 3123b5aaad2a..cafc99c48e20 100644
> --- a/fs/xfs/xfs_xattr.c
> +++ b/fs/xfs/xfs_xattr.c
> @@ -18,7 +18,8 @@
>
> static int
> xfs_xattr_get(const struct xattr_handler *handler, struct dentry *unused,
> - struct inode *inode, const char *name, void *value, size_t size)
> + struct inode *inode, const char *name, void *value, size_t size,
> + int flags)
> {
> int xflags = handler->flags;
> struct xfs_inode *ip = XFS_I(inode);
> diff --git a/include/linux/xattr.h b/include/linux/xattr.h
> index 6dad031be3c2..e5c191b30818 100644
> --- a/include/linux/xattr.h
> +++ b/include/linux/xattr.h
> @@ -30,10 +30,10 @@ struct xattr_handler {
> const char *prefix;
> int flags; /* fs private flags */
> bool (*list)(struct dentry *dentry);
> - int (*get)(const struct xattr_handler *, struct dentry *dentry,
> + int (*get)(const struct xattr_handler *handler, struct dentry *dentry,
> struct inode *inode, const char *name, void *buffer,
> - size_t size);
> - int (*set)(const struct xattr_handler *, struct dentry *dentry,
> + size_t size, int flags);
> + int (*set)(const struct xattr_handler *handler, struct dentry *dentry,
> struct inode *inode, const char *name, const void *buffer,
> size_t size, int flags);
> };
> @@ -46,7 +46,8 @@ struct xattr {
> size_t value_len;
> };
>
> -ssize_t __vfs_getxattr(struct dentry *, struct inode *, const char *, void *, size_t);
> +ssize_t __vfs_getxattr(struct dentry *, struct inode *, const char *, void *,
> + size_t, int);
> ssize_t vfs_getxattr(struct dentry *, const char *, void *, size_t);
> ssize_t vfs_listxattr(struct dentry *d, char *list, size_t size);
> int __vfs_setxattr(struct dentry *, struct inode *, const char *, const void *, size_t, int);
> diff --git a/include/uapi/linux/xattr.h b/include/uapi/linux/xattr.h
> index c1395b5bd432..1216d777d210 100644
> --- a/include/uapi/linux/xattr.h
> +++ b/include/uapi/linux/xattr.h
> @@ -17,8 +17,9 @@
> #if __UAPI_DEF_XATTR
> #define __USE_KERNEL_XATTR_DEFS
>
> -#define XATTR_CREATE 0x1 /* set value, fail if attr already exists */
> -#define XATTR_REPLACE 0x2 /* set value, fail if attr does not exist */
> +#define XATTR_CREATE 0x1 /* set value, fail if attr already exists */
> +#define XATTR_REPLACE 0x2 /* set value, fail if attr does not exist */
> +#define XATTR_NOSECURITY 0x4 /* get value, do not involve security check */
> #endif
>
> /* Namespaces */
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 626d8c74b973..34d3818b4424 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -3206,7 +3206,8 @@ static int shmem_initxattrs(struct inode *inode,
>
> static int shmem_xattr_handler_get(const struct xattr_handler *handler,
> struct dentry *unused, struct inode *inode,
> - const char *name, void *buffer, size_t size)
> + const char *name, void *buffer, size_t size,
> + int flags)
> {
> struct shmem_inode_info *info = SHMEM_I(inode);
>
> diff --git a/net/socket.c b/net/socket.c
> index 6a9ab7a8b1d2..6b0fea92dd02 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -300,7 +300,8 @@ static const struct dentry_operations sockfs_dentry_operations = {
>
> static int sockfs_xattr_get(const struct xattr_handler *handler,
> struct dentry *dentry, struct inode *inode,
> - const char *suffix, void *value, size_t size)
> + const char *suffix, void *value, size_t size,
> + int flags)
> {
> if (value) {
> if (dentry->d_name.len + 1 > size)
> diff --git a/security/commoncap.c b/security/commoncap.c
> index f4ee0ae106b2..378a2f66a73d 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -297,7 +297,8 @@ int cap_inode_need_killpriv(struct dentry *dentry)
> struct inode *inode = d_backing_inode(dentry);
> int error;
>
> - error = __vfs_getxattr(dentry, inode, XATTR_NAME_CAPS, NULL, 0);
> + error = __vfs_getxattr(dentry, inode, XATTR_NAME_CAPS, NULL, 0,
> + XATTR_NOSECURITY);
> return error > 0;
> }
>
> @@ -586,7 +587,8 @@ int get_vfs_caps_from_disk(const struct dentry *dentry, struct cpu_vfs_cap_data
>
> fs_ns = inode->i_sb->s_user_ns;
> size = __vfs_getxattr((struct dentry *)dentry, inode,
> - XATTR_NAME_CAPS, &data, XATTR_CAPS_SZ);
> + XATTR_NAME_CAPS, &data, XATTR_CAPS_SZ,
> + XATTR_NOSECURITY);
> if (size == -ENODATA || size == -EOPNOTSUPP)
> /* no data, that's ok */
> return -ENODATA;
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index f9a81b187fae..921c8f2afcaf 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -100,7 +100,8 @@ static int evm_find_protected_xattrs(struct dentry *dentry)
> return -EOPNOTSUPP;
>
> list_for_each_entry_rcu(xattr, &evm_config_xattrnames, list) {
> - error = __vfs_getxattr(dentry, inode, xattr->name, NULL, 0);
> + error = __vfs_getxattr(dentry, inode, xattr->name, NULL, 0,
> + XATTR_NOSECURITY);
> if (error < 0) {
> if (error == -ENODATA)
> continue;
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 74dd46de01b6..b0822da0658f 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -552,7 +552,8 @@ static int sb_finish_set_opts(struct super_block *sb)
> goto out;
> }
>
> - rc = __vfs_getxattr(root, root_inode, XATTR_NAME_SELINUX, NULL, 0);
> + rc = __vfs_getxattr(root, root_inode, XATTR_NAME_SELINUX, NULL,
> + 0, XATTR_NOSECURITY);
> if (rc < 0 && rc != -ENODATA) {
> if (rc == -EOPNOTSUPP)
> pr_warn("SELinux: (dev %s, type "
> @@ -1378,12 +1379,14 @@ static int inode_doinit_use_xattr(struct inode *inode, struct dentry *dentry,
> return -ENOMEM;
>
> context[len] = '\0';
> - rc = __vfs_getxattr(dentry, inode, XATTR_NAME_SELINUX, context, len);
> + rc = __vfs_getxattr(dentry, inode, XATTR_NAME_SELINUX, context, len,
> + XATTR_NOSECURITY);
> if (rc == -ERANGE) {
> kfree(context);
>
> /* Need a larger buffer. Query for the right size. */
> - rc = __vfs_getxattr(dentry, inode, XATTR_NAME_SELINUX, NULL, 0);
> + rc = __vfs_getxattr(dentry, inode, XATTR_NAME_SELINUX, NULL, 0,
> + XATTR_NOSECURITY);
> if (rc < 0)
> return rc;
>
> @@ -1394,7 +1397,7 @@ static int inode_doinit_use_xattr(struct inode *inode, struct dentry *dentry,
>
> context[len] = '\0';
> rc = __vfs_getxattr(dentry, inode, XATTR_NAME_SELINUX,
> - context, len);
> + context, len, XATTR_NOSECURITY);
> }
> if (rc < 0) {
> kfree(context);
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 4c5e5a438f8b..158b35772be1 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -292,7 +292,8 @@ static struct smack_known *smk_fetch(const char *name, struct inode *ip,
> if (buffer == NULL)
> return ERR_PTR(-ENOMEM);
>
> - rc = __vfs_getxattr(dp, ip, name, buffer, SMK_LONGLABEL);
> + rc = __vfs_getxattr(dp, ip, name, buffer, SMK_LONGLABEL,
> + XATTR_NOSECURITY);
> if (rc < 0)
> skp = ERR_PTR(rc);
> else if (rc == 0)
> @@ -3442,7 +3443,7 @@ static void smack_d_instantiate(struct dentry *opt_dentry, struct inode *inode)
> } else {
> rc = __vfs_getxattr(dp, inode,
> XATTR_NAME_SMACKTRANSMUTE, trattr,
> - TRANS_TRUE_SIZE);
> + TRANS_TRUE_SIZE, XATTR_NOSECURITY);
> if (rc >= 0 && strncmp(trattr, TRANS_TRUE,
> TRANS_TRUE_SIZE) != 0)
> rc = -EINVAL;
^ permalink raw reply
* Re: [RFC/RFT v3 2/3] KEYS: trusted: move tpm2 trusted keys code
From: Jarkko Sakkinen @ 2019-08-08 15:15 UTC (permalink / raw)
To: Sumit Garg
Cc: keyrings, linux-integrity,
open list:HARDWARE RANDOM NUMBER GENERATOR CORE,
linux-security-module, dhowells, Herbert Xu, davem, peterhuewe,
jgg, jejb, Arnd Bergmann, Greg Kroah-Hartman, Mimi Zohar,
James Morris, Serge E. Hallyn, Casey Schaufler, Ard Biesheuvel,
Daniel Thompson, Linux Kernel Mailing List,
tee-dev @ lists . linaro . org
In-Reply-To: <CAFA6WYN-6MpP2TZQEz49BmjSQiMSqghVFWRZCCY0o1UVad1AFw@mail.gmail.com>
On Thu, Aug 08, 2019 at 06:51:38PM +0530, Sumit Garg wrote:
> It seems to be a functional change which I think requires proper unit
> testing. I am afraid that I don't posses a TPM device to test this and
> also very less conversant with tpm_buf code.
>
> So what I have done here is to rename existing TPM 1.x trusted keys
> code to use tpm1_buf.
>
> And I would be happy to integrate a tested patch if anyone familiar
> could work on this.
I can test it on TPM 1.2.
/Jarkko
^ 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