| Message ID | 20180705145539.9627-1-osalvador@techadventures.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 5, 2018 at 7:55 AM, <osalvador@techadventures.net> wrote: > From: Oscar Salvador <osalvador@suse.de> > > The current code does not make sure to page align bss before calling > vm_brk(), and this can lead to a VM_BUG_ON() in __mm_populate() > due to the requested lenght not being correctly aligned. > > Let us make sure to align it properly. > > Signed-off-by: Oscar Salvador <osalvador@suse.de> > Tested-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> > Reported-by: syzbot+5dcb560fe12aa5091c06@syzkaller.appspotmail.com Wow. CONFIG_USELIB? I'm surprised distros are still using this. 32-bit only, and libc5 and earlier only. Regardless, this appears to match the current bss alignment logic in the main elf loader, so: Acked-by: Kees Cook <keescook@chromium.org> -Kees > --- > fs/binfmt_elf.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c > index 0ac456b52bdd..816cc921cf36 100644 > --- a/fs/binfmt_elf.c > +++ b/fs/binfmt_elf.c > @@ -1259,9 +1259,8 @@ static int load_elf_library(struct file *file) > goto out_free_ph; > } > > - len = ELF_PAGESTART(eppnt->p_filesz + eppnt->p_vaddr + > - ELF_MIN_ALIGN - 1); > - bss = eppnt->p_memsz + eppnt->p_vaddr; > + len = ELF_PAGEALIGN(eppnt->p_filesz + eppnt->p_vaddr); > + bss = ELF_PAGEALIGN(eppnt->p_memsz + eppnt->p_vaddr); > if (bss > len) { > error = vm_brk(len, bss - len); > if (error) > -- > 2.13.6 >
On Thu, Jul 05, 2018 at 08:44:18AM -0700, Kees Cook wrote: > On Thu, Jul 5, 2018 at 7:55 AM, <osalvador@techadventures.net> wrote: > > From: Oscar Salvador <osalvador@suse.de> > > > > The current code does not make sure to page align bss before calling > > vm_brk(), and this can lead to a VM_BUG_ON() in __mm_populate() > > due to the requested lenght not being correctly aligned. > > > > Let us make sure to align it properly. > > > > Signed-off-by: Oscar Salvador <osalvador@suse.de> > > Tested-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> > > Reported-by: syzbot+5dcb560fe12aa5091c06@syzkaller.appspotmail.com > > Wow. CONFIG_USELIB? I'm surprised distros are still using this. 32-bit > only, and libc5 and earlier only. > > Regardless, this appears to match the current bss alignment logic in > the main elf loader, so: > > Acked-by: Kees Cook <keescook@chromium.org> > > -Kees > > > --- > > fs/binfmt_elf.c | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c > > index 0ac456b52bdd..816cc921cf36 100644 > > --- a/fs/binfmt_elf.c > > +++ b/fs/binfmt_elf.c > > @@ -1259,9 +1259,8 @@ static int load_elf_library(struct file *file) > > goto out_free_ph; > > } > > > > - len = ELF_PAGESTART(eppnt->p_filesz + eppnt->p_vaddr + > > - ELF_MIN_ALIGN - 1); > > - bss = eppnt->p_memsz + eppnt->p_vaddr; > > + len = ELF_PAGEALIGN(eppnt->p_filesz + eppnt->p_vaddr); > > + bss = ELF_PAGEALIGN(eppnt->p_memsz + eppnt->p_vaddr); > > if (bss > len) { > > error = vm_brk(len, bss - len); > > if (error) > > -- > > 2.13.6 > > CC Andrew Hi Andrew, in case this patch gets accepted, does it have to go through your tree? Or is it for someone else to take it? Thanks
On Fri, Jul 6, 2018 at 12:13 AM, Oscar Salvador <osalvador@techadventures.net> wrote: > On Thu, Jul 05, 2018 at 08:44:18AM -0700, Kees Cook wrote: >> On Thu, Jul 5, 2018 at 7:55 AM, <osalvador@techadventures.net> wrote: >> > From: Oscar Salvador <osalvador@suse.de> >> > >> > The current code does not make sure to page align bss before calling >> > vm_brk(), and this can lead to a VM_BUG_ON() in __mm_populate() >> > due to the requested lenght not being correctly aligned. >> > >> > Let us make sure to align it properly. >> > >> > Signed-off-by: Oscar Salvador <osalvador@suse.de> >> > Tested-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> >> > Reported-by: syzbot+5dcb560fe12aa5091c06@syzkaller.appspotmail.com >> >> Wow. CONFIG_USELIB? I'm surprised distros are still using this. 32-bit >> only, and libc5 and earlier only. >> >> Regardless, this appears to match the current bss alignment logic in >> the main elf loader, so: >> >> Acked-by: Kees Cook <keescook@chromium.org> >> >> -Kees >> >> > --- >> > fs/binfmt_elf.c | 5 ++--- >> > 1 file changed, 2 insertions(+), 3 deletions(-) >> > >> > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c >> > index 0ac456b52bdd..816cc921cf36 100644 >> > --- a/fs/binfmt_elf.c >> > +++ b/fs/binfmt_elf.c >> > @@ -1259,9 +1259,8 @@ static int load_elf_library(struct file *file) >> > goto out_free_ph; >> > } >> > >> > - len = ELF_PAGESTART(eppnt->p_filesz + eppnt->p_vaddr + >> > - ELF_MIN_ALIGN - 1); >> > - bss = eppnt->p_memsz + eppnt->p_vaddr; >> > + len = ELF_PAGEALIGN(eppnt->p_filesz + eppnt->p_vaddr); >> > + bss = ELF_PAGEALIGN(eppnt->p_memsz + eppnt->p_vaddr); >> > if (bss > len) { >> > error = vm_brk(len, bss - len); >> > if (error) >> > -- >> > 2.13.6 >> > > CC Andrew > > Hi Andrew, > > in case this patch gets accepted, does it have to go through your tree? > Or is it for someone else to take it? (FWIW, binfmt_elf changes have traditionally gone through -mm, yes.) -Kees
On Thu, 5 Jul 2018 08:44:18 -0700 Kees Cook <keescook@chromium.org> wrote: > On Thu, Jul 5, 2018 at 7:55 AM, <osalvador@techadventures.net> wrote: > > From: Oscar Salvador <osalvador@suse.de> > > > > The current code does not make sure to page align bss before calling > > vm_brk(), and this can lead to a VM_BUG_ON() in __mm_populate() > > due to the requested lenght not being correctly aligned. > > > > Let us make sure to align it properly. > > > > Signed-off-by: Oscar Salvador <osalvador@suse.de> > > Tested-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> > > Reported-by: syzbot+5dcb560fe12aa5091c06@syzkaller.appspotmail.com > > Wow. CONFIG_USELIB? I'm surprised distros are still using this. 32-bit > only, and libc5 and earlier only. Presumably doesn't happen much, but people who *are* enabling this will want the fix, so I added the cc:stable.
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index 0ac456b52bdd..816cc921cf36 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -1259,9 +1259,8 @@ static int load_elf_library(struct file *file) goto out_free_ph; } - len = ELF_PAGESTART(eppnt->p_filesz + eppnt->p_vaddr + - ELF_MIN_ALIGN - 1); - bss = eppnt->p_memsz + eppnt->p_vaddr; + len = ELF_PAGEALIGN(eppnt->p_filesz + eppnt->p_vaddr); + bss = ELF_PAGEALIGN(eppnt->p_memsz + eppnt->p_vaddr); if (bss > len) { error = vm_brk(len, bss - len); if (error)