* [PATCH 1/3] Smack: fix behavior of smack_inode_listsecurity
@ 2014-08-07 16:52 Konstantin Khlebnikov
2014-08-07 16:52 ` [PATCH 2/3] Smack: handle zero-length security labels without panic Konstantin Khlebnikov
` (4 more replies)
0 siblings, 5 replies; 13+ messages in thread
From: Konstantin Khlebnikov @ 2014-08-07 16:52 UTC (permalink / raw)
To: Casey Schaufler
Cc: linux-security-module, linux-kernel, James Morris, Serge E. Hallyn
Security operation ->inode_listsecurity is used for generating list of
available extended attributes for syscall listxattr. Currently it's used
only in nfs4 or if filesystem doesn't provide i_op->listxattr.
The list is the set of NULL-terminated names, one after the other.
This method must include zero byte at the and into result.
Also this function must return length even if string does not fit into
output buffer or it is NULL, see similar method in selinux and man listxattr.
Signed-off-by: Konstantin Khlebnikov <k.khlebnikov@samsung.com>
---
security/smack/smack_lsm.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index e6ab307..b11ab23 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -1122,13 +1122,12 @@ static int smack_inode_getsecurity(const struct inode *inode,
static int smack_inode_listsecurity(struct inode *inode, char *buffer,
size_t buffer_size)
{
- int len = strlen(XATTR_NAME_SMACK);
+ int len = sizeof(XATTR_NAME_SMACK);
- if (buffer != NULL && len <= buffer_size) {
+ if (buffer != NULL && len <= buffer_size)
memcpy(buffer, XATTR_NAME_SMACK, len);
- return len;
- }
- return -EINVAL;
+
+ return len;
}
/**
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/3] Smack: handle zero-length security labels without panic
2014-08-07 16:52 [PATCH 1/3] Smack: fix behavior of smack_inode_listsecurity Konstantin Khlebnikov
@ 2014-08-07 16:52 ` Konstantin Khlebnikov
2014-08-08 20:25 ` Serge E. Hallyn
2014-08-08 22:05 ` Casey Schaufler
2014-08-07 16:52 ` [PATCH 3/3] Smack: remove unneeded NULL-termination from securtity label Konstantin Khlebnikov
` (3 subsequent siblings)
4 siblings, 2 replies; 13+ messages in thread
From: Konstantin Khlebnikov @ 2014-08-07 16:52 UTC (permalink / raw)
To: Casey Schaufler
Cc: linux-security-module, linux-kernel, James Morris, Serge E. Hallyn
Zero-length security labels are invalid but kernel should handle them.
This patch fixes kernel panic after setting zero-length security labels:
# attr -S -s "SMACK64" -V "" file
And after writing zero-length string into smackfs files syslog and onlycp:
# python -c 'import os; os.write(1, "")' > /smack/syslog
The problem is caused by brain-damaged logic in function smk_parse_smack()
which takes pointer to buffer and its length but if length below or equal zero
it thinks that the buffer is zero-terminated. Unfortunately callers of this
function are widely used and proper fix requires serious refactoring.
Signed-off-by: Konstantin Khlebnikov <k.khlebnikov@samsung.com>
---
Example:
[ 28.063935] BUG: unable to handle kernel NULL pointer dereference at (null)
[ 28.064623] IP: [<ffffffff812ea780>] strlen+0x0/0x30
[ 28.064623] PGD 29a77067 PUD 2972d067 PMD 0
[ 28.064623] Oops: 0000 [#1] SMP
[ 28.064623] Modules linked in:
[ 28.064623] CPU: 0 PID: 824 Comm: attr Not tainted 3.16.0+ #6
[ 28.064623] task: ffff880029bd88c0 ti: ffff8800297d0000 task.ti: ffff8800297d0000
[ 28.064623] RIP: 0010:[<ffffffff812ea780>] [<ffffffff812ea780>] strlen+0x0/0x30
[ 28.064623] RSP: 0018:ffff8800297d3ca8 EFLAGS: 00010246
[ 28.064623] RAX: 0000000000000001 RBX: ffff880029c096c0 RCX: 0000000000000001
[ 28.064623] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
[ 28.064623] RBP: ffff8800297d3cd0 R08: ffffffff81e3d940 R09: ffff88002a3e8588
[ 28.064623] R10: 8080808080808080 R11: 0000000000000000 R12: 0000000000000000
[ 28.064623] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
[ 28.064623] FS: 00007effe391a700(0000) GS:ffff88002a400000(0000) knlGS:0000000000000000
[ 28.064623] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 28.064623] CR2: 0000000000000000 CR3: 00000000297a1000 CR4: 00000000001406b0
[ 28.064623] Stack:
[ 28.064623] ffffffff812a6fa5 ffff880029c096c0 0000000000000001 0000000000000000
[ 28.064623] 0000000000000000 ffff8800297d3cf8 ffffffff812a70ae ffff880029c096c0
[ 28.064623] 0000000000000001 0000000000000000 ffff8800297d3d88 ffffffff812a64dc
[ 28.064623] Call Trace:
[ 28.064623] [<ffffffff812a6fa5>] ? smk_parse_smack+0xe5/0x110
[ 28.064623] [<ffffffff812a70ae>] smk_import_entry+0xe/0x130
[ 28.064623] [<ffffffff812a64dc>] smack_inode_setxattr+0x11c/0x280
[ 28.064623] [<ffffffff8116a767>] ? lookup_fast+0x177/0x2e0
[ 28.064623] [<ffffffff81177001>] ? dput+0x21/0x1a0
[ 28.064623] [<ffffffff812a0eeb>] security_inode_setxattr+0x1b/0x30
[ 28.064623] [<ffffffff8118488f>] vfs_setxattr+0x6f/0xb0
[ 28.064623] [<ffffffff81184a76>] setxattr+0x1a6/0x1f0
[ 28.064623] [<ffffffff8116af6d>] ? final_putname+0x1d/0x40
[ 28.064623] [<ffffffff8116b1c4>] ? putname+0x24/0x40
[ 28.064623] [<ffffffff8117144a>] ? user_path_at_empty+0x5a/0xa0
[ 28.064623] [<ffffffff81162b84>] ? __sb_start_write+0x44/0xe0
[ 28.064623] [<ffffffff8113cd21>] ? do_brk+0x241/0x320
[ 28.064623] [<ffffffff81184d97>] SyS_lsetxattr+0x87/0xe0
[ 28.064623] [<ffffffff81806f69>] system_call_fastpath+0x16/0x1b
[ 28.064623] Code: 89 f8 48 89 e5 f6 82 40 3e a6 81 20 74 15 0f 1f 44 00 00 48 83 c0 01 0f b6 10 f6 82 40 3e a6 81 20 75 f0 5d c3 66 0f 1f 44 00 00 <80> 3f 00 55 48 89 e5 74 15 48 89 f8 0f 1f 40 00 48 83 c0 01 80
[ 28.064623] RIP [<ffffffff812ea780>] strlen+0x0/0x30
[ 28.064623] RSP <ffff8800297d3ca8>
[ 28.064623] CR2: 0000000000000000
[ 28.093561] ---[ end trace de1055429a98a5be ]---
---
security/smack/smack_lsm.c | 2 +-
security/smack/smackfs.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index b11ab23..afa5ad0 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -923,7 +923,7 @@ static int smack_inode_setxattr(struct dentry *dentry, const char *name,
rc = -EPERM;
if (rc == 0 && check_import) {
- skp = smk_import_entry(value, size);
+ skp = size ? smk_import_entry(value, size) : NULL;
if (skp == NULL || (check_star &&
(skp == &smack_known_star || skp == &smack_known_web)))
rc = -EINVAL;
diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
index 3c720ff..56a1439 100644
--- a/security/smack/smackfs.c
+++ b/security/smack/smackfs.c
@@ -1677,7 +1677,7 @@ static ssize_t smk_write_onlycap(struct file *file, const char __user *buf,
if (smack_onlycap != NULL && smack_onlycap != skp)
return -EPERM;
- data = kzalloc(count, GFP_KERNEL);
+ data = kzalloc(count + 1, GFP_KERNEL);
if (data == NULL)
return -ENOMEM;
@@ -2228,7 +2228,7 @@ static ssize_t smk_write_syslog(struct file *file, const char __user *buf,
if (!smack_privileged(CAP_MAC_ADMIN))
return -EPERM;
- data = kzalloc(count, GFP_KERNEL);
+ data = kzalloc(count + 1, GFP_KERNEL);
if (data == NULL)
return -ENOMEM;
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 3/3] Smack: remove unneeded NULL-termination from securtity label
2014-08-07 16:52 [PATCH 1/3] Smack: fix behavior of smack_inode_listsecurity Konstantin Khlebnikov
2014-08-07 16:52 ` [PATCH 2/3] Smack: handle zero-length security labels without panic Konstantin Khlebnikov
@ 2014-08-07 16:52 ` Konstantin Khlebnikov
2014-08-08 20:51 ` Serge E. Hallyn
2014-08-08 22:05 ` Casey Schaufler
2014-08-08 20:27 ` [PATCH 1/3] Smack: fix behavior of smack_inode_listsecurity Serge E. Hallyn
` (2 subsequent siblings)
4 siblings, 2 replies; 13+ messages in thread
From: Konstantin Khlebnikov @ 2014-08-07 16:52 UTC (permalink / raw)
To: Casey Schaufler
Cc: linux-security-module, linux-kernel, James Morris, Serge E. Hallyn
Values of extended attributes are stored as binary blobs. NULL-termination
of them isn't required. It just wastes disk space and confuses command-line
tools like getfattr because they have to print that zero byte at the end.
This patch removes terminating zero byte from initial security label in
smack_inode_init_security and cuts it out in function smack_inode_getsecurity
which is used by syscall getxattr. This change seems completely safe, because
function smk_parse_smack ignores everything after first zero byte.
Signed-off-by: Konstantin Khlebnikov <k.khlebnikov@samsung.com>
---
security/smack/smack_lsm.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index afa5ad0..16ae853 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -672,7 +672,7 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
}
if (len)
- *len = strlen(isp) + 1;
+ *len = strlen(isp);
return 0;
}
@@ -1076,7 +1076,7 @@ static int smack_inode_getsecurity(const struct inode *inode,
if (strcmp(name, XATTR_SMACK_SUFFIX) == 0) {
isp = smk_of_inode(inode);
- ilen = strlen(isp) + 1;
+ ilen = strlen(isp);
*buffer = isp;
return ilen;
}
@@ -1101,7 +1101,7 @@ static int smack_inode_getsecurity(const struct inode *inode,
else
return -EOPNOTSUPP;
- ilen = strlen(isp) + 1;
+ ilen = strlen(isp);
if (rc == 0) {
*buffer = isp;
rc = ilen;
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] Smack: handle zero-length security labels without panic
2014-08-07 16:52 ` [PATCH 2/3] Smack: handle zero-length security labels without panic Konstantin Khlebnikov
@ 2014-08-08 20:25 ` Serge E. Hallyn
2014-08-08 20:48 ` Konstantin Khlebnikov
2014-08-08 22:05 ` Casey Schaufler
1 sibling, 1 reply; 13+ messages in thread
From: Serge E. Hallyn @ 2014-08-08 20:25 UTC (permalink / raw)
To: Konstantin Khlebnikov
Cc: Casey Schaufler, linux-security-module, linux-kernel,
James Morris, Serge E. Hallyn
Quoting Konstantin Khlebnikov (k.khlebnikov@samsung.com):
> Zero-length security labels are invalid but kernel should handle them.
>
> This patch fixes kernel panic after setting zero-length security labels:
> # attr -S -s "SMACK64" -V "" file
>
> And after writing zero-length string into smackfs files syslog and onlycp:
> # python -c 'import os; os.write(1, "")' > /smack/syslog
>
> The problem is caused by brain-damaged logic in function smk_parse_smack()
> which takes pointer to buffer and its length but if length below or equal zero
> it thinks that the buffer is zero-terminated. Unfortunately callers of this
> function are widely used and proper fix requires serious refactoring.
>
> Signed-off-by: Konstantin Khlebnikov <k.khlebnikov@samsung.com>
Acked-by: Serge Hallyn <serge.hallyn@canonical.com>
>
> ---
>
> Example:
>
> [ 28.063935] BUG: unable to handle kernel NULL pointer dereference at (null)
> [ 28.064623] IP: [<ffffffff812ea780>] strlen+0x0/0x30
> [ 28.064623] PGD 29a77067 PUD 2972d067 PMD 0
> [ 28.064623] Oops: 0000 [#1] SMP
> [ 28.064623] Modules linked in:
> [ 28.064623] CPU: 0 PID: 824 Comm: attr Not tainted 3.16.0+ #6
> [ 28.064623] task: ffff880029bd88c0 ti: ffff8800297d0000 task.ti: ffff8800297d0000
> [ 28.064623] RIP: 0010:[<ffffffff812ea780>] [<ffffffff812ea780>] strlen+0x0/0x30
> [ 28.064623] RSP: 0018:ffff8800297d3ca8 EFLAGS: 00010246
> [ 28.064623] RAX: 0000000000000001 RBX: ffff880029c096c0 RCX: 0000000000000001
> [ 28.064623] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
> [ 28.064623] RBP: ffff8800297d3cd0 R08: ffffffff81e3d940 R09: ffff88002a3e8588
> [ 28.064623] R10: 8080808080808080 R11: 0000000000000000 R12: 0000000000000000
> [ 28.064623] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
> [ 28.064623] FS: 00007effe391a700(0000) GS:ffff88002a400000(0000) knlGS:0000000000000000
> [ 28.064623] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 28.064623] CR2: 0000000000000000 CR3: 00000000297a1000 CR4: 00000000001406b0
> [ 28.064623] Stack:
> [ 28.064623] ffffffff812a6fa5 ffff880029c096c0 0000000000000001 0000000000000000
> [ 28.064623] 0000000000000000 ffff8800297d3cf8 ffffffff812a70ae ffff880029c096c0
> [ 28.064623] 0000000000000001 0000000000000000 ffff8800297d3d88 ffffffff812a64dc
> [ 28.064623] Call Trace:
> [ 28.064623] [<ffffffff812a6fa5>] ? smk_parse_smack+0xe5/0x110
> [ 28.064623] [<ffffffff812a70ae>] smk_import_entry+0xe/0x130
> [ 28.064623] [<ffffffff812a64dc>] smack_inode_setxattr+0x11c/0x280
> [ 28.064623] [<ffffffff8116a767>] ? lookup_fast+0x177/0x2e0
> [ 28.064623] [<ffffffff81177001>] ? dput+0x21/0x1a0
> [ 28.064623] [<ffffffff812a0eeb>] security_inode_setxattr+0x1b/0x30
> [ 28.064623] [<ffffffff8118488f>] vfs_setxattr+0x6f/0xb0
> [ 28.064623] [<ffffffff81184a76>] setxattr+0x1a6/0x1f0
> [ 28.064623] [<ffffffff8116af6d>] ? final_putname+0x1d/0x40
> [ 28.064623] [<ffffffff8116b1c4>] ? putname+0x24/0x40
> [ 28.064623] [<ffffffff8117144a>] ? user_path_at_empty+0x5a/0xa0
> [ 28.064623] [<ffffffff81162b84>] ? __sb_start_write+0x44/0xe0
> [ 28.064623] [<ffffffff8113cd21>] ? do_brk+0x241/0x320
> [ 28.064623] [<ffffffff81184d97>] SyS_lsetxattr+0x87/0xe0
> [ 28.064623] [<ffffffff81806f69>] system_call_fastpath+0x16/0x1b
> [ 28.064623] Code: 89 f8 48 89 e5 f6 82 40 3e a6 81 20 74 15 0f 1f 44 00 00 48 83 c0 01 0f b6 10 f6 82 40 3e a6 81 20 75 f0 5d c3 66 0f 1f 44 00 00 <80> 3f 00 55 48 89 e5 74 15 48 89 f8 0f 1f 40 00 48 83 c0 01 80
> [ 28.064623] RIP [<ffffffff812ea780>] strlen+0x0/0x30
> [ 28.064623] RSP <ffff8800297d3ca8>
> [ 28.064623] CR2: 0000000000000000
> [ 28.093561] ---[ end trace de1055429a98a5be ]---
> ---
> security/smack/smack_lsm.c | 2 +-
> security/smack/smackfs.c | 4 ++--
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index b11ab23..afa5ad0 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -923,7 +923,7 @@ static int smack_inode_setxattr(struct dentry *dentry, const char *name,
> rc = -EPERM;
>
> if (rc == 0 && check_import) {
> - skp = smk_import_entry(value, size);
> + skp = size ? smk_import_entry(value, size) : NULL;
> if (skp == NULL || (check_star &&
> (skp == &smack_known_star || skp == &smack_known_web)))
> rc = -EINVAL;
> diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
> index 3c720ff..56a1439 100644
> --- a/security/smack/smackfs.c
> +++ b/security/smack/smackfs.c
> @@ -1677,7 +1677,7 @@ static ssize_t smk_write_onlycap(struct file *file, const char __user *buf,
> if (smack_onlycap != NULL && smack_onlycap != skp)
> return -EPERM;
>
> - data = kzalloc(count, GFP_KERNEL);
> + data = kzalloc(count + 1, GFP_KERNEL);
> if (data == NULL)
> return -ENOMEM;
>
> @@ -2228,7 +2228,7 @@ static ssize_t smk_write_syslog(struct file *file, const char __user *buf,
> if (!smack_privileged(CAP_MAC_ADMIN))
> return -EPERM;
>
How about putting a comment here explaining that this way if len is 0 you
make sure to have '\0' to avoid parsing problems?
> - data = kzalloc(count, GFP_KERNEL);
> + data = kzalloc(count + 1, GFP_KERNEL);
> if (data == NULL)
> return -ENOMEM;
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] Smack: fix behavior of smack_inode_listsecurity
2014-08-07 16:52 [PATCH 1/3] Smack: fix behavior of smack_inode_listsecurity Konstantin Khlebnikov
2014-08-07 16:52 ` [PATCH 2/3] Smack: handle zero-length security labels without panic Konstantin Khlebnikov
2014-08-07 16:52 ` [PATCH 3/3] Smack: remove unneeded NULL-termination from securtity label Konstantin Khlebnikov
@ 2014-08-08 20:27 ` Serge E. Hallyn
2014-08-08 20:29 ` Casey Schaufler
2014-08-08 22:04 ` Casey Schaufler
4 siblings, 0 replies; 13+ messages in thread
From: Serge E. Hallyn @ 2014-08-08 20:27 UTC (permalink / raw)
To: Konstantin Khlebnikov
Cc: Casey Schaufler, linux-security-module, linux-kernel,
James Morris, Serge E. Hallyn
Quoting Konstantin Khlebnikov (k.khlebnikov@samsung.com):
> Security operation ->inode_listsecurity is used for generating list of
> available extended attributes for syscall listxattr. Currently it's used
> only in nfs4 or if filesystem doesn't provide i_op->listxattr.
>
> The list is the set of NULL-terminated names, one after the other.
> This method must include zero byte at the and into result.
>
> Also this function must return length even if string does not fit into
> output buffer or it is NULL, see similar method in selinux and man listxattr.
>
> Signed-off-by: Konstantin Khlebnikov <k.khlebnikov@samsung.com>
Acked-by: Serge Hallyn <serge.hallyn@canonical.com>
> ---
> security/smack/smack_lsm.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index e6ab307..b11ab23 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -1122,13 +1122,12 @@ static int smack_inode_getsecurity(const struct inode *inode,
> static int smack_inode_listsecurity(struct inode *inode, char *buffer,
> size_t buffer_size)
> {
> - int len = strlen(XATTR_NAME_SMACK);
> + int len = sizeof(XATTR_NAME_SMACK);
>
> - if (buffer != NULL && len <= buffer_size) {
> + if (buffer != NULL && len <= buffer_size)
> memcpy(buffer, XATTR_NAME_SMACK, len);
> - return len;
> - }
> - return -EINVAL;
> +
> + return len;
> }
>
> /**
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] Smack: fix behavior of smack_inode_listsecurity
2014-08-07 16:52 [PATCH 1/3] Smack: fix behavior of smack_inode_listsecurity Konstantin Khlebnikov
` (2 preceding siblings ...)
2014-08-08 20:27 ` [PATCH 1/3] Smack: fix behavior of smack_inode_listsecurity Serge E. Hallyn
@ 2014-08-08 20:29 ` Casey Schaufler
2014-08-08 22:04 ` Casey Schaufler
4 siblings, 0 replies; 13+ messages in thread
From: Casey Schaufler @ 2014-08-08 20:29 UTC (permalink / raw)
To: Konstantin Khlebnikov
Cc: linux-security-module, linux-kernel, James Morris, Serge E. Hallyn
On 8/7/2014 9:52 AM, Konstantin Khlebnikov wrote:
> Security operation ->inode_listsecurity is used for generating list of
> available extended attributes for syscall listxattr. Currently it's used
> only in nfs4 or if filesystem doesn't provide i_op->listxattr.
>
> The list is the set of NULL-terminated names, one after the other.
> This method must include zero byte at the and into result.
>
> Also this function must return length even if string does not fit into
> output buffer or it is NULL, see similar method in selinux and man listxattr.
>
> Signed-off-by: Konstantin Khlebnikov <k.khlebnikov@samsung.com>
I plan to ack this patch set once I've run them through my tests.
> ---
> security/smack/smack_lsm.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index e6ab307..b11ab23 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -1122,13 +1122,12 @@ static int smack_inode_getsecurity(const struct inode *inode,
> static int smack_inode_listsecurity(struct inode *inode, char *buffer,
> size_t buffer_size)
> {
> - int len = strlen(XATTR_NAME_SMACK);
> + int len = sizeof(XATTR_NAME_SMACK);
>
> - if (buffer != NULL && len <= buffer_size) {
> + if (buffer != NULL && len <= buffer_size)
> memcpy(buffer, XATTR_NAME_SMACK, len);
> - return len;
> - }
> - return -EINVAL;
> +
> + return len;
> }
>
> /**
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] Smack: handle zero-length security labels without panic
2014-08-08 20:25 ` Serge E. Hallyn
@ 2014-08-08 20:48 ` Konstantin Khlebnikov
2014-08-08 20:54 ` Serge E. Hallyn
0 siblings, 1 reply; 13+ messages in thread
From: Konstantin Khlebnikov @ 2014-08-08 20:48 UTC (permalink / raw)
To: Serge E. Hallyn
Cc: Konstantin Khlebnikov, Casey Schaufler, linux-security-module,
Linux Kernel Mailing List, James Morris
On Sat, Aug 9, 2014 at 12:25 AM, Serge E. Hallyn <serge@hallyn.com> wrote:
> Quoting Konstantin Khlebnikov (k.khlebnikov@samsung.com):
>> Zero-length security labels are invalid but kernel should handle them.
>>
>> This patch fixes kernel panic after setting zero-length security labels:
>> # attr -S -s "SMACK64" -V "" file
>>
>> And after writing zero-length string into smackfs files syslog and onlycp:
>> # python -c 'import os; os.write(1, "")' > /smack/syslog
>>
>> The problem is caused by brain-damaged logic in function smk_parse_smack()
>> which takes pointer to buffer and its length but if length below or equal zero
>> it thinks that the buffer is zero-terminated. Unfortunately callers of this
>> function are widely used and proper fix requires serious refactoring.
>>
>> Signed-off-by: Konstantin Khlebnikov <k.khlebnikov@samsung.com>
>
> Acked-by: Serge Hallyn <serge.hallyn@canonical.com>
>
>>
>> ---
>>
>> Example:
>>
>> [ 28.063935] BUG: unable to handle kernel NULL pointer dereference at (null)
>> [ 28.064623] IP: [<ffffffff812ea780>] strlen+0x0/0x30
>> [ 28.064623] PGD 29a77067 PUD 2972d067 PMD 0
>> [ 28.064623] Oops: 0000 [#1] SMP
>> [ 28.064623] Modules linked in:
>> [ 28.064623] CPU: 0 PID: 824 Comm: attr Not tainted 3.16.0+ #6
>> [ 28.064623] task: ffff880029bd88c0 ti: ffff8800297d0000 task.ti: ffff8800297d0000
>> [ 28.064623] RIP: 0010:[<ffffffff812ea780>] [<ffffffff812ea780>] strlen+0x0/0x30
>> [ 28.064623] RSP: 0018:ffff8800297d3ca8 EFLAGS: 00010246
>> [ 28.064623] RAX: 0000000000000001 RBX: ffff880029c096c0 RCX: 0000000000000001
>> [ 28.064623] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
>> [ 28.064623] RBP: ffff8800297d3cd0 R08: ffffffff81e3d940 R09: ffff88002a3e8588
>> [ 28.064623] R10: 8080808080808080 R11: 0000000000000000 R12: 0000000000000000
>> [ 28.064623] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
>> [ 28.064623] FS: 00007effe391a700(0000) GS:ffff88002a400000(0000) knlGS:0000000000000000
>> [ 28.064623] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> [ 28.064623] CR2: 0000000000000000 CR3: 00000000297a1000 CR4: 00000000001406b0
>> [ 28.064623] Stack:
>> [ 28.064623] ffffffff812a6fa5 ffff880029c096c0 0000000000000001 0000000000000000
>> [ 28.064623] 0000000000000000 ffff8800297d3cf8 ffffffff812a70ae ffff880029c096c0
>> [ 28.064623] 0000000000000001 0000000000000000 ffff8800297d3d88 ffffffff812a64dc
>> [ 28.064623] Call Trace:
>> [ 28.064623] [<ffffffff812a6fa5>] ? smk_parse_smack+0xe5/0x110
>> [ 28.064623] [<ffffffff812a70ae>] smk_import_entry+0xe/0x130
>> [ 28.064623] [<ffffffff812a64dc>] smack_inode_setxattr+0x11c/0x280
>> [ 28.064623] [<ffffffff8116a767>] ? lookup_fast+0x177/0x2e0
>> [ 28.064623] [<ffffffff81177001>] ? dput+0x21/0x1a0
>> [ 28.064623] [<ffffffff812a0eeb>] security_inode_setxattr+0x1b/0x30
>> [ 28.064623] [<ffffffff8118488f>] vfs_setxattr+0x6f/0xb0
>> [ 28.064623] [<ffffffff81184a76>] setxattr+0x1a6/0x1f0
>> [ 28.064623] [<ffffffff8116af6d>] ? final_putname+0x1d/0x40
>> [ 28.064623] [<ffffffff8116b1c4>] ? putname+0x24/0x40
>> [ 28.064623] [<ffffffff8117144a>] ? user_path_at_empty+0x5a/0xa0
>> [ 28.064623] [<ffffffff81162b84>] ? __sb_start_write+0x44/0xe0
>> [ 28.064623] [<ffffffff8113cd21>] ? do_brk+0x241/0x320
>> [ 28.064623] [<ffffffff81184d97>] SyS_lsetxattr+0x87/0xe0
>> [ 28.064623] [<ffffffff81806f69>] system_call_fastpath+0x16/0x1b
>> [ 28.064623] Code: 89 f8 48 89 e5 f6 82 40 3e a6 81 20 74 15 0f 1f 44 00 00 48 83 c0 01 0f b6 10 f6 82 40 3e a6 81 20 75 f0 5d c3 66 0f 1f 44 00 00 <80> 3f 00 55 48 89 e5 74 15 48 89 f8 0f 1f 40 00 48 83 c0 01 80
>> [ 28.064623] RIP [<ffffffff812ea780>] strlen+0x0/0x30
>> [ 28.064623] RSP <ffff8800297d3ca8>
>> [ 28.064623] CR2: 0000000000000000
>> [ 28.093561] ---[ end trace de1055429a98a5be ]---
>> ---
>> security/smack/smack_lsm.c | 2 +-
>> security/smack/smackfs.c | 4 ++--
>> 2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>> index b11ab23..afa5ad0 100644
>> --- a/security/smack/smack_lsm.c
>> +++ b/security/smack/smack_lsm.c
>> @@ -923,7 +923,7 @@ static int smack_inode_setxattr(struct dentry *dentry, const char *name,
>> rc = -EPERM;
>>
>> if (rc == 0 && check_import) {
>> - skp = smk_import_entry(value, size);
>> + skp = size ? smk_import_entry(value, size) : NULL;
>> if (skp == NULL || (check_star &&
>> (skp == &smack_known_star || skp == &smack_known_web)))
>> rc = -EINVAL;
>> diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
>> index 3c720ff..56a1439 100644
>> --- a/security/smack/smackfs.c
>> +++ b/security/smack/smackfs.c
>> @@ -1677,7 +1677,7 @@ static ssize_t smk_write_onlycap(struct file *file, const char __user *buf,
>> if (smack_onlycap != NULL && smack_onlycap != skp)
>> return -EPERM;
>>
>> - data = kzalloc(count, GFP_KERNEL);
>> + data = kzalloc(count + 1, GFP_KERNEL);
>> if (data == NULL)
>> return -ENOMEM;
>>
>> @@ -2228,7 +2228,7 @@ static ssize_t smk_write_syslog(struct file *file, const char __user *buf,
>> if (!smack_privileged(CAP_MAC_ADMIN))
>> return -EPERM;
>>
>
> How about putting a comment here explaining that this way if len is 0 you
> make sure to have '\0' to avoid parsing problems?
There are plenty of these comments. That design is uncommon and
extremely dangerous,
it cannot be neutralized just by a comment.
Probably magic length '0' might be replaced with magic '-1' which is much safer.
>
>> - data = kzalloc(count, GFP_KERNEL);
>> + data = kzalloc(count + 1, GFP_KERNEL);
>> if (data == NULL)
>> return -ENOMEM;
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] Smack: remove unneeded NULL-termination from securtity label
2014-08-07 16:52 ` [PATCH 3/3] Smack: remove unneeded NULL-termination from securtity label Konstantin Khlebnikov
@ 2014-08-08 20:51 ` Serge E. Hallyn
2014-08-08 22:05 ` Casey Schaufler
1 sibling, 0 replies; 13+ messages in thread
From: Serge E. Hallyn @ 2014-08-08 20:51 UTC (permalink / raw)
To: Konstantin Khlebnikov
Cc: Casey Schaufler, linux-security-module, linux-kernel,
James Morris, Serge E. Hallyn
Quoting Konstantin Khlebnikov (k.khlebnikov@samsung.com):
> Values of extended attributes are stored as binary blobs. NULL-termination
> of them isn't required. It just wastes disk space and confuses command-line
> tools like getfattr because they have to print that zero byte at the end.
>
> This patch removes terminating zero byte from initial security label in
> smack_inode_init_security and cuts it out in function smack_inode_getsecurity
> which is used by syscall getxattr. This change seems completely safe, because
> function smk_parse_smack ignores everything after first zero byte.
>
> Signed-off-by: Konstantin Khlebnikov <k.khlebnikov@samsung.com>
selinux doesn't seem to do it this way, and apparmor doesn't use these
at all, but your reasoning does seem correct.
Acked-by: Serge Hallyn <serge.hallyn@canonical.com>
> ---
> security/smack/smack_lsm.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index afa5ad0..16ae853 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -672,7 +672,7 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
> }
>
> if (len)
> - *len = strlen(isp) + 1;
> + *len = strlen(isp);
>
> return 0;
> }
> @@ -1076,7 +1076,7 @@ static int smack_inode_getsecurity(const struct inode *inode,
>
> if (strcmp(name, XATTR_SMACK_SUFFIX) == 0) {
> isp = smk_of_inode(inode);
> - ilen = strlen(isp) + 1;
> + ilen = strlen(isp);
> *buffer = isp;
> return ilen;
> }
> @@ -1101,7 +1101,7 @@ static int smack_inode_getsecurity(const struct inode *inode,
> else
> return -EOPNOTSUPP;
>
> - ilen = strlen(isp) + 1;
> + ilen = strlen(isp);
> if (rc == 0) {
> *buffer = isp;
> rc = ilen;
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] Smack: handle zero-length security labels without panic
2014-08-08 20:48 ` Konstantin Khlebnikov
@ 2014-08-08 20:54 ` Serge E. Hallyn
2014-08-08 21:10 ` Casey Schaufler
0 siblings, 1 reply; 13+ messages in thread
From: Serge E. Hallyn @ 2014-08-08 20:54 UTC (permalink / raw)
To: Konstantin Khlebnikov
Cc: Serge E. Hallyn, Konstantin Khlebnikov, Casey Schaufler,
linux-security-module, Linux Kernel Mailing List, James Morris
Quoting Konstantin Khlebnikov (koct9i@gmail.com):
> On Sat, Aug 9, 2014 at 12:25 AM, Serge E. Hallyn <serge@hallyn.com> wrote:
> > Quoting Konstantin Khlebnikov (k.khlebnikov@samsung.com):
> >> Zero-length security labels are invalid but kernel should handle them.
> >>
> >> This patch fixes kernel panic after setting zero-length security labels:
> >> # attr -S -s "SMACK64" -V "" file
> >>
> >> And after writing zero-length string into smackfs files syslog and onlycp:
> >> # python -c 'import os; os.write(1, "")' > /smack/syslog
> >>
> >> The problem is caused by brain-damaged logic in function smk_parse_smack()
> >> which takes pointer to buffer and its length but if length below or equal zero
> >> it thinks that the buffer is zero-terminated. Unfortunately callers of this
> >> function are widely used and proper fix requires serious refactoring.
> >>
> >> Signed-off-by: Konstantin Khlebnikov <k.khlebnikov@samsung.com>
> >
> > Acked-by: Serge Hallyn <serge.hallyn@canonical.com>
> >
> >>
> >> ---
> >>
> >> Example:
> >>
> >> [ 28.063935] BUG: unable to handle kernel NULL pointer dereference at (null)
> >> [ 28.064623] IP: [<ffffffff812ea780>] strlen+0x0/0x30
> >> [ 28.064623] PGD 29a77067 PUD 2972d067 PMD 0
> >> [ 28.064623] Oops: 0000 [#1] SMP
> >> [ 28.064623] Modules linked in:
> >> [ 28.064623] CPU: 0 PID: 824 Comm: attr Not tainted 3.16.0+ #6
> >> [ 28.064623] task: ffff880029bd88c0 ti: ffff8800297d0000 task.ti: ffff8800297d0000
> >> [ 28.064623] RIP: 0010:[<ffffffff812ea780>] [<ffffffff812ea780>] strlen+0x0/0x30
> >> [ 28.064623] RSP: 0018:ffff8800297d3ca8 EFLAGS: 00010246
> >> [ 28.064623] RAX: 0000000000000001 RBX: ffff880029c096c0 RCX: 0000000000000001
> >> [ 28.064623] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
> >> [ 28.064623] RBP: ffff8800297d3cd0 R08: ffffffff81e3d940 R09: ffff88002a3e8588
> >> [ 28.064623] R10: 8080808080808080 R11: 0000000000000000 R12: 0000000000000000
> >> [ 28.064623] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
> >> [ 28.064623] FS: 00007effe391a700(0000) GS:ffff88002a400000(0000) knlGS:0000000000000000
> >> [ 28.064623] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >> [ 28.064623] CR2: 0000000000000000 CR3: 00000000297a1000 CR4: 00000000001406b0
> >> [ 28.064623] Stack:
> >> [ 28.064623] ffffffff812a6fa5 ffff880029c096c0 0000000000000001 0000000000000000
> >> [ 28.064623] 0000000000000000 ffff8800297d3cf8 ffffffff812a70ae ffff880029c096c0
> >> [ 28.064623] 0000000000000001 0000000000000000 ffff8800297d3d88 ffffffff812a64dc
> >> [ 28.064623] Call Trace:
> >> [ 28.064623] [<ffffffff812a6fa5>] ? smk_parse_smack+0xe5/0x110
> >> [ 28.064623] [<ffffffff812a70ae>] smk_import_entry+0xe/0x130
> >> [ 28.064623] [<ffffffff812a64dc>] smack_inode_setxattr+0x11c/0x280
> >> [ 28.064623] [<ffffffff8116a767>] ? lookup_fast+0x177/0x2e0
> >> [ 28.064623] [<ffffffff81177001>] ? dput+0x21/0x1a0
> >> [ 28.064623] [<ffffffff812a0eeb>] security_inode_setxattr+0x1b/0x30
> >> [ 28.064623] [<ffffffff8118488f>] vfs_setxattr+0x6f/0xb0
> >> [ 28.064623] [<ffffffff81184a76>] setxattr+0x1a6/0x1f0
> >> [ 28.064623] [<ffffffff8116af6d>] ? final_putname+0x1d/0x40
> >> [ 28.064623] [<ffffffff8116b1c4>] ? putname+0x24/0x40
> >> [ 28.064623] [<ffffffff8117144a>] ? user_path_at_empty+0x5a/0xa0
> >> [ 28.064623] [<ffffffff81162b84>] ? __sb_start_write+0x44/0xe0
> >> [ 28.064623] [<ffffffff8113cd21>] ? do_brk+0x241/0x320
> >> [ 28.064623] [<ffffffff81184d97>] SyS_lsetxattr+0x87/0xe0
> >> [ 28.064623] [<ffffffff81806f69>] system_call_fastpath+0x16/0x1b
> >> [ 28.064623] Code: 89 f8 48 89 e5 f6 82 40 3e a6 81 20 74 15 0f 1f 44 00 00 48 83 c0 01 0f b6 10 f6 82 40 3e a6 81 20 75 f0 5d c3 66 0f 1f 44 00 00 <80> 3f 00 55 48 89 e5 74 15 48 89 f8 0f 1f 40 00 48 83 c0 01 80
> >> [ 28.064623] RIP [<ffffffff812ea780>] strlen+0x0/0x30
> >> [ 28.064623] RSP <ffff8800297d3ca8>
> >> [ 28.064623] CR2: 0000000000000000
> >> [ 28.093561] ---[ end trace de1055429a98a5be ]---
> >> ---
> >> security/smack/smack_lsm.c | 2 +-
> >> security/smack/smackfs.c | 4 ++--
> >> 2 files changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> >> index b11ab23..afa5ad0 100644
> >> --- a/security/smack/smack_lsm.c
> >> +++ b/security/smack/smack_lsm.c
> >> @@ -923,7 +923,7 @@ static int smack_inode_setxattr(struct dentry *dentry, const char *name,
> >> rc = -EPERM;
> >>
> >> if (rc == 0 && check_import) {
> >> - skp = smk_import_entry(value, size);
> >> + skp = size ? smk_import_entry(value, size) : NULL;
> >> if (skp == NULL || (check_star &&
> >> (skp == &smack_known_star || skp == &smack_known_web)))
> >> rc = -EINVAL;
> >> diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
> >> index 3c720ff..56a1439 100644
> >> --- a/security/smack/smackfs.c
> >> +++ b/security/smack/smackfs.c
> >> @@ -1677,7 +1677,7 @@ static ssize_t smk_write_onlycap(struct file *file, const char __user *buf,
> >> if (smack_onlycap != NULL && smack_onlycap != skp)
> >> return -EPERM;
> >>
> >> - data = kzalloc(count, GFP_KERNEL);
> >> + data = kzalloc(count + 1, GFP_KERNEL);
> >> if (data == NULL)
> >> return -ENOMEM;
> >>
> >> @@ -2228,7 +2228,7 @@ static ssize_t smk_write_syslog(struct file *file, const char __user *buf,
> >> if (!smack_privileged(CAP_MAC_ADMIN))
> >> return -EPERM;
> >>
> >
> > How about putting a comment here explaining that this way if len is 0 you
> > make sure to have '\0' to avoid parsing problems?
>
> There are plenty of these comments. That design is uncommon and
Exactly why it should be documented.
> extremely dangerous,
> it cannot be neutralized just by a comment.
It's not about neutralizing it. It's about down the road, while you're on leave
for a few months, someone saying "say that +1 seems unnecessary, I'll just remove it."
> Probably magic length '0' might be replaced with magic '-1' which is much safer.
Could be.
> >
> >> - data = kzalloc(count, GFP_KERNEL);
> >> + data = kzalloc(count + 1, GFP_KERNEL);
> >> if (data == NULL)
> >> return -ENOMEM;
> >>
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] Smack: handle zero-length security labels without panic
2014-08-08 20:54 ` Serge E. Hallyn
@ 2014-08-08 21:10 ` Casey Schaufler
0 siblings, 0 replies; 13+ messages in thread
From: Casey Schaufler @ 2014-08-08 21:10 UTC (permalink / raw)
To: Serge E. Hallyn, Konstantin Khlebnikov
Cc: Konstantin Khlebnikov, linux-security-module,
Linux Kernel Mailing List, James Morris, Casey Schaufler
On 8/8/2014 1:54 PM, Serge E. Hallyn wrote:
> Quoting Konstantin Khlebnikov (koct9i@gmail.com):
>> On Sat, Aug 9, 2014 at 12:25 AM, Serge E. Hallyn <serge@hallyn.com> wrote:
>>> Quoting Konstantin Khlebnikov (k.khlebnikov@samsung.com):
>>>> Zero-length security labels are invalid but kernel should handle them.
>>>>
>>>> This patch fixes kernel panic after setting zero-length security labels:
>>>> # attr -S -s "SMACK64" -V "" file
>>>>
>>>> And after writing zero-length string into smackfs files syslog and onlycp:
>>>> # python -c 'import os; os.write(1, "")' > /smack/syslog
>>>>
>>>> The problem is caused by brain-damaged logic in function smk_parse_smack()
>>>> which takes pointer to buffer and its length but if length below or equal zero
>>>> it thinks that the buffer is zero-terminated. Unfortunately callers of this
>>>> function are widely used and proper fix requires serious refactoring.
>>>>
>>>> Signed-off-by: Konstantin Khlebnikov <k.khlebnikov@samsung.com>
>>> Acked-by: Serge Hallyn <serge.hallyn@canonical.com>
>>>
>>>> ---
>>>>
>>>> Example:
>>>>
>>>> [ 28.063935] BUG: unable to handle kernel NULL pointer dereference at (null)
>>>> [ 28.064623] IP: [<ffffffff812ea780>] strlen+0x0/0x30
>>>> [ 28.064623] PGD 29a77067 PUD 2972d067 PMD 0
>>>> [ 28.064623] Oops: 0000 [#1] SMP
>>>> [ 28.064623] Modules linked in:
>>>> [ 28.064623] CPU: 0 PID: 824 Comm: attr Not tainted 3.16.0+ #6
>>>> [ 28.064623] task: ffff880029bd88c0 ti: ffff8800297d0000 task.ti: ffff8800297d0000
>>>> [ 28.064623] RIP: 0010:[<ffffffff812ea780>] [<ffffffff812ea780>] strlen+0x0/0x30
>>>> [ 28.064623] RSP: 0018:ffff8800297d3ca8 EFLAGS: 00010246
>>>> [ 28.064623] RAX: 0000000000000001 RBX: ffff880029c096c0 RCX: 0000000000000001
>>>> [ 28.064623] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
>>>> [ 28.064623] RBP: ffff8800297d3cd0 R08: ffffffff81e3d940 R09: ffff88002a3e8588
>>>> [ 28.064623] R10: 8080808080808080 R11: 0000000000000000 R12: 0000000000000000
>>>> [ 28.064623] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
>>>> [ 28.064623] FS: 00007effe391a700(0000) GS:ffff88002a400000(0000) knlGS:0000000000000000
>>>> [ 28.064623] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>> [ 28.064623] CR2: 0000000000000000 CR3: 00000000297a1000 CR4: 00000000001406b0
>>>> [ 28.064623] Stack:
>>>> [ 28.064623] ffffffff812a6fa5 ffff880029c096c0 0000000000000001 0000000000000000
>>>> [ 28.064623] 0000000000000000 ffff8800297d3cf8 ffffffff812a70ae ffff880029c096c0
>>>> [ 28.064623] 0000000000000001 0000000000000000 ffff8800297d3d88 ffffffff812a64dc
>>>> [ 28.064623] Call Trace:
>>>> [ 28.064623] [<ffffffff812a6fa5>] ? smk_parse_smack+0xe5/0x110
>>>> [ 28.064623] [<ffffffff812a70ae>] smk_import_entry+0xe/0x130
>>>> [ 28.064623] [<ffffffff812a64dc>] smack_inode_setxattr+0x11c/0x280
>>>> [ 28.064623] [<ffffffff8116a767>] ? lookup_fast+0x177/0x2e0
>>>> [ 28.064623] [<ffffffff81177001>] ? dput+0x21/0x1a0
>>>> [ 28.064623] [<ffffffff812a0eeb>] security_inode_setxattr+0x1b/0x30
>>>> [ 28.064623] [<ffffffff8118488f>] vfs_setxattr+0x6f/0xb0
>>>> [ 28.064623] [<ffffffff81184a76>] setxattr+0x1a6/0x1f0
>>>> [ 28.064623] [<ffffffff8116af6d>] ? final_putname+0x1d/0x40
>>>> [ 28.064623] [<ffffffff8116b1c4>] ? putname+0x24/0x40
>>>> [ 28.064623] [<ffffffff8117144a>] ? user_path_at_empty+0x5a/0xa0
>>>> [ 28.064623] [<ffffffff81162b84>] ? __sb_start_write+0x44/0xe0
>>>> [ 28.064623] [<ffffffff8113cd21>] ? do_brk+0x241/0x320
>>>> [ 28.064623] [<ffffffff81184d97>] SyS_lsetxattr+0x87/0xe0
>>>> [ 28.064623] [<ffffffff81806f69>] system_call_fastpath+0x16/0x1b
>>>> [ 28.064623] Code: 89 f8 48 89 e5 f6 82 40 3e a6 81 20 74 15 0f 1f 44 00 00 48 83 c0 01 0f b6 10 f6 82 40 3e a6 81 20 75 f0 5d c3 66 0f 1f 44 00 00 <80> 3f 00 55 48 89 e5 74 15 48 89 f8 0f 1f 40 00 48 83 c0 01 80
>>>> [ 28.064623] RIP [<ffffffff812ea780>] strlen+0x0/0x30
>>>> [ 28.064623] RSP <ffff8800297d3ca8>
>>>> [ 28.064623] CR2: 0000000000000000
>>>> [ 28.093561] ---[ end trace de1055429a98a5be ]---
>>>> ---
>>>> security/smack/smack_lsm.c | 2 +-
>>>> security/smack/smackfs.c | 4 ++--
>>>> 2 files changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>>>> index b11ab23..afa5ad0 100644
>>>> --- a/security/smack/smack_lsm.c
>>>> +++ b/security/smack/smack_lsm.c
>>>> @@ -923,7 +923,7 @@ static int smack_inode_setxattr(struct dentry *dentry, const char *name,
>>>> rc = -EPERM;
>>>>
>>>> if (rc == 0 && check_import) {
>>>> - skp = smk_import_entry(value, size);
>>>> + skp = size ? smk_import_entry(value, size) : NULL;
>>>> if (skp == NULL || (check_star &&
>>>> (skp == &smack_known_star || skp == &smack_known_web)))
>>>> rc = -EINVAL;
>>>> diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
>>>> index 3c720ff..56a1439 100644
>>>> --- a/security/smack/smackfs.c
>>>> +++ b/security/smack/smackfs.c
>>>> @@ -1677,7 +1677,7 @@ static ssize_t smk_write_onlycap(struct file *file, const char __user *buf,
>>>> if (smack_onlycap != NULL && smack_onlycap != skp)
>>>> return -EPERM;
>>>>
>>>> - data = kzalloc(count, GFP_KERNEL);
>>>> + data = kzalloc(count + 1, GFP_KERNEL);
>>>> if (data == NULL)
>>>> return -ENOMEM;
>>>>
>>>> @@ -2228,7 +2228,7 @@ static ssize_t smk_write_syslog(struct file *file, const char __user *buf,
>>>> if (!smack_privileged(CAP_MAC_ADMIN))
>>>> return -EPERM;
>>>>
>>> How about putting a comment here explaining that this way if len is 0 you
>>> make sure to have '\0' to avoid parsing problems?
>> There are plenty of these comments. That design is uncommon and
> Exactly why it should be documented.
The code was much simpler when I decided that a 0 length could
safely indicate that the string was known to be terminated. Several
other people have been in the code since, and it got crufted up
in the name of performance. Oh well.Thank you for taking the time to
provide a patch. I appreciate your efforts.
>
>> extremely dangerous,
>> it cannot be neutralized just by a comment.
> It's not about neutralizing it. It's about down the road, while you're on leave
> for a few months, someone saying "say that +1 seems unnecessary, I'll just remove it."
I'd rather have a pass through the entirety of the label import code.
Some of the performance optimizations were done in a way that, while it
seemed like a good idea at the time, didn't necessarily demonstrate a
through understanding of what the surrounding code was up to.
>
>> Probably magic length '0' might be replaced with magic '-1' which is much safer.
> Could be.
Not a bad idea.
>
>>>> - data = kzalloc(count, GFP_KERNEL);
>>>> + data = kzalloc(count + 1, GFP_KERNEL);
>>>> if (data == NULL)
>>>> return -ENOMEM;
>>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>> Please read the FAQ at http://www.tux.org/lkml/
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] Smack: fix behavior of smack_inode_listsecurity
2014-08-07 16:52 [PATCH 1/3] Smack: fix behavior of smack_inode_listsecurity Konstantin Khlebnikov
` (3 preceding siblings ...)
2014-08-08 20:29 ` Casey Schaufler
@ 2014-08-08 22:04 ` Casey Schaufler
4 siblings, 0 replies; 13+ messages in thread
From: Casey Schaufler @ 2014-08-08 22:04 UTC (permalink / raw)
To: Konstantin Khlebnikov
Cc: linux-security-module, linux-kernel, James Morris, Serge E. Hallyn
On 8/7/2014 9:52 AM, Konstantin Khlebnikov wrote:
> Security operation ->inode_listsecurity is used for generating list of
> available extended attributes for syscall listxattr. Currently it's used
> only in nfs4 or if filesystem doesn't provide i_op->listxattr.
>
> The list is the set of NULL-terminated names, one after the other.
> This method must include zero byte at the and into result.
>
> Also this function must return length even if string does not fit into
> output buffer or it is NULL, see similar method in selinux and man listxattr.
>
> Signed-off-by: Konstantin Khlebnikov <k.khlebnikov@samsung.com>
Applied to git://git.gitorious.org/smack-next/kernel.git#smack-for-3.18
> ---
> security/smack/smack_lsm.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index e6ab307..b11ab23 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -1122,13 +1122,12 @@ static int smack_inode_getsecurity(const struct inode *inode,
> static int smack_inode_listsecurity(struct inode *inode, char *buffer,
> size_t buffer_size)
> {
> - int len = strlen(XATTR_NAME_SMACK);
> + int len = sizeof(XATTR_NAME_SMACK);
>
> - if (buffer != NULL && len <= buffer_size) {
> + if (buffer != NULL && len <= buffer_size)
> memcpy(buffer, XATTR_NAME_SMACK, len);
> - return len;
> - }
> - return -EINVAL;
> +
> + return len;
> }
>
> /**
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] Smack: handle zero-length security labels without panic
2014-08-07 16:52 ` [PATCH 2/3] Smack: handle zero-length security labels without panic Konstantin Khlebnikov
2014-08-08 20:25 ` Serge E. Hallyn
@ 2014-08-08 22:05 ` Casey Schaufler
1 sibling, 0 replies; 13+ messages in thread
From: Casey Schaufler @ 2014-08-08 22:05 UTC (permalink / raw)
To: Konstantin Khlebnikov
Cc: linux-security-module, linux-kernel, James Morris, Serge E. Hallyn
On 8/7/2014 9:52 AM, Konstantin Khlebnikov wrote:
> Zero-length security labels are invalid but kernel should handle them.
>
> This patch fixes kernel panic after setting zero-length security labels:
> # attr -S -s "SMACK64" -V "" file
>
> And after writing zero-length string into smackfs files syslog and onlycp:
> # python -c 'import os; os.write(1, "")' > /smack/syslog
>
> The problem is caused by brain-damaged logic in function smk_parse_smack()
> which takes pointer to buffer and its length but if length below or equal zero
> it thinks that the buffer is zero-terminated. Unfortunately callers of this
> function are widely used and proper fix requires serious refactoring.
>
> Signed-off-by: Konstantin Khlebnikov <k.khlebnikov@samsung.com>
Applied to git://git.gitorious.org/smack-next/kernel.git#smack-for-3.18
>
> ---
>
> Example:
>
> [ 28.063935] BUG: unable to handle kernel NULL pointer dereference at (null)
> [ 28.064623] IP: [<ffffffff812ea780>] strlen+0x0/0x30
> [ 28.064623] PGD 29a77067 PUD 2972d067 PMD 0
> [ 28.064623] Oops: 0000 [#1] SMP
> [ 28.064623] Modules linked in:
> [ 28.064623] CPU: 0 PID: 824 Comm: attr Not tainted 3.16.0+ #6
> [ 28.064623] task: ffff880029bd88c0 ti: ffff8800297d0000 task.ti: ffff8800297d0000
> [ 28.064623] RIP: 0010:[<ffffffff812ea780>] [<ffffffff812ea780>] strlen+0x0/0x30
> [ 28.064623] RSP: 0018:ffff8800297d3ca8 EFLAGS: 00010246
> [ 28.064623] RAX: 0000000000000001 RBX: ffff880029c096c0 RCX: 0000000000000001
> [ 28.064623] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
> [ 28.064623] RBP: ffff8800297d3cd0 R08: ffffffff81e3d940 R09: ffff88002a3e8588
> [ 28.064623] R10: 8080808080808080 R11: 0000000000000000 R12: 0000000000000000
> [ 28.064623] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
> [ 28.064623] FS: 00007effe391a700(0000) GS:ffff88002a400000(0000) knlGS:0000000000000000
> [ 28.064623] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 28.064623] CR2: 0000000000000000 CR3: 00000000297a1000 CR4: 00000000001406b0
> [ 28.064623] Stack:
> [ 28.064623] ffffffff812a6fa5 ffff880029c096c0 0000000000000001 0000000000000000
> [ 28.064623] 0000000000000000 ffff8800297d3cf8 ffffffff812a70ae ffff880029c096c0
> [ 28.064623] 0000000000000001 0000000000000000 ffff8800297d3d88 ffffffff812a64dc
> [ 28.064623] Call Trace:
> [ 28.064623] [<ffffffff812a6fa5>] ? smk_parse_smack+0xe5/0x110
> [ 28.064623] [<ffffffff812a70ae>] smk_import_entry+0xe/0x130
> [ 28.064623] [<ffffffff812a64dc>] smack_inode_setxattr+0x11c/0x280
> [ 28.064623] [<ffffffff8116a767>] ? lookup_fast+0x177/0x2e0
> [ 28.064623] [<ffffffff81177001>] ? dput+0x21/0x1a0
> [ 28.064623] [<ffffffff812a0eeb>] security_inode_setxattr+0x1b/0x30
> [ 28.064623] [<ffffffff8118488f>] vfs_setxattr+0x6f/0xb0
> [ 28.064623] [<ffffffff81184a76>] setxattr+0x1a6/0x1f0
> [ 28.064623] [<ffffffff8116af6d>] ? final_putname+0x1d/0x40
> [ 28.064623] [<ffffffff8116b1c4>] ? putname+0x24/0x40
> [ 28.064623] [<ffffffff8117144a>] ? user_path_at_empty+0x5a/0xa0
> [ 28.064623] [<ffffffff81162b84>] ? __sb_start_write+0x44/0xe0
> [ 28.064623] [<ffffffff8113cd21>] ? do_brk+0x241/0x320
> [ 28.064623] [<ffffffff81184d97>] SyS_lsetxattr+0x87/0xe0
> [ 28.064623] [<ffffffff81806f69>] system_call_fastpath+0x16/0x1b
> [ 28.064623] Code: 89 f8 48 89 e5 f6 82 40 3e a6 81 20 74 15 0f 1f 44 00 00 48 83 c0 01 0f b6 10 f6 82 40 3e a6 81 20 75 f0 5d c3 66 0f 1f 44 00 00 <80> 3f 00 55 48 89 e5 74 15 48 89 f8 0f 1f 40 00 48 83 c0 01 80
> [ 28.064623] RIP [<ffffffff812ea780>] strlen+0x0/0x30
> [ 28.064623] RSP <ffff8800297d3ca8>
> [ 28.064623] CR2: 0000000000000000
> [ 28.093561] ---[ end trace de1055429a98a5be ]---
> ---
> security/smack/smack_lsm.c | 2 +-
> security/smack/smackfs.c | 4 ++--
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index b11ab23..afa5ad0 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -923,7 +923,7 @@ static int smack_inode_setxattr(struct dentry *dentry, const char *name,
> rc = -EPERM;
>
> if (rc == 0 && check_import) {
> - skp = smk_import_entry(value, size);
> + skp = size ? smk_import_entry(value, size) : NULL;
> if (skp == NULL || (check_star &&
> (skp == &smack_known_star || skp == &smack_known_web)))
> rc = -EINVAL;
> diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
> index 3c720ff..56a1439 100644
> --- a/security/smack/smackfs.c
> +++ b/security/smack/smackfs.c
> @@ -1677,7 +1677,7 @@ static ssize_t smk_write_onlycap(struct file *file, const char __user *buf,
> if (smack_onlycap != NULL && smack_onlycap != skp)
> return -EPERM;
>
> - data = kzalloc(count, GFP_KERNEL);
> + data = kzalloc(count + 1, GFP_KERNEL);
> if (data == NULL)
> return -ENOMEM;
>
> @@ -2228,7 +2228,7 @@ static ssize_t smk_write_syslog(struct file *file, const char __user *buf,
> if (!smack_privileged(CAP_MAC_ADMIN))
> return -EPERM;
>
> - data = kzalloc(count, GFP_KERNEL);
> + data = kzalloc(count + 1, GFP_KERNEL);
> if (data == NULL)
> return -ENOMEM;
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] Smack: remove unneeded NULL-termination from securtity label
2014-08-07 16:52 ` [PATCH 3/3] Smack: remove unneeded NULL-termination from securtity label Konstantin Khlebnikov
2014-08-08 20:51 ` Serge E. Hallyn
@ 2014-08-08 22:05 ` Casey Schaufler
1 sibling, 0 replies; 13+ messages in thread
From: Casey Schaufler @ 2014-08-08 22:05 UTC (permalink / raw)
To: Konstantin Khlebnikov
Cc: linux-security-module, linux-kernel, James Morris, Serge E. Hallyn
On 8/7/2014 9:52 AM, Konstantin Khlebnikov wrote:
> Values of extended attributes are stored as binary blobs. NULL-termination
> of them isn't required. It just wastes disk space and confuses command-line
> tools like getfattr because they have to print that zero byte at the end.
>
> This patch removes terminating zero byte from initial security label in
> smack_inode_init_security and cuts it out in function smack_inode_getsecurity
> which is used by syscall getxattr. This change seems completely safe, because
> function smk_parse_smack ignores everything after first zero byte.
>
> Signed-off-by: Konstantin Khlebnikov <k.khlebnikov@samsung.com>
Applied to git://git.gitorious.org/smack-next/kernel.git#smack-for-3.18
> ---
> security/smack/smack_lsm.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index afa5ad0..16ae853 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -672,7 +672,7 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
> }
>
> if (len)
> - *len = strlen(isp) + 1;
> + *len = strlen(isp);
>
> return 0;
> }
> @@ -1076,7 +1076,7 @@ static int smack_inode_getsecurity(const struct inode *inode,
>
> if (strcmp(name, XATTR_SMACK_SUFFIX) == 0) {
> isp = smk_of_inode(inode);
> - ilen = strlen(isp) + 1;
> + ilen = strlen(isp);
> *buffer = isp;
> return ilen;
> }
> @@ -1101,7 +1101,7 @@ static int smack_inode_getsecurity(const struct inode *inode,
> else
> return -EOPNOTSUPP;
>
> - ilen = strlen(isp) + 1;
> + ilen = strlen(isp);
> if (rc == 0) {
> *buffer = isp;
> rc = ilen;
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2014-08-08 22:12 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-08-07 16:52 [PATCH 1/3] Smack: fix behavior of smack_inode_listsecurity Konstantin Khlebnikov
2014-08-07 16:52 ` [PATCH 2/3] Smack: handle zero-length security labels without panic Konstantin Khlebnikov
2014-08-08 20:25 ` Serge E. Hallyn
2014-08-08 20:48 ` Konstantin Khlebnikov
2014-08-08 20:54 ` Serge E. Hallyn
2014-08-08 21:10 ` Casey Schaufler
2014-08-08 22:05 ` Casey Schaufler
2014-08-07 16:52 ` [PATCH 3/3] Smack: remove unneeded NULL-termination from securtity label Konstantin Khlebnikov
2014-08-08 20:51 ` Serge E. Hallyn
2014-08-08 22:05 ` Casey Schaufler
2014-08-08 20:27 ` [PATCH 1/3] Smack: fix behavior of smack_inode_listsecurity Serge E. Hallyn
2014-08-08 20:29 ` Casey Schaufler
2014-08-08 22:04 ` Casey Schaufler
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.