From: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
To: Sabyrzhan Tasbolatov <snovitoll@gmail.com>
Cc: andreyknvl@google.com, casey@schaufler-ca.com, jmorris@namei.org,
linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org, mhocko@suse.com,
serge@hallyn.com,
syzbot+a71a442385a0b2815497@syzkaller.appspotmail.com
Subject: Re: [PATCH v2] smackfs: restrict bytes count in smackfs write functions
Date: Thu, 28 Jan 2021 23:24:00 +0900 [thread overview]
Message-ID: <8d66b6fd-81d3-38bd-703f-522a2e2d6fca@i-love.sakura.ne.jp> (raw)
In-Reply-To: <20210128132721.1111920-1-snovitoll@gmail.com>
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.
next prev parent reply other threads:[~2021-01-28 14:27 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2021-01-29 2:10 ` Casey Schaufler
2021-02-02 19:13 ` Sabyrzhan Tasbolatov
2021-02-02 19:33 ` Casey Schaufler
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8d66b6fd-81d3-38bd-703f-522a2e2d6fca@i-love.sakura.ne.jp \
--to=penguin-kernel@i-love.sakura.ne.jp \
--cc=andreyknvl@google.com \
--cc=casey@schaufler-ca.com \
--cc=jmorris@namei.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=mhocko@suse.com \
--cc=serge@hallyn.com \
--cc=snovitoll@gmail.com \
--cc=syzbot+a71a442385a0b2815497@syzkaller.appspotmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.