* [PATCH] smackfs: restrict bytes count in smackfs write functions @ 2021-01-24 14:36 Sabyrzhan Tasbolatov 2021-01-25 18:08 ` Casey Schaufler 0 siblings, 1 reply; 10+ messages in thread From: Sabyrzhan Tasbolatov @ 2021-01-24 14:36 UTC (permalink / raw) To: casey Cc: jmorris, serge, andreyknvl, linux-security-module, linux-kernel, syzbot+a71a442385a0b2815497 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] smackfs: restrict bytes count in smackfs write functions 2021-01-24 14:36 [PATCH] smackfs: restrict bytes count in smackfs write functions Sabyrzhan Tasbolatov @ 2021-01-25 18:08 ` Casey Schaufler 2021-01-25 22:42 ` Tetsuo Handa 0 siblings, 1 reply; 10+ messages in thread From: Casey Schaufler @ 2021-01-25 18:08 UTC (permalink / raw) To: Sabyrzhan Tasbolatov Cc: jmorris, serge, andreyknvl, linux-security-module, linux-kernel, syzbot+a71a442385a0b2815497, 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)) ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] smackfs: restrict bytes count in smackfs write functions 2021-01-25 18:08 ` Casey Schaufler @ 2021-01-25 22:42 ` Tetsuo Handa 2021-01-28 11:58 ` [PATCH v2] " Sabyrzhan Tasbolatov 0 siblings, 1 reply; 10+ messages in thread From: Tetsuo Handa @ 2021-01-25 22:42 UTC (permalink / raw) To: Casey Schaufler, Sabyrzhan Tasbolatov Cc: jmorris, serge, andreyknvl, linux-security-module, linux-kernel, syzbot+a71a442385a0b2815497, Michal Hocko 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 . ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2] smackfs: restrict bytes count in smackfs write functions 2021-01-25 22:42 ` Tetsuo Handa @ 2021-01-28 11:58 ` Sabyrzhan Tasbolatov 2021-01-28 12:59 ` Tetsuo Handa 0 siblings, 1 reply; 10+ messages in thread From: Sabyrzhan Tasbolatov @ 2021-01-28 11:58 UTC (permalink / raw) To: penguin-kernel Cc: andreyknvl, casey, jmorris, linux-kernel, linux-security-module, mhocko, serge, snovitoll, syzbot+a71a442385a0b2815497 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2] smackfs: restrict bytes count in smackfs write functions 2021-01-28 11:58 ` [PATCH v2] " Sabyrzhan Tasbolatov @ 2021-01-28 12:59 ` Tetsuo Handa 2021-01-28 13:27 ` Sabyrzhan Tasbolatov 0 siblings, 1 reply; 10+ messages in thread From: Tetsuo Handa @ 2021-01-28 12:59 UTC (permalink / raw) To: Sabyrzhan Tasbolatov Cc: andreyknvl, casey, jmorris, linux-kernel, linux-security-module, mhocko, serge, syzbot+a71a442385a0b2815497 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)); ? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] smackfs: restrict bytes count in smackfs write functions 2021-01-28 12:59 ` Tetsuo Handa @ 2021-01-28 13:27 ` Sabyrzhan Tasbolatov 2021-01-28 14:24 ` Tetsuo Handa 0 siblings, 1 reply; 10+ messages in thread From: Sabyrzhan Tasbolatov @ 2021-01-28 13:27 UTC (permalink / raw) To: penguin-kernel Cc: andreyknvl, casey, jmorris, linux-kernel, linux-security-module, mhocko, serge, snovitoll, syzbot+a71a442385a0b2815497 > > /* > > + * 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] smackfs: restrict bytes count in smackfs write functions 2021-01-28 13:27 ` Sabyrzhan Tasbolatov @ 2021-01-28 14:24 ` Tetsuo Handa 2021-01-29 2:10 ` Casey Schaufler 0 siblings, 1 reply; 10+ messages in thread From: Tetsuo Handa @ 2021-01-28 14:24 UTC (permalink / raw) To: Sabyrzhan Tasbolatov Cc: andreyknvl, casey, jmorris, linux-kernel, linux-security-module, mhocko, serge, syzbot+a71a442385a0b2815497 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] smackfs: restrict bytes count in smackfs write functions 2021-01-28 14:24 ` Tetsuo Handa @ 2021-01-29 2:10 ` Casey Schaufler 2021-02-02 19:13 ` Sabyrzhan Tasbolatov 0 siblings, 1 reply; 10+ messages in thread From: Casey Schaufler @ 2021-01-29 2:10 UTC (permalink / raw) To: Tetsuo Handa, Sabyrzhan Tasbolatov Cc: andreyknvl, jmorris, linux-kernel, linux-security-module, mhocko, serge, syzbot+a71a442385a0b2815497, 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. > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] smackfs: restrict bytes count in smackfs write functions 2021-01-29 2:10 ` Casey Schaufler @ 2021-02-02 19:13 ` Sabyrzhan Tasbolatov 2021-02-02 19:33 ` Casey Schaufler 0 siblings, 1 reply; 10+ messages in thread From: Sabyrzhan Tasbolatov @ 2021-02-02 19:13 UTC (permalink / raw) To: casey Cc: andreyknvl, jmorris, linux-kernel, linux-security-module, mhocko, penguin-kernel, serge, snovitoll, syzbot+a71a442385a0b2815497 > 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] smackfs: restrict bytes count in smackfs write functions 2021-02-02 19:13 ` Sabyrzhan Tasbolatov @ 2021-02-02 19:33 ` Casey Schaufler 0 siblings, 0 replies; 10+ messages in thread From: Casey Schaufler @ 2021-02-02 19:33 UTC (permalink / raw) To: Sabyrzhan Tasbolatov Cc: andreyknvl, jmorris, linux-kernel, linux-security-module, mhocko, penguin-kernel, serge, syzbot+a71a442385a0b2815497, 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-02-02 19:36 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-24 14:36 [PATCH] smackfs: restrict bytes count in smackfs write functions Sabyrzhan Tasbolatov 2021-01-25 18:08 ` Casey Schaufler 2021-01-25 22:42 ` Tetsuo Handa 2021-01-28 11:58 ` [PATCH v2] " Sabyrzhan Tasbolatov 2021-01-28 12:59 ` Tetsuo Handa 2021-01-28 13:27 ` Sabyrzhan Tasbolatov 2021-01-28 14:24 ` Tetsuo Handa 2021-01-29 2:10 ` Casey Schaufler 2021-02-02 19:13 ` Sabyrzhan Tasbolatov 2021-02-02 19:33 ` 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.