[PATCH] smackfs: restrict bytes count in smackfs write functions

[PATCH] smackfs: restrict bytes count in smackfs write functions

[Sabyrzhan Tasbolatov]

syzbot found WARNINGs in several smackfs write operations where
bytes count is passed to memdup_user_nul which exceeds
GFP MAX_ORDER. Check count size if bigger SMK_LONGLABEL,
for smk_write_syslog if bigger than PAGE_SIZE - 1.

Reported-by: syzbot+a71a442385a0b2815497@syzkaller.appspotmail.com
Signed-off-by: Sabyrzhan Tasbolatov <snovitoll@gmail.com>
---
 security/smack/smackfs.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
index 5d44b7d258ef..88678c6f1b8c 100644
--- a/security/smack/smackfs.c
+++ b/security/smack/smackfs.c
@@ -1167,7 +1167,7 @@ static ssize_t smk_write_net4addr(struct file *file, const char __user *buf,
 		return -EPERM;
 	if (*ppos != 0)
 		return -EINVAL;
-	if (count < SMK_NETLBLADDRMIN)
+	if (count < SMK_NETLBLADDRMIN || count > SMK_LONGLABEL)
 		return -EINVAL;
 
 	data = memdup_user_nul(buf, count);
@@ -1427,7 +1427,7 @@ static ssize_t smk_write_net6addr(struct file *file, const char __user *buf,
 		return -EPERM;
 	if (*ppos != 0)
 		return -EINVAL;
-	if (count < SMK_NETLBLADDRMIN)
+	if (count < SMK_NETLBLADDRMIN || count > SMK_LONGLABEL)
 		return -EINVAL;
 
 	data = memdup_user_nul(buf, count);
@@ -2647,6 +2647,8 @@ static ssize_t smk_write_syslog(struct file *file, const char __user *buf,
 
 	if (!smack_privileged(CAP_MAC_ADMIN))
 		return -EPERM;
+	if (count > PAGE_SIZE - 1)
+		return -EINVAL;
 
 	data = memdup_user_nul(buf, count);
 	if (IS_ERR(data))
@@ -2744,6 +2746,8 @@ static ssize_t smk_write_relabel_self(struct file *file, const char __user *buf,
 	 */
 	if (*ppos != 0)
 		return -EINVAL;
+	if (count > SMK_LONGLABEL)
+		return -EINVAL;
 
 	data = memdup_user_nul(buf, count);
 	if (IS_ERR(data))
-- 
2.25.1

Re: [PATCH] smackfs: restrict bytes count in smackfs write functions

[Casey Schaufler]

On 1/24/2021 6:36 AM, Sabyrzhan Tasbolatov wrote:
> syzbot found WARNINGs in several smackfs write operations where
> bytes count is passed to memdup_user_nul which exceeds
> GFP MAX_ORDER. Check count size if bigger SMK_LONGLABEL,
> for smk_write_syslog if bigger than PAGE_SIZE - 1.
>
> Reported-by: syzbot+a71a442385a0b2815497@syzkaller.appspotmail.com
> Signed-off-by: Sabyrzhan Tasbolatov <snovitoll@gmail.com>

Thank you for the patch. Unfortunately, SMK_LONGLABEL isn't
the right value in some of these cases. 

> ---
>  security/smack/smackfs.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
> index 5d44b7d258ef..88678c6f1b8c 100644
> --- a/security/smack/smackfs.c
> +++ b/security/smack/smackfs.c
> @@ -1167,7 +1167,7 @@ static ssize_t smk_write_net4addr(struct file *file, const char __user *buf,
>  		return -EPERM;
>  	if (*ppos != 0)
>  		return -EINVAL;
> -	if (count < SMK_NETLBLADDRMIN)
> +	if (count < SMK_NETLBLADDRMIN || count > SMK_LONGLABEL)
>  		return -EINVAL;
>  
>  	data = memdup_user_nul(buf, count);
> @@ -1427,7 +1427,7 @@ static ssize_t smk_write_net6addr(struct file *file, const char __user *buf,
>  		return -EPERM;
>  	if (*ppos != 0)
>  		return -EINVAL;
> -	if (count < SMK_NETLBLADDRMIN)
> +	if (count < SMK_NETLBLADDRMIN || count > SMK_LONGLABEL)
>  		return -EINVAL;
>  
>  	data = memdup_user_nul(buf, count);
> @@ -2647,6 +2647,8 @@ static ssize_t smk_write_syslog(struct file *file, const char __user *buf,
>  
>  	if (!smack_privileged(CAP_MAC_ADMIN))
>  		return -EPERM;
> +	if (count > PAGE_SIZE - 1)
> +		return -EINVAL;
>  
>  	data = memdup_user_nul(buf, count);
>  	if (IS_ERR(data))
> @@ -2744,6 +2746,8 @@ static ssize_t smk_write_relabel_self(struct file *file, const char __user *buf,
>  	 */
>  	if (*ppos != 0)
>  		return -EINVAL;
> +	if (count > SMK_LONGLABEL)
> +		return -EINVAL;
>  
>  	data = memdup_user_nul(buf, count);
>  	if (IS_ERR(data))

Re: [PATCH] smackfs: restrict bytes count in smackfs write functions

[Tetsuo Handa]

On 2021/01/26 3:08, Casey Schaufler wrote:
> On 1/24/2021 6:36 AM, Sabyrzhan Tasbolatov wrote:
>> syzbot found WARNINGs in several smackfs write operations where
>> bytes count is passed to memdup_user_nul which exceeds
>> GFP MAX_ORDER. Check count size if bigger SMK_LONGLABEL,
>> for smk_write_syslog if bigger than PAGE_SIZE - 1.
>>
>> Reported-by: syzbot+a71a442385a0b2815497@syzkaller.appspotmail.com
>> Signed-off-by: Sabyrzhan Tasbolatov <snovitoll@gmail.com>
> 
> Thank you for the patch. Unfortunately, SMK_LONGLABEL isn't
> the right value in some of these cases. 
> 

Since it uses sscanf(), I think that whitespaces must be excluded from upper limit
check. I'm proposing adding __GFP_NOWARM on the memdup_user_nul() side at
https://lkml.kernel.org/r/20210120103436.11830-1-penguin-kernel@I-love.SAKURA.ne.jp .

[PATCH v2] smackfs: restrict bytes count in smackfs write functions

[Sabyrzhan Tasbolatov]

syzbot found WARNINGs in several smackfs write operations where
bytes count is passed to memdup_user_nul which exceeds
GFP MAX_ORDER. Check count size if bigger than PAGE_SIZE.

Per smackfs doc, smk_write_net4addr accepts any label or -CIPSO,
smk_write_net6addr accepts any label or -DELETE. I couldn't find
any general rule for other label lengths except SMK_LABELLEN,
SMK_LONGLABEL, SMK_CIPSOMAX which are documented.

Let's constrain, in general, smackfs label lengths for PAGE_SIZE.
Although fuzzer crashes write to smackfs/netlabel on 0x400000 length.

Here is a quick way to reproduce the WARNING:
python -c "print('A' * 0x400000)" > /sys/fs/smackfs/netlabel

Reported-by: syzbot+a71a442385a0b2815497@syzkaller.appspotmail.com
Signed-off-by: Sabyrzhan Tasbolatov <snovitoll@gmail.com>
---
 security/smack/smackfs.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
index 5d44b7d258ef..22ded2c26089 100644
--- a/security/smack/smackfs.c
+++ b/security/smack/smackfs.c
@@ -1167,7 +1167,7 @@ static ssize_t smk_write_net4addr(struct file *file, const char __user *buf,
 		return -EPERM;
 	if (*ppos != 0)
 		return -EINVAL;
-	if (count < SMK_NETLBLADDRMIN)
+	if (count < SMK_NETLBLADDRMIN || count > PAGE_SIZE - 1)
 		return -EINVAL;
 
 	data = memdup_user_nul(buf, count);
@@ -1427,7 +1427,7 @@ static ssize_t smk_write_net6addr(struct file *file, const char __user *buf,
 		return -EPERM;
 	if (*ppos != 0)
 		return -EINVAL;
-	if (count < SMK_NETLBLADDRMIN)
+	if (count < SMK_NETLBLADDRMIN || count > PAGE_SIZE - 1)
 		return -EINVAL;
 
 	data = memdup_user_nul(buf, count);
@@ -1834,6 +1834,10 @@ static ssize_t smk_write_ambient(struct file *file, const char __user *buf,
 	if (!smack_privileged(CAP_MAC_ADMIN))
 		return -EPERM;
 
+	/* Enough data must be present */
+	if (count == 0 || count > PAGE_SIZE)
+		return -EINVAL;
+
 	data = memdup_user_nul(buf, count);
 	if (IS_ERR(data))
 		return PTR_ERR(data);
@@ -2005,6 +2009,9 @@ static ssize_t smk_write_onlycap(struct file *file, const char __user *buf,
 	if (!smack_privileged(CAP_MAC_ADMIN))
 		return -EPERM;
 
+	if (count > PAGE_SIZE)
+		return -EINVAL;
+
 	data = memdup_user_nul(buf, count);
 	if (IS_ERR(data))
 		return PTR_ERR(data);
@@ -2092,6 +2099,9 @@ static ssize_t smk_write_unconfined(struct file *file, const char __user *buf,
 	if (!smack_privileged(CAP_MAC_ADMIN))
 		return -EPERM;
 
+	if (count > PAGE_SIZE)
+		return -EINVAL;
+
 	data = memdup_user_nul(buf, count);
 	if (IS_ERR(data))
 		return PTR_ERR(data);
@@ -2648,6 +2658,10 @@ static ssize_t smk_write_syslog(struct file *file, const char __user *buf,
 	if (!smack_privileged(CAP_MAC_ADMIN))
 		return -EPERM;
 
+	/* Enough data must be present */
+	if (count == 0 || count > PAGE_SIZE)
+		return -EINVAL;
+
 	data = memdup_user_nul(buf, count);
 	if (IS_ERR(data))
 		return PTR_ERR(data);
@@ -2740,10 +2754,13 @@ static ssize_t smk_write_relabel_self(struct file *file, const char __user *buf,
 		return -EPERM;
 
 	/*
+	 * No partial write.
 	 * Enough data must be present.
 	 */
 	if (*ppos != 0)
 		return -EINVAL;
+	if (count == 0 || count > PAGE_SIZE)
+		return -EINVAL;
 
 	data = memdup_user_nul(buf, count);
 	if (IS_ERR(data))
-- 
2.25.1

Re: [PATCH v2] smackfs: restrict bytes count in smackfs write functions

[Tetsuo Handa]

On 2021/01/28 20:58, Sabyrzhan Tasbolatov wrote:
> @@ -2005,6 +2009,9 @@ static ssize_t smk_write_onlycap(struct file *file, const char __user *buf,
>  	if (!smack_privileged(CAP_MAC_ADMIN))
>  		return -EPERM;
>  
> +	if (count > PAGE_SIZE)
> +		return -EINVAL;
> +
>  	data = memdup_user_nul(buf, count);
>  	if (IS_ERR(data))
>  		return PTR_ERR(data);
> @@ -2740,10 +2754,13 @@ static ssize_t smk_write_relabel_self(struct file *file, const char __user *buf,
>  		return -EPERM;
>  
>  	/*
> +	 * No partial write.
>  	 * Enough data must be present.
>  	 */
>  	if (*ppos != 0)
>  		return -EINVAL;
> +	if (count == 0 || count > PAGE_SIZE)
> +		return -EINVAL;
>  
>  	data = memdup_user_nul(buf, count);
>  	if (IS_ERR(data))
> 

Doesn't this change break legitimate requests like

  char buffer[20000];

  memset(buffer, ' ', sizeof(buffer));
  memcpy(buffer + sizeof(buffer) - 10, "foo", 3);
  write(fd, buffer, sizeof(buffer));

?

Re: [PATCH v2] smackfs: restrict bytes count in smackfs write functions

[Sabyrzhan Tasbolatov]

> >  	/*
> > +	 * No partial write.
> >  	 * Enough data must be present.
> >  	 */
> >  	if (*ppos != 0)
> >  		return -EINVAL;
> > +	if (count == 0 || count > PAGE_SIZE)
> > +		return -EINVAL;
> >  
> >  	data = memdup_user_nul(buf, count);
> >  	if (IS_ERR(data))
> > 
> 
> Doesn't this change break legitimate requests like
> 
>   char buffer[20000];
> 
>   memset(buffer, ' ', sizeof(buffer));
>   memcpy(buffer + sizeof(buffer) - 10, "foo", 3);
>   write(fd, buffer, sizeof(buffer));
> 
> ?

It does, in this case. Then I need to patch another version with
whitespace stripping before, after label. I just followed the same thing
that I see in security/selinux/selinuxfs.c sel_write_enforce() etc.

It has the same memdup_user_nul() and count >= PAGE_SIZE check prior to that.

Re: [PATCH v2] smackfs: restrict bytes count in smackfs write functions

[Tetsuo Handa]

On 2021/01/28 22:27, Sabyrzhan Tasbolatov wrote:
>> Doesn't this change break legitimate requests like
>>
>>   char buffer[20000];
>>
>>   memset(buffer, ' ', sizeof(buffer));
>>   memcpy(buffer + sizeof(buffer) - 10, "foo", 3);
>>   write(fd, buffer, sizeof(buffer));
>>
>> ?
> 
> It does, in this case. Then I need to patch another version with
> whitespace stripping before, after label. I just followed the same thing
> that I see in security/selinux/selinuxfs.c sel_write_enforce() etc.
> 
> It has the same memdup_user_nul() and count >= PAGE_SIZE check prior to that.

Since sel_write_enforce() accepts string representation of an integer value, PAGE_SIZE is sufficient.
But since smk_write_onlycap() and smk_write_relabel_self() accept list of space-delimited words,
you need to prove why PAGE_SIZE does not break userspace in your patch.

Also, due to the "too small to fail" memory-allocation rule, memdup_user_nul() for
count < PAGE_SIZE * 8 bytes is "never fails with -ENOMEM unless SIGKILLed by the OOM
killer". Also, memdup_user_nul() for count >= PAGE_SIZE * (1 << MAX_ORDER) - 1 bytes is
"never succeeds". Thus, you can safely add

	if (count >= PAGE_SIZE * (1 << MAX_ORDER) - 1)
		return -EINVAL; // or -ENOMEM if you want compatibility

to smackfs write functions. But it is a strange requirement that the caller of
memdup_user_nul() has to be aware of upper limit in a way that we won't hit

	/*
	 * There are several places where we assume that the order value is sane
	 * so bail out early if the request is out of bound.
	 */
	if (unlikely(order >= MAX_ORDER)) {
		WARN_ON_ONCE(!(gfp_mask & __GFP_NOWARN));
		return NULL;
	}

path. memdup_user_nul() side should do

	if (count >= PAGE_SIZE * (1 << MAX_ORDER) - 1)
		return -ENOMEM;

check and return -ENOMEM if memdup_user_nul() does not want to use __GFP_NOWARN.
I still believe that memdup_user_nul() side should be fixed.

Re: [PATCH v2] smackfs: restrict bytes count in smackfs write functions

[Casey Schaufler]

On 1/28/2021 6:24 AM, Tetsuo Handa wrote:
> On 2021/01/28 22:27, Sabyrzhan Tasbolatov wrote:
>>> Doesn't this change break legitimate requests like
>>>
>>>   char buffer[20000];
>>>
>>>   memset(buffer, ' ', sizeof(buffer));
>>>   memcpy(buffer + sizeof(buffer) - 10, "foo", 3);
>>>   write(fd, buffer, sizeof(buffer));
>>>
>>> ?
>> It does, in this case. Then I need to patch another version with
>> whitespace stripping before, after label. I just followed the same thing
>> that I see in security/selinux/selinuxfs.c sel_write_enforce() etc.
>>
>> It has the same memdup_user_nul() and count >= PAGE_SIZE check prior to that.
> Since sel_write_enforce() accepts string representation of an integer value, PAGE_SIZE is sufficient.
> But since smk_write_onlycap() and smk_write_relabel_self() accept list of space-delimited words,
> you need to prove why PAGE_SIZE does not break userspace in your patch.

if PAGE_SIZE >= SMK_LOADSIZE all legitimate requests can be made
using PAGE_SIZE as a limit. Your example with 19990 spaces before
the data demonstrates that the interface is inadequately documented.
Tizen and Automotive Grade Linux are going to be fine with a PAGE_SIZE
limit. The best way to address this concern is to go ahead with the
PAGE_SIZE limit and create ABI documents for the smackfs interfaces.
I will take your patch for the former and create a patch for the latter.


>
> Also, due to the "too small to fail" memory-allocation rule, memdup_user_nul() for
> count < PAGE_SIZE * 8 bytes is "never fails with -ENOMEM unless SIGKILLed by the OOM
> killer". Also, memdup_user_nul() for count >= PAGE_SIZE * (1 << MAX_ORDER) - 1 bytes is
> "never succeeds". Thus, you can safely add
>
> 	if (count >= PAGE_SIZE * (1 << MAX_ORDER) - 1)
> 		return -EINVAL; // or -ENOMEM if you want compatibility
>
> to smackfs write functions. But it is a strange requirement that the caller of
> memdup_user_nul() has to be aware of upper limit in a way that we won't hit
>
> 	/*
> 	 * There are several places where we assume that the order value is sane
> 	 * so bail out early if the request is out of bound.
> 	 */
> 	if (unlikely(order >= MAX_ORDER)) {
> 		WARN_ON_ONCE(!(gfp_mask & __GFP_NOWARN));
> 		return NULL;
> 	}
>
> path. memdup_user_nul() side should do
>
> 	if (count >= PAGE_SIZE * (1 << MAX_ORDER) - 1)
> 		return -ENOMEM;
>
> check and return -ENOMEM if memdup_user_nul() does not want to use __GFP_NOWARN.
> I still believe that memdup_user_nul() side should be fixed.
>

Re: [PATCH v2] smackfs: restrict bytes count in smackfs write functions

[Sabyrzhan Tasbolatov]

> if PAGE_SIZE >= SMK_LOADSIZE all legitimate requests can be made
> using PAGE_SIZE as a limit. Your example with 19990 spaces before
> the data demonstrates that the interface is inadequately documented.
> Tizen and Automotive Grade Linux are going to be fine with a PAGE_SIZE
> limit. The best way to address this concern is to go ahead with the
> PAGE_SIZE limit and create ABI documents for the smackfs interfaces.
> I will take your patch for the former and create a patch for the latter.

Please let me know if there is anything else required for this patch.
AFAIU, we agreed with PAGE_SIZE as the limit.

Re: [PATCH v2] smackfs: restrict bytes count in smackfs write functions

[Casey Schaufler]

On 2/2/2021 11:13 AM, Sabyrzhan Tasbolatov wrote:
>> if PAGE_SIZE >= SMK_LOADSIZE all legitimate requests can be made
>> using PAGE_SIZE as a limit. Your example with 19990 spaces before
>> the data demonstrates that the interface is inadequately documented.
>> Tizen and Automotive Grade Linux are going to be fine with a PAGE_SIZE
>> limit. The best way to address this concern is to go ahead with the
>> PAGE_SIZE limit and create ABI documents for the smackfs interfaces.
>> I will take your patch for the former and create a patch for the latter.
> Please let me know if there is anything else required for this patch.
> AFAIU, we agreed with PAGE_SIZE as the limit.

I am in the process of adding your patch to smack-next for 5.12.

I will have a separate documentation patch.