| Message ID | 20210128115801.1096425-1-snovitoll@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] smackfs: restrict bytes count in smackfs write functions | expand |
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)); ?
> > /* > > + * 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.
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.
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. >
> 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.
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.
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))
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(-)