On 2021/01/28 8:27, Casey Schaufler wrote:

quoted

quoted

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. :-)