[PATCH 1/3] Smack: fix behavior of smack_inode_listsecurity

[PATCH 1/3] Smack: fix behavior of smack_inode_listsecurity

[Konstantin Khlebnikov]

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;
 }
 
 /**

[PATCH 2/3] Smack: handle zero-length security labels without panic

[Konstantin Khlebnikov]

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;
 

Re: [PATCH 2/3] Smack: handle zero-length security labels without panic

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

Re: [PATCH 2/3] Smack: handle zero-length security labels without panic

[Konstantin Khlebnikov]

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/

Re: [PATCH 2/3] Smack: handle zero-length security labels without panic

[Serge E. Hallyn]

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/

Re: [PATCH 2/3] Smack: handle zero-length security labels without panic

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

Re: [PATCH 2/3] Smack: handle zero-length security labels without panic

[Casey Schaufler]

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

[PATCH 3/3] Smack: remove unneeded NULL-termination from securtity label

[Konstantin Khlebnikov]

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;

Re: [PATCH 3/3] Smack: remove unneeded NULL-termination from securtity label

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

Re: [PATCH 3/3] Smack: remove unneeded NULL-termination from securtity label

[Casey Schaufler]

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

Re: [PATCH 1/3] Smack: fix behavior of smack_inode_listsecurity

[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;
>  }
>  
>  /**

Re: [PATCH 1/3] Smack: fix behavior of smack_inode_listsecurity

[Casey Schaufler]

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;
>  }
>  
>  /**
>
>

Re: [PATCH 1/3] Smack: fix behavior of smack_inode_listsecurity

[Casey Schaufler]

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;
>  }
>  
>  /**
>
>