brk() in v6.1-rc1 can expand file mappings, seemingly without taking file locks

brk() in v6.1-rc1 can expand file mappings, seemingly without taking file locks

[Jann Horn]

As of commit ca57f02295f, brk() can expand ordinary file mappings (but
not file mappings with weird flags), and I think it does it with
insufficient locks. I think brk() probably needs some extra checks to
make sure it's operating on a brk-like VMA (which means it should at
least be anonymous, and perhaps pass the full can_vma_merge_after()
check so that we're not creating unnecessary special cases?).

user@vm:~/brk_stretch$ cat brk_file.c
#define _GNU_SOURCE
#include <unistd.h>
#include <stdio.h>
#include <fcntl.h>
#include <err.h>
#include <stdlib.h>
#include <malloc.h>
#include <sys/auxv.h>
#include <sys/mman.h>
#include <sys/syscall.h>

#define SYSCHK(x) ({          \
  typeof(x) __res = (x);      \
  if (__res == (typeof(x))-1) \
    err(1, "SYSCHK(" #x ")"); \
  __res;                      \
})

int main(void) {
  mallopt(M_MMAP_THRESHOLD, 0);

  void *brk_space = sbrk(0x2000);
  if (brk_space == NULL)
    errx(1, "sbrk() fail");
  printf("brk_space = %p\n", brk_space);
  int fd = SYSCHK(open("/etc/services", O_RDONLY));
  void *map = SYSCHK(mmap(brk_space, 0x2000, PROT_READ|PROT_WRITE,
MAP_PRIVATE|MAP_FIXED, fd, 0));

  /* stretch */
  if (sbrk(0x111000) == NULL)
    err(1, "sbrk");
  printf("sbrk() success\n");

  system("cat /proc/$PPID/maps");

  return 0;
}
user@vm:~/brk_stretch$ gcc -o brk_file brk_file.c
user@vm:~/brk_stretch$ ./brk_file
brk_space = 0x557f71b5d000
sbrk() success
557f70616000-557f70617000 r--p 00000000 fd:00 2752938
  /home/user/brk_stretch/brk_file
557f70617000-557f70618000 r-xp 00001000 fd:00 2752938
  /home/user/brk_stretch/brk_file
557f70618000-557f70619000 r--p 00002000 fd:00 2752938
  /home/user/brk_stretch/brk_file
557f70619000-557f7061a000 r--p 00002000 fd:00 2752938
  /home/user/brk_stretch/brk_file
557f7061a000-557f7061b000 rw-p 00003000 fd:00 2752938
  /home/user/brk_stretch/brk_file
557f71b5d000-557f71c70000 rw-p 00000000 fd:00 2621496
  /etc/services
7fd67993d000-7fd67995f000 r--p 00000000 fd:00 527268
  /usr/lib/x86_64-linux-gnu/libc-2.28.so
7fd67995f000-7fd679aa6000 r-xp 00022000 fd:00 527268
  /usr/lib/x86_64-linux-gnu/libc-2.28.so
7fd679aa6000-7fd679af2000 r--p 00169000 fd:00 527268
  /usr/lib/x86_64-linux-gnu/libc-2.28.so
7fd679af2000-7fd679af3000 ---p 001b5000 fd:00 527268
  /usr/lib/x86_64-linux-gnu/libc-2.28.so
7fd679af3000-7fd679af7000 r--p 001b5000 fd:00 527268
  /usr/lib/x86_64-linux-gnu/libc-2.28.so
7fd679af7000-7fd679af9000 rw-p 001b9000 fd:00 527268
  /usr/lib/x86_64-linux-gnu/libc-2.28.so
7fd679af9000-7fd679aff000 rw-p 00000000 00:00 0
7fd679b16000-7fd679b18000 rw-p 00000000 00:00 0
7fd679b18000-7fd679b19000 r--p 00000000 fd:00 527258
  /usr/lib/x86_64-linux-gnu/ld-2.28.so
7fd679b19000-7fd679b37000 r-xp 00001000 fd:00 527258
  /usr/lib/x86_64-linux-gnu/ld-2.28.so
7fd679b37000-7fd679b3f000 r--p 0001f000 fd:00 527258
  /usr/lib/x86_64-linux-gnu/ld-2.28.so
7fd679b3f000-7fd679b40000 r--p 00026000 fd:00 527258
  /usr/lib/x86_64-linux-gnu/ld-2.28.so
7fd679b40000-7fd679b41000 rw-p 00027000 fd:00 527258
  /usr/lib/x86_64-linux-gnu/ld-2.28.so
7fd679b41000-7fd679b42000 rw-p 00000000 00:00 0
7ffd58087000-7ffd580a8000 rw-p 00000000 00:00 0                          [stack]
7ffd581fa000-7ffd581fe000 r--p 00000000 00:00 0                          [vvar]
7ffd581fe000-7ffd58200000 r-xp 00000000 00:00 0                          [vdso]
user@vm:~/brk_stretch$

The codepaths that are intended to expand file VMAs do stuff like
i_mmap_lock_write() and vma_interval_tree_remove(), which
do_brk_flags() doesn't seem to do (because it was never intended to
operate on file VMAs?).

Re: brk() in v6.1-rc1 can expand file mappings, seemingly without taking file locks

[Jann Horn]

On Fri, Dec 2, 2022 at 7:53 PM Jann Horn <jannh@google.com> wrote:
> As of commit ca57f02295f, brk() can expand ordinary file mappings (but

Sorry, that was worded confusingly - I meant "ca57f02295f is the
commit from Linus' tree on top of which I was testing".

The broken code seems to have been introduced in
commit 2e7ce7d354f2 ("mm/mmap: change do_brk_flags() to expand
existing VMA and add do_brk_munmap()").

Re: brk() in v6.1-rc1 can expand file mappings, seemingly without taking file locks

[Liam Howlett]

* Jann Horn <jannh@google.com> [221202 13:54]:
> As of commit ca57f02295f, brk() can expand ordinary file mappings (but
> not file mappings with weird flags), and I think it does it with
> insufficient locks. I think brk() probably needs some extra checks to
> make sure it's operating on a brk-like VMA (which means it should at
> least be anonymous, and perhaps pass the full can_vma_merge_after()
> check so that we're not creating unnecessary special cases?).


Thanks.  This is probably caused by commit 2e7ce7d354f2: "mm/mmap:
change do_brk_flags() to expand existing VMA and add do_brk_munmap()"

Specifically the checks around expanding the VMA. 

> user@vm:~/brk_stretch$ cat brk_file.c

Thanks for the testcase.  I have a fix that I'm testing, but it's worth
noting that the brk call will succeed - except a new VMA will be
created.  Is this what you expect?

...
> 
> The codepaths that are intended to expand file VMAs do stuff like
> i_mmap_lock_write() and vma_interval_tree_remove(), which
> do_brk_flags() doesn't seem to do (because it was never intended to
> operate on file VMAs?).

I don't think the locks were there before and I think you are correct
that it wasn't intended to operate on file VMAs.  I made sure all the
ltp tests around brk pass with my changes but this seems insufficient.

Re: brk() in v6.1-rc1 can expand file mappings, seemingly without taking file locks

[Jann Horn]

On Mon, Dec 5, 2022 at 3:50 PM Liam Howlett <liam.howlett@oracle.com> wrote:
> * Jann Horn <jannh@google.com> [221202 13:54]:
> > As of commit ca57f02295f, brk() can expand ordinary file mappings (but
> > not file mappings with weird flags), and I think it does it with
> > insufficient locks. I think brk() probably needs some extra checks to
> > make sure it's operating on a brk-like VMA (which means it should at
> > least be anonymous, and perhaps pass the full can_vma_merge_after()
> > check so that we're not creating unnecessary special cases?).
>
>
> Thanks.  This is probably caused by commit 2e7ce7d354f2: "mm/mmap:
> change do_brk_flags() to expand existing VMA and add do_brk_munmap()"

Yeah.

> Specifically the checks around expanding the VMA.
>
> > user@vm:~/brk_stretch$ cat brk_file.c
>
> Thanks for the testcase.  I have a fix that I'm testing, but it's worth
> noting that the brk call will succeed - except a new VMA will be
> created.  Is this what you expect?

Yes, that's what I would expect.