Messages in this thread | | | Subject | Re: [PATCH v2] smackfs: restrict bytes count in smackfs write functions | From | Casey Schaufler <> | Date | Thu, 28 Jan 2021 18:10:21 -0800 |
| |
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. >
| |