Re: [PATCH] Smack: Fix wrong copy size
On 10/2/2015 6:19 AM, José Bollo wrote:
> The function strncpy was copying an extra character
> when i == len (what is possible via revoke interface).
>
> Change-Id: Ic7452da05773e620a1d7bbc55e859c25a86c65f6
> Signed-off-by: José Bollo
> Signed-off-by: Stephane Desneux
Thank you for the patch. It appears that Lukasz Pawelczyk
had a fix for this already.
> ---
> security/smack/smack_access.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/security/smack/smack_access.c
> b/security/smack/smack_access.c
> index c062e94..930e548 100644
> --- a/security/smack/smack_access.c
> +++ b/security/smack/smack_access.c
> @@ -432,7 +432,7 @@ char *smk_parse_smack(const char *string, int len)
>
> smack = kzalloc(i + 1, GFP_KERNEL);
> if (smack != NULL) {
> - strncpy(smack, string, i + 1);
> + strncpy(smack, string, i);
> smack[i] = '\0';
> }
> return smack;
--
To unsubscribe from this list: send the line "unsubscribe
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 5/5] firmware: add an extensible system data helpers
On Thu, Oct 08, 2015 at 01:59:11PM -0400, Josh Boyer wrote:
> On Thu, Oct 1, 2015 at 1:44 PM, Luis R. Rodriguez
> > +static inline int desc_sync_found_call_cb(const struct sysdata_file_desc
> > *desc,
> > + const struct sysdata_file
> > *sysdata)
> > +{
> > + BUG_ON(desc->sync_reqs.mode != SYNCDATA_SYNC);
>
> ngh... Why do these inline functions all have BUG_ONs in them? If it
> is to catch a programming error, why can't you just return EINVAL like
> you do in the async function case? (Even that WARN_ON seems
> excessive).
Sure, I've replaced the pesky BUG_ON() with returning -EINVAL's.
Let me know if there is anything else.
Luis
--
To unsubscribe from this list: send the line "unsubscribe
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/5] firmware: generalize reading file contents as a helper
On Thu, Oct 08, 2015 at 01:36:53PM -0400, Josh Boyer wrote: > On Thu, Oct 1, 2015 at 1:44 PM, Luis R. Rodriguez > wrote: > > From: David Howells > > > > We'll want to reuse this same code later in order to > > read two separate types of file contents. This generalizes > > fw_read_file() for reading a file rebrands it as fw_read_file(). > > Er, maybe that should read "...fw_read_file_contents() for reading a > file and rebrands it as fw_read_file()." ? Thanks, corrected. > > This caller lets us pegs arbitrary data onto the target > > buffer and size if the file is found. > > This sentence is somewhat confusing. The data isn't arbitrary. It is > what the caller wants you to read from path. What is arbitrary, at > least in the context of this function, is the path passed to it. > Maybe rewrite this as: > > "The new function allows us to read file contents from arbitrary paths > and return the data and size of the files read." The path is arbitrary but what I meant by arbitrary data is that the data need no longer be firmware, whereas fw_read_file_contents() *did* require passing firmware_class data structures. What this does is it make the possibility of eventually making a more core system data file reader more obvious, so for instance the goal is to later share a reader with: - firmware_class: fw_read_file() - module: kernel_read() - kexec: copy_file_fd() I will clarify this in the commit log and also clarify the path is arbitrary as well as you note. > > While at it this cleans up the exit paths on fw_read_file(). > > > > Signed-off-by: David Howells > > Signed-off-by: Luis R. Rodriguez > > The code changes themselves look fine. Thank you for the review. Can I peg your Acked-by or Reviewed-by? How about this for a change in the commit log: firmware: generalize reading file contents as a helper We'll want to reuse this same code later in order to read two separate types of file contents. This generalizes fw_read_file_contents() for reading a file and rebrands it as fw_read_file(). This new caller is now generic and that path can be arbitrary, the caller is also agnostic to the firmware_class code now, which begs the possibility of code re-use with other similar callers in the kernel. For instance in the future we may want to share a solution with: - firmware_class: fw_read_file() - module: kernel_read() - kexec: copy_file_fd() While at it this also cleans up the exit paths on fw_read_file(). Luis -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] firmware: generalize "firmware" as "system data" helpers
On Tue, Oct 6, 2015 at 5:08 AM, Greg KH wrote: > Just responding to one thing at the moment: > > On Mon, Oct 05, 2015 at 11:22:22PM +0200, Luis R. Rodriguez wrote: >> * we should phase out the usermode helper from firmware_class long term > > You can "phase out", but you can not delete it as it's a user/kernel api > that we have to support for forever, sorry. Assuming that dell-rbu is the only in-tree legitimate user of the userhelper code, I'm curious if the code itself could simply move into that driver. It might help prevent the spread of reliance on an API we don't want to see grow in usage. We'd probably need to evaluate if the two new users could migrate off that. > Also, for some devices / use cases, the usermode helper is the solution > (think async loading of firmware when the host wants to do it.) Are any of those use cases in the kernel today, aside from dell-rbu? Would Luis' async mode to system data suffice? josh -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Introduces generic __list_splice_init_rcu();
On Tue, 2015-10-06 at 11:37 -0700, Paul E. McKenney wrote: > On Sun, Sep 27, 2015 at 06:10:28PM +0300, Petko Manolov wrote: > > __list_splice_init_rcu() can be used to splice lists forming both stack and > > queue structures, depending on its arguments. It is based on the initial > > list_splice_init_rcu() with a few minor modifications to help abstracting > > it. > > > > Signed-off-by: Petko Manolov > > At first glance, this looks sane. > > But who is using the new list_splice_tail_init_rcu() function? Hi, Paul. Up to now a single policy was loaded, normally in the initramfs, and never changed. Petko is adding support to extend the policy rules. The additional policies would be appended to the existing list, only after verifying the new set of rules are good. list.h contains list_splice() and list_splice_tail(), but the RCU equivalent functions don't exist. Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 5/5] firmware: add an extensible system data helpers
On Thu, Oct 1, 2015 at 1:44 PM, Luis R. Rodriguez
wrote:
> diff --git a/include/linux/sysdata.h b/include/linux/sysdata.h
> new file mode 100644
> index ..a69cf5ef082c
> --- /dev/null
> +++ b/include/linux/sysdata.h
> @@ -0,0 +1,208 @@
> +#ifndef _LINUX_SYSDATA_H
> +#define _LINUX_SYSDATA_H
> +
> +#include
> +#include
> +#include
> +
> +/*
> + * System Data internals
> + *
> + * Copyright (C) 2015 Luis R. Rodriguez
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public Licence
> + * as published by the Free Software Foundation; either version
> + * 2 of the Licence, or (at your option) any later version.
> + */
> +
> +struct sysdata_file {
> + size_t size;
> + const u8 *data;
> +
> + /* sysdata loader private fields */
> + void *priv;
> +};
> +
> +/**
> + * enum sync_data_mode - system data mode of operation
> + *
> + * SYNCDATA_SYNC: your call to request system data is synchronous. We will
> + * look for the system data file you have requested immediatley.
> + * SYNCDATA_ASYNC: your call to request system data is asynchronous. We will
> + * schedule the search for your system data file to be run at a later
> + * time.
> + */
> +enum sync_data_mode {
> + SYNCDATA_SYNC,
> + SYNCDATA_ASYNC,
> +};
> +
> +/* one per sync_data_mode */
> +union sysdata_file_cbs {
> + struct {
> + int __must_check (*found_cb)(void *, const struct
> sysdata_file *);
> + void *found_context;
> +
> + int __must_check (*opt_fail_cb)(void *);
> + void *opt_fail_context;
> + } sync;
> + struct {
> + void (*found_cb)(const struct sysdata_file *, void *);
> + void *found_context;
> +
> + void (*opt_fail_cb)(void *);
> + void *opt_fail_context;
> + } async;
> +};
> +
> +struct sysdata_file_sync_reqs {
> + enum sync_data_mode mode;
> + struct module *module;
> + gfp_t gfp;
> +};
> +
> +/**
> + * struct sysdata_file_desc - system data file descriptor
> + * @optional: if true it is not a hard requirement by the caller that this
> + * file be present. An error will not be recorded if the file is not
> + * found.
> + * @keep: if set the caller wants to claim ownership over the system data
> + * through one of its callbacks, it must later free it with
> + * release_sysdata_file(). By default this is set to false and the kernel
> + * will release the system data file for you after callback processing
> + * has completed.
> + * @sync_reqs: synchronization requirements, this will be taken care for you
> + * by default if you are usingy sdata_file_request(), otherwise you
> + * should provide your own requirements.
> + *
> + * This structure is set the by the driver and passed to the system data
> + * file helpers sysdata_file_request() or sysdata_file_request_async().
> + * It is intended to carry all requirements and specifications required
> + * to complete the task to get the requested system date file to the caller.
> + * If you wish to extend functionality of system data file requests you
> + * should extend this data structure and make use of the extensions on
> + * the callers to avoid unnecessary collateral evolutions.
> + *
> + * You are allowed to provide a callback to handle if a system data file was
> + * found or not. You do not need to provide a callback. You may also set
> + * an optional flag which would enable you to declare that the system data
> + * file is optional and that if it is not found an alternative callback be
> + * run for you.
> + *
> + * Refer to sysdata_file_request() and sysdata_file_request_async() for more
> + * details.
> + */
> +struct sysdata_file_desc {
> + bool optional;
> + bool keep;
> + struct sysdata_file_sync_reqs sync_reqs;
> + union sysdata_file_cbs cbs;
> +};
> +
> +/*
> + * We keep these template definitions to a minimum for the most
> + * popular requests.
> + */
> +
> +/* Typical sync data case */
> +#define SYSDATA_SYNC_FOUND(__found_cb, __context) \
> + .cbs.sync.found_cb = __found_cb,\
> + .cbs.sync.found_context = __context
> +
> +/* If you have one fallback routine */
> +#define SYSDATA_SYNC_OPT_CB(__found_cb, __context) \
> + .cbs.sync.opt_fail_cb = __found_cb, \
> + .cbs.sync.opt_fail_context = __context
> +
> +/*
> + * Used to define the default asynchronization requirements for
> + * sysdata_file_request_async(). Drivers can override.
> + */
> +#define SYSDATA_DEFAULT_ASYNC(__found_cb, __context) \
> + .sync_reqs = { \
> + .mode = SYNCDATA_ASYNC, \
> + .module = THIS_MODULE,
Re: [PATCH v2 4/5] firmware: generalize reading file contents as a helper
On Thu, Oct 1, 2015 at 1:44 PM, Luis R. Rodriguez wrote: > From: David Howells > > We'll want to reuse this same code later in order to > read two separate types of file contents. This generalizes > fw_read_file() for reading a file rebrands it as fw_read_file(). Er, maybe that should read "...fw_read_file_contents() for reading a file and rebrands it as fw_read_file()." ? > This caller lets us pegs arbitrary data onto the target > buffer and size if the file is found. This sentence is somewhat confusing. The data isn't arbitrary. It is what the caller wants you to read from path. What is arbitrary, at least in the context of this function, is the path passed to it. Maybe rewrite this as: "The new function allows us to read file contents from arbitrary paths and return the data and size of the files read." > While at it this cleans up the exit paths on fw_read_file(). > > Signed-off-by: David Howells > Signed-off-by: Luis R. Rodriguez The code changes themselves look fine. josh -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] security: selinux: Use a kmem_cache for allocation struct file_security_struct
On Monday, October 05, 2015 02:45:41 PM Sangwoo wrote:
> The size of struct file_security_struct is 16byte at my setup.
> But, the real allocation size for per each file_security_struct
> is 64bytes in my setup that kmalloc min size is 64bytes
> because ARCH_DMA_MINALIGN is 64.
>
> This allocation is called every times at file allocation(alloc_file()).
> So, the total slack memory size(allocated size - request size)
> is increased exponentially.
>
> E.g) Min Kmalloc Size : 64bytes, Unit : bytes
> Allocated Size | Request Size | Slack Size | Allocation Count
> ---
> 770048 |192512| 577536 | 12032
>
> At the result, this change reduce memory usage 42bytes per each
> file_security_struct
>
> Signed-off-by: Sangwoo
> ---
> security/selinux/hooks.c |8 ++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
Applied, thanks for the patch and the data. Sorry for the delay, it should be
in linux-next tomorrow.
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 3f8d567..c20e082 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -126,6 +126,7 @@ int selinux_enabled = 1;
> #endif
>
> static struct kmem_cache *sel_inode_cache;
> +static struct kmem_cache *file_security_cache;
>
> /**
> * selinux_secmark_enabled - Check to see if SECMARK is currently enabled
> @@ -287,7 +288,7 @@ static int file_alloc_security(struct file *file)
> struct file_security_struct *fsec;
> u32 sid = current_sid();
>
> - fsec = kzalloc(sizeof(struct file_security_struct), GFP_KERNEL);
> + fsec = kmem_cache_zalloc(file_security_cache, GFP_KERNEL);
> if (!fsec)
> return -ENOMEM;
>
> @@ -302,7 +303,7 @@ static void file_free_security(struct file *file)
> {
> struct file_security_struct *fsec = file->f_security;
> file->f_security = NULL;
> - kfree(fsec);
> + kmem_cache_free(file_security_cache, fsec);
> }
>
> static int superblock_alloc_security(struct super_block *sb)
> @@ -6086,6 +6087,9 @@ static __init int selinux_init(void)
> sel_inode_cache = kmem_cache_create("selinux_inode_security",
> sizeof(struct
> inode_security_struct),
> 0, SLAB_PANIC, NULL);
> + file_security_cache = kmem_cache_create("selinux_file_security",
> + sizeof(struct file_security_struct),
> + 0, SLAB_PANIC, NULL);
> avc_init();
>
> security_add_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks));
--
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH v3 5/5] selinux: introduce kdbus access controls
On Wednesday, October 07, 2015 07:08:48 PM Paul Moore wrote:
> +static int selinux_kdbus_conn_see_notification(const struct cred *creds)
> +{
> + return avc_has_perm(SECINITSID_KERNEL, cred_sid(creds),
> + SECCLASS_KDBUS, KDBUS__SEE_NOTIFICATION, NULL);
> +}
I'm going to flip the subj/obj ordering here, the passed credentials should be
the subject.
--
paul moore
security @ redhat
--
To unsubscribe from this list: send the line "unsubscribe
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/5] fs: Verify access of user towards block device file when mounting
On Thu, Oct 01, 2015 at 09:41:37AM -0500, Seth Forshee wrote:
> On Thu, Oct 01, 2015 at 09:40:52AM -0400, Mike Snitzer wrote:
> > On Thu, Oct 01 2015 at 8:55am -0400,
> > Seth Forshee wrote:
> >
> > > On Wed, Sep 30, 2015 at 07:42:15PM -0400, Mike Snitzer wrote:
> > > > On Wed, Sep 30 2015 at 4:15pm -0400,
> > > > Seth Forshee wrote:
> > > >
> > > > > When mounting a filesystem on a block device there is currently
> > > > > no verification that the user has appropriate access to the
> > > > > device file passed to mount. This has not been an issue so far
> > > > > since the user in question has always been root, but this must
> > > > > be changed before allowing unprivileged users to mount in user
> > > > > namespaces.
> > > > >
> > > > > To fix this, add an argument to lookup_bdev() to specify the
> > > > > required permissions. If the mask of permissions is zero, or
> > > > > if the user has CAP_SYS_ADMIN, the permission check is skipped,
> > > > > otherwise the lookup fails if the user does not have the
> > > > > specified access rights for the inode at the supplied path.
> > > > >
> > > > > Callers associated with mounting are updated to pass permission
> > > > > masks to lookup_bdev() so that these mounts will fail for an
> > > > > unprivileged user who lacks permissions for the block device
> > > > > inode. All other callers pass 0 to maintain their current
> > > > > behaviors.
> > > > >
> > > > > Signed-off-by: Seth Forshee
> > > > > ---
> > > > > drivers/md/bcache/super.c | 2 +-
> > > > > drivers/md/dm-table.c | 2 +-
> > > > > drivers/mtd/mtdsuper.c| 6 +-
> > > > > fs/block_dev.c| 18 +++---
> > > > > fs/quota/quota.c | 2 +-
> > > > > include/linux/fs.h| 2 +-
> > > > > 6 files changed, 24 insertions(+), 8 deletions(-)
> > > > >
> > > > > diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> > > > > index e76ed003769e..35bb3ea4cbe2 100644
> > > > > --- a/drivers/md/dm-table.c
> > > > > +++ b/drivers/md/dm-table.c
> > > > > @@ -380,7 +380,7 @@ int dm_get_device(struct dm_target *ti, const
> > > > > char *path, fmode_t mode,
> > > > > BUG_ON(!t);
> > > > >
> > > > > /* convert the path to a device */
> > > > > - bdev = lookup_bdev(path);
> > > > > + bdev = lookup_bdev(path, 0);
> > > > > if (IS_ERR(bdev)) {
> > > > > dev = name_to_dev_t(path);
> > > > > if (!dev)
> > > >
> > > > Given dm_get_device() is passed @mode why not have it do something like
> > > > you did in blkdev_get_by_path()? e.g.:
> > >
> > > I only dealt with code related to mounting in this patch since that's
> > > what I'm working on. I have it on my TODO list to consider converting
> > > other callers of lookup_bdev. But if you're sure doing so makes sense
> > > for dm_get_device and that it won't cause regressions then I could add a
> > > patch for it.
> >
> > OK, dm_get_device() is called in DM device activation path (by tools
> > like lvm2).
> >
> > After lookup_bdev() it goes on to call blkdev_get_by_dev() with this
> > call chain:
> > dm_get_device -> dm_get_table_device -> open_table_device ->
> > blkdev_get_by_dev
> >
> > Not immediately clear to me why we'd need to augment blkdev_get_by_dev()
> > to do this checking also.
> >
> > However, thinking further: In a device stack (e.g. dm/lvm2, md, etc)
> > new virtual block devices are created that layer ontop of the
> > traditional block devices. This level of indirection may cause your
> > lookup_bdev() check to go on to succeed (if access constraints were not
> > established on the upper level dm or md device?). I'm just thinking
> > outloud here: but have you verified your changes work as intended on
> > devices created with either lvm2 or mdadm?
> >
> > What layer establishes access rights to historically root-only
> > priviledged block devices? Is it user namespaces?
>
> I'm going to start with this last question and work my way backwards.
>
> Who determines access rights to the block devices varies to some degree.
> Any normal user could unshare the user/mount namespaces and still see
> all the block devices in /dev. If we're going to allow that user to then
> mount block devices in their private namespaces, the kernel must verify
> that the user has appropriate permissions for the block device inode.
> That's the point of this patch. In a container context the host process
> which sets up the container might share some block devices into the
> container via bind mounts, in which case it would be responsible for
> setting up access rights (both via inode permissions and potentially via
> devcgroups). I also have some patches I'm working on for a loop psuedo
> filesystem which would allow a container to allocate loop devices for
> its own use (via the loop-control device), in which case the kernel
> creates a device node in the loopfs filesystem from which the new loop
> device was allocated, owned by the root user in the
Re: [PATCH] af_unix: introduce unix_sk_const helper
On Thursday 08 October 2015 04:03:32 David Miller wrote:
> From: Arnd Bergmann
> Date: Tue, 06 Oct 2015 22:52:46 +0200
>
> > Commit 124613012db1 ("af_unix: Convert the unix_sk macro to an inline
> > function for type safety") was recently added to catch incorrect
> > uses of the unix_sk helper using compiler warnings.
> >
> > It has now caught one such case in lsm_audit.c. The code is technically
> > correct, but as it converts a const pointer to a non-const pointer,
> > the annotation got lost, which gcc now warns about.
> >
> > This patch avoids the warning by introducing an additional helper
> > that has const input and output, which makes the lsm_audit code build
> > cleanly again.
> >
> > Signed-off-by: Arnd Bergmann
> > ---
> > I'm not entirely happy with this workaround myself, but could not come
> > up with a better one.
>
> You can make the argument unconditionally const, as Paul Moore has done
> in a separate patch submission.
Ok, I see now how Paul's "audit: constify parts of common_audit_data and
lsm_network_audit" patch caused the problem and is now gone from linux-next.
That seems nicer indeed.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] af_unix: introduce unix_sk_const helper
From: Arnd Bergmann
Date: Tue, 06 Oct 2015 22:52:46 +0200
> Commit 124613012db1 ("af_unix: Convert the unix_sk macro to an inline
> function for type safety") was recently added to catch incorrect
> uses of the unix_sk helper using compiler warnings.
>
> It has now caught one such case in lsm_audit.c. The code is technically
> correct, but as it converts a const pointer to a non-const pointer,
> the annotation got lost, which gcc now warns about.
>
> This patch avoids the warning by introducing an additional helper
> that has const input and output, which makes the lsm_audit code build
> cleanly again.
>
> Signed-off-by: Arnd Bergmann
> ---
> I'm not entirely happy with this workaround myself, but could not come
> up with a better one.
You can make the argument unconditionally const, as Paul Moore has done
in a separate patch submission.
--
To unsubscribe from this list: send the line "unsubscribe
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
