Messages in this thread | | | Subject | Re: [PATCH v2] smackfs: restrict bytes count in smackfs write functions | From | Tetsuo Handa <> | Date | Thu, 28 Jan 2021 23:24:00 +0900 |
| |
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.
| |