Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v2] smackfs: restrict bytes count in smackfs write
 functions
To:     Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>,
        Sabyrzhan Tasbolatov <snovitoll@gmail.com>
Cc:     andreyknvl@google.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,
        Casey Schaufler <casey@schaufler-ca.com>
References: <5271074f-930a-46e9-8ece-2cc65d45dc19@i-love.sakura.ne.jp>
 <20210128132721.1111920-1-snovitoll@gmail.com>
 <8d66b6fd-81d3-38bd-703f-522a2e2d6fca@i-love.sakura.ne.jp>
From:   Casey Schaufler <casey@schaufler-ca.com>
Message-ID: <d0f9a341-281b-704c-04e4-4304ed17ba23@schaufler-ca.com>
Date:   Thu, 28 Jan 2021 18:10:21 -0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.6.1
MIME-Version: 1.0
In-Reply-To: <8d66b6fd-81d3-38bd-703f-522a2e2d6fca@i-love.sakura.ne.jp>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Content-Language: en-US
Content-Length: 2528
Precedence: bulk

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 thi=
ng
>> that I see in security/selinux/selinuxfs.c sel_write_enforce() etc.
>>
>> It has the same memdup_user_nul() and count >=3D PAGE_SIZE check prior=
 to that.
> Since sel_write_enforce() accepts string representation of an integer v=
alue, 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 >=3D 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_use=
r_nul() for
> count < PAGE_SIZE * 8 bytes is "never fails with -ENOMEM unless SIGKILL=
ed by the OOM
> killer". Also, memdup_user_nul() for count >=3D PAGE_SIZE * (1 << MAX_O=
RDER) - 1 bytes is
> "never succeeds". Thus, you can safely add
>
> 	if (count >=3D 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 ca=
ller 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 sa=
ne
> 	 * so bail out early if the request is out of bound.
> 	 */
> 	if (unlikely(order >=3D MAX_ORDER)) {
> 		WARN_ON_ONCE(!(gfp_mask & __GFP_NOWARN));
> 		return NULL;
> 	}
>
> path. memdup_user_nul() side should do
>
> 	if (count >=3D PAGE_SIZE * (1 << MAX_ORDER) - 1)
> 		return -ENOMEM;
>
> check and return -ENOMEM if memdup_user_nul() does not want to use __GF=
P_NOWARN.
> I still believe that memdup_user_nul() side should be fixed.
>