Re: [PATCH v3] mm: memdup_user*() should use same gfp flags

[Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>]

On 2021/01/28 8:27, Casey Schaufler wrote:
>>> There is no point with allowing userspace to allocate 2GB of physically non-contiguous
>>> memory using kvmalloc(). Size is controlled by userspace, and memdup_user_nul() is used
>>> for allocating temporary memory which will be released before returning to userspace.
>>>
>>> Sane userspace processes should allocate only one or a few pages using memdup_user_nul().
>>> Just making insane user processes (like fuzzer) fail memory allocation requests is a
>>> reasonable decision.
>> (cc Casey)
>>
>> I'd say that the immediate problem is in smk_write_syslog().  Obviously
>> it was implemented expecting small writes, but the fuzzer is passing it a
>> huge write and things fall apart.
> 
> Yes, Smack should be checking that. Patch is in the works.

Caller of memdup_user_nul() is responsible for making sure that size != -1 in order to
avoid integer overflow overlooked by kmalloc(0) != NULL because memdup_user_nul() allocates
size + 1 bytes. And this is automatically made sure for smackfs because vfs_write() makes
sure that size <= (INT_MAX & PAGE_MASK) bytes.

But some legitimate userspace might be already doing "write(fd, buffer, 20000);" for
smk_write_onlycap()/smk_write_relabel_self(). How can you guarantee that introducing
upper limit on the caller side does not break existing userspace tools?

If some caller wants size > 32767 for memdup_user_nul(), it is just a matter of introducing
vmemdup_user_nul(). memdup_user() and memdup_user_nul() had better behave similarly.
There is no reason to use different gfp flags between memdup_user() and memdup_user_nul().

> I hates fuzzers.

A surprising comment from security person. Smack is free to opt out of syzbot testing. :-)