Re: [PATCH 1/2] ext4: use XATTR_*_PREFIX_LEN instead sizeof(...)

[José Bollo]

I prefer the use of sizeof that can't be faked even by error but why not

Le dimanche 03 janvier 2016 à 20:56 +0100, Toralf Förster a écrit :
> use the definition in include/uapi/linux/xattr.h
> 
> Signed-off-by: Toralf Förster 
> ---
>  fs/ext4/xattr_security.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/xattr_security.c b/fs/ext4/xattr_security.c
> index 36f4c1a..1a3d629 100644
> --- a/fs/ext4/xattr_security.c
> +++ b/fs/ext4/xattr_security.c
> @@ -16,7 +16,7 @@ ext4_xattr_security_list(const struct xattr_handler 
> *handler,
>struct dentry *dentry, char *list, size_t list_size,
>const char *name, size_t name_len)
>  {
> - const size_t prefix_len = sizeof(XATTR_SECURITY_PREFIX)-1;
> + const size_t prefix_len = XATTR_SECURITY_PREFIX_LEN;
>   const size_t total_len = prefix_len + name_len + 1;
>  
> 


--
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] X.509: Don't check the signature on apparently self-signed keys [ver #2]

[David Howells]

Mimi Zohar  wrote:

> The x509_validate_trust() was originally added for IMA to ensure, on a
> secure boot system, a certificate chain of trust rooted in hardware.
> The IMA MOK keyring extends this certificate chain of trust to the
> running system.

The problem is that because 'trusted' is a boolean, a key in the IMA MOK
keyring will permit addition to the system keyring.

David
--
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

[PATCH] X.509: Partially revert patch to add validation against IMA MOK keyring

[David Howells]

Partially revert commit 41c89b64d7184a780f12f2cccdabe65cb2408893:

Author: Petko Manolov 
Date:   Wed Dec 2 17:47:55 2015 +0200
IMA: create machine owner and blacklist keyrings

The problem is that prep->trusted is a simple boolean and the additional
x509_validate_trust() call doesn't therefore distinguish levels of
trustedness, but is just OR'd with the result of validation against the
system trusted keyring.

However, setting the trusted flag means that this key may be added to *any*
trusted-only keyring - including the system trusted keyring.

Whilst I appreciate what the patch is trying to do, I don't think this is
quite the right solution.

Signed-off-by: David Howells 
cc: Petko Manolov 
cc: Mimi Zohar 
cc: keyri...@vger.kernel.org
---

 crypto/asymmetric_keys/x509_public_key.c |2 --
 1 file changed, 2 deletions(-)

diff --git a/crypto/asymmetric_keys/x509_public_key.c 
b/crypto/asymmetric_keys/x509_public_key.c
index 9e9e5a6a9ed6..2a44b3752471 100644
--- a/crypto/asymmetric_keys/x509_public_key.c
+++ b/crypto/asymmetric_keys/x509_public_key.c
@@ -321,8 +321,6 @@ static int x509_key_preparse(struct key_preparsed_payload 
*prep)
goto error_free_cert;
} else if (!prep->trusted) {
ret = x509_validate_trust(cert, get_system_trusted_keyring());
-   if (ret)
-   ret = x509_validate_trust(cert, get_ima_mok_keyring());
if (!ret)
prep->trusted = 1;
}

--
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] X.509: Don't check the signature on apparently self-signed keys [ver #2]

[Mimi Zohar]

On Wed, 2016-01-06 at 13:21 +, David Howells wrote:
> Mimi Zohar  wrote:
> 
> > The x509_validate_trust() was originally added for IMA to ensure, on a
> > secure boot system, a certificate chain of trust rooted in hardware.
> > The IMA MOK keyring extends this certificate chain of trust to the
> > running system.
> 
> The problem is that because 'trusted' is a boolean, a key in the IMA MOK
> keyring will permit addition to the system keyring.

Once the builtin keys are loaded onto the system keyring, isn't the
system keyring locked?  Or is this the only mechanism used for locking?

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: [RFC PATCH] X.509: Don't check the signature on apparently self-signed keys [ver #2]

[David Howells]

Mimi Zohar  wrote:

> Once the builtin keys are loaded onto the system keyring, isn't the
> system keyring locked?

No.

David
--
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] X.509: Don't check the signature on apparently self-signed keys [ver #2]

[Mimi Zohar]

On Tue, 2016-01-05 at 16:39 +, David Howells wrote:
> Mimi Zohar  wrote:
> 
> > You're missing Petko's patch:
> > 41c89b6 IMA: create machine owner and blacklist keyrings
> 
> Hmmm...  This is wrong.  x509_key_preparse() shouldn't be polling the IMA MOK
> keyring under all circumstances.

The x509_validate_trust() was originally added for IMA to ensure, on a
secure boot system, a certificate chain of trust rooted in hardware.
The IMA MOK keyring extends this certificate chain of trust to the
running system.

The IMA MOK keyring is a build configuration option that defaults to
off.

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: [RFC PATCH] X.509: Don't check the signature on apparently self-signed keys [ver #2]

[Petko Manolov]

On 16-01-06 13:21:27, David Howells wrote:
> Mimi Zohar  wrote:
> 
> > The x509_validate_trust() was originally added for IMA to ensure, on a 
> > secure boot system, a certificate chain of trust rooted in hardware. The 
> > IMA 
> > MOK keyring extends this certificate chain of trust to the running system.
> 
> The problem is that because 'trusted' is a boolean, a key in the IMA MOK 
> keyring will permit addition to the system keyring.

If this is true the i am clearly doing the wrong thing.  The CA hierarchy 
should 
run top-bottom, not the other way around.

IMA MOK was introduced mainly because .system keyring was static at the time.  
Assuming i have my root certificate in .system how can i add more keys to this 
keyring?  The new keys have been signed by my root CA?  Is this possible since 
your October patch-set or i've been missing something this whole time?


Petko
--
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: [PATCHv1 6/6] rdmacg: Added documentation for rdma controller.

[Parav Pandit]

On Thu, Jan 7, 2016 at 4:27 AM, Tejun Heo  wrote:
> Hello,
>
> On Thu, Jan 07, 2016 at 04:14:26AM +0530, Parav Pandit wrote:
>> Yes. I read through. I can see two changes to be made in V2 version of
>> this patch.
>> 1. rdma.resource.verb.usage and rdma.resource.verb.limit to change
>> respectively to,
>> 2. rdma.resource.verb.stat and rdma.resource.verb.max.
>> 3. rdma.resource.verb.failcnt indicate failure events, which I think
>> should go to events.
>
> What's up with the ".resource" part?

I can remove "resource" key word. If just that if something other than
resource comes up to limit to in future, it will be hard to define at
that time.

> Also can't the .max file list
> the available resources?  Why does it need a separtae list file?
>
max file does lists them only after limits are configured for that
device. Thats when rpool (array of max and usage counts) is allocated.

If user wants to know what all knobs are available, than list file
exposes them on per device basis without actually mentioning actual
limit or without allocating rpool arrays.

If you are hinting that I should allocate rpool array when rdma cgroup
is created, that can be done for already discovered devices.
If new devices are discovered after cgroup is created, for them we
anyway have to allocate/free when they appear/disappear.

In different implementation, where list of all the rdma cgroups can be
maintained, and rpool arrays can be allocated for all of them when new
device appear/disappear. This can move complexity of dynamic
allocation from try_charge/uncharge to device addition and removal
APIs. ib_register_ib_device() level.
However this comes with memory cost, where even if those device doesnt
participate in cgroup, for them rpool memory will be allocated for
each such rdma cgroup.

list file looks like below for two device entries.
mlx4_0 ah qp mr pd srq flow
ocrdma0 ah qp mr pd

max file looks like below.
mlx4_0 ah=100 qp=40 mr=10 pd=90 srq=10 flow=10


>> I roll out new patch for events post this patch as additional feature
>> and remove this feature in V2.
>>
>> rdma.resource.verb.list file is unique to rdma cgroup, so I believe
>> this is fine.
>
> Please see above.
>
>> We will conclude whether to have rdma.resource.hw. or not in
>> other patches.
>> I am in opinion to keep "resource" and "verb" or "hw" tags around to
>> keep it verbose enough to know what are we trying to control.
>
> What does that achieve?  I feel that it's getting overengineered
> constantly.

Please see above for "resource". I guess we are not loosing anything
by having "rdma.resource" vs just having "rdma".
But if that sounds too much, we can remove "resource".

>
> Thanks.
>
> --
> tejun
--
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: [PATCHv1 6/6] rdmacg: Added documentation for rdma controller.

[Parav Pandit]

On Wed, Jan 6, 2016 at 3:23 AM, Tejun Heo  wrote:
> Hello,
>
> On Wed, Jan 06, 2016 at 12:28:06AM +0530, Parav Pandit wrote:
>> +5-4-1. RDMA Interface Files
>> +
>> +  rdma.resource.verb.list
>> +  rdma.resource.verb.limit
>> +  rdma.resource.verb.usage
>> +  rdma.resource.verb.failcnt
>> +  rdma.resource.hw.list
>> +  rdma.resource.hw.limit
>> +  rdma.resource.hw.usage
>> +  rdma.resource.hw.failcnt
>
> Can you please read the rest of cgroup.txt and put the interface in
> line with the common conventions followed by other controllers?
>

Yes. I read through. I can see two changes to be made in V2 version of
this patch.
1. rdma.resource.verb.usage and rdma.resource.verb.limit to change
respectively to,
2. rdma.resource.verb.stat and rdma.resource.verb.max.
3. rdma.resource.verb.failcnt indicate failure events, which I think
should go to events.
I roll out new patch for events post this patch as additional feature
and remove this feature in V2.

rdma.resource.verb.list file is unique to rdma cgroup, so I believe
this is fine.

We will conclude whether to have rdma.resource.hw. or not in
other patches.
I am in opinion to keep "resource" and "verb" or "hw" tags around to
keep it verbose enough to know what are we trying to control.

Is that ok?

> Thanks.
>
> --
> tejun
--
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: [PATCHv1 3/6] rdmacg: implements rdma cgroup

[Parav Pandit]

On Wed, Jan 6, 2016 at 3:31 AM, Tejun Heo  wrote:
> Hello,
>
> On Wed, Jan 06, 2016 at 12:28:03AM +0530, Parav Pandit wrote:
>> +/* hash table to keep map of tgid to owner cgroup */
>> +DEFINE_HASHTABLE(pid_cg_map_tbl, 7);
>> +DEFINE_SPINLOCK(pid_cg_map_lock);/* lock to protect hash table access */
>> +
>> +/* Keeps mapping of pid to its owning cgroup at rdma level,
>> + * This mapping doesn't change, even if process migrates from one to other
>> + * rdma cgroup.
>> + */
>> +struct pid_cg_map {
>> + struct pid *pid;/* hash key */
>> + struct rdma_cgroup *cg;
>> +
>> + struct hlist_node hlist;/* pid to cgroup hash table link */
>> + atomic_t refcnt;/* count active user tasks to figure 
>> out
>> +  * when to free the memory
>> +  */
>> +};
>
> Ugh, there's something clearly wrong here.  Why does the rdma
> controller need to keep track of pid cgroup membership?
>
Rdma resource can be allocated by parent process, used and freed by
child process.
Child process could belong to different rdma cgroup.
Parent process might have been terminated after creation of rdma
cgroup. (Followed by cgroup might have been deleted too).
Its discussed in https://lkml.org/lkml/2015/11/2/307

In nutshell, there is process that clearly owns the rdma resource.
So to keep the design simple, rdma resource is owned by the creator
process and cgroup without modifying the task_struct.

>> +static void _dealloc_cg_rpool(struct rdma_cgroup *cg,
>> +   struct cg_resource_pool *rpool)
>> +{
>> + spin_lock(>cg_list_lock);
>> +
>> + /* if its started getting used by other task,
>> +  * before we take the spin lock, then skip,
>> +  * freeing it.
>> +  */
>
> Please follow CodingStyle.
>
>> + if (atomic_read(>refcnt) == 0) {
>> + list_del_init(>cg_list);
>> + spin_unlock(>cg_list_lock);
>> +
>> + _free_cg_rpool(rpool);
>> + return;
>> + }
>> + spin_unlock(>cg_list_lock);
>> +}
>> +
>> +static void dealloc_cg_rpool(struct rdma_cgroup *cg,
>> +  struct cg_resource_pool *rpool)
>> +{
>> + /* Don't free the resource pool which is created by the
>> +  * user, otherwise we miss the configured limits. We don't
>> +  * gain much either by splitting storage of limit and usage.
>> +  * So keep it around until user deletes the limits.
>> +  */
>> + if (atomic_read(>creator) == RDMACG_RPOOL_CREATOR_DEFAULT)
>> + _dealloc_cg_rpool(cg, rpool);
>
> I'm pretty sure you can get away with an fixed length array of
> counters.  Please keep it simple.  It's a simple hard limit enforcer.
> There's no need to create a massive dynamic infrastrucure.
>
Every resource pool for verbs resource is fixed length array. Length
of the array is defined by the IB stack modules.
This array is per cgroup, per device.
Its per device, because we agreed that we want to address requirement
of controlling/configuring them on per device basis.
Devices appear and disappear. Therefore they are allocated dynamically.
Otherwise this array could be static in cgroup structure.



> Thanks.
>
> --
> tejun
--
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] X.509: Partially revert patch to add validation against IMA MOK keyring

[David Howells]

David Howells  wrote:

> Partially revert commit 41c89b64d7184a780f12f2cccdabe65cb2408893:
> 
>   Author: Petko Manolov 
>   Date:   Wed Dec 2 17:47:55 2015 +0200
>   IMA: create machine owner and blacklist keyrings
> 
> The problem is that prep->trusted is a simple boolean and the additional
> x509_validate_trust() call doesn't therefore distinguish levels of
> trustedness, but is just OR'd with the result of validation against the
> system trusted keyring.
> 
> However, setting the trusted flag means that this key may be added to *any*
> trusted-only keyring - including the system trusted keyring.
> 
> Whilst I appreciate what the patch is trying to do, I don't think this is
> quite the right solution.

Please apply this to security/next.

Thanks,
David
--
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] X.509: Partially revert patch to add validation against IMA MOK keyring

[Mimi Zohar]

On Thu, 2016-01-07 at 00:34 +, David Howells wrote:
> David Howells  wrote:
> 
> > Partially revert commit 41c89b64d7184a780f12f2cccdabe65cb2408893:
> > 
> > Author: Petko Manolov 
> > Date:   Wed Dec 2 17:47:55 2015 +0200
> > IMA: create machine owner and blacklist keyrings
> > 
> > The problem is that prep->trusted is a simple boolean and the additional
> > x509_validate_trust() call doesn't therefore distinguish levels of
> > trustedness, but is just OR'd with the result of validation against the
> > system trusted keyring.
> > 
> > However, setting the trusted flag means that this key may be added to *any*
> > trusted-only keyring - including the system trusted keyring.

Hm, I'm not able to add a key to the system keyring that is signed by a
key on either the system or the IMA MOK keyrings.  The system keyring
seems to be "locked".  A key that is signed by either a key on the
system or the IMA MOK keyring can be added to the IMA keyring.

keyctl show %keyring:.system_keyring
Keyring
 973688077 ---lswrv  0 0  keyring: .system_keyring

evmctl import m1-cert-signed.der 973688077
add_key failed
errno: Permission denied (13)

Mimi

> > Whilst I appreciate what the patch is trying to do, I don't think this is
> > quite the right solution.

> Please apply this to security/next.
> 
> Thanks,
> David


--
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] selinux: Inode label revalidation performance fix

[Stephen Smalley]

On 01/05/2016 05:12 PM, Andreas Gruenbacher wrote:

Commit 5d226df4 has introduced a performance regression of about
10% in the UnixBench pipe benchmark.  It turns out that the call
to inode_security in selinux_file_permission can be moved below
the zero-mask test and that inode_security_revalidate can be
removed entirely, which brings us back to roughly the original
performance.

Signed-off-by: Andreas Gruenbacher 


Acked-by:  Stephen Smalley 


---
  security/selinux/hooks.c | 10 ++
  1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 40e071a..f8110cf 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -273,11 +273,6 @@ static int __inode_security_revalidate(struct inode *inode,
return 0;
  }

-static void inode_security_revalidate(struct inode *inode)
-{
-   __inode_security_revalidate(inode, NULL, true);
-}
-
  static struct inode_security_struct *inode_security_novalidate(struct inode 
*inode)
  {
return inode->i_security;
@@ -3277,19 +3272,19 @@ static int selinux_file_permission(struct file *file, 
int mask)
  {
struct inode *inode = file_inode(file);
struct file_security_struct *fsec = file->f_security;
-   struct inode_security_struct *isec = inode_security(inode);
+   struct inode_security_struct *isec;
u32 sid = current_sid();

if (!mask)
/* No permission to check.  Existence test. */
return 0;

+   isec = inode_security(inode);
if (sid == fsec->sid && fsec->isid == isec->sid &&
fsec->pseqno == avc_policy_seqno())
/* No change since file_open check. */
return 0;

-   inode_security_revalidate(inode);
return selinux_revalidate_file_permission(file, mask);
  }

@@ -3595,7 +3590,6 @@ static int selinux_file_open(struct file *file, const 
struct cred *cred)
 * new inode label or new policy.
 * This check is not redundant - do not remove.
 */
-   inode_security_revalidate(file_inode(file));
return file_path_has_perm(cred, file, open_file_to_av(file));
  }




--
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