Re: [PATCH v2] smackfs: restrict bytes count in smackfs write functions
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.
Re: [PATCH v2] smackfs: restrict bytes count in smackfs write functions
> 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
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[2]; >>> >>> 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
On 2021/01/28 22:27, Sabyrzhan Tasbolatov wrote: >> Doesn't this change break legitimate requests like >> >> char buffer[2]; >> >> 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
> > /* > > +* 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[2]; > > 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
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[2]; memset(buffer, ' ', sizeof(buffer)); memcpy(buffer + sizeof(buffer) - 10, "foo", 3); write(fd, buffer, sizeof(buffer)); ?