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