Re: KASSERT in exec_elf.c for DYN executable when p_align==0
On Mar 18, 8:15am, al...@yandex.ru (Alexander Nasonov) wrote: -- Subject: Re: KASSERT in exec_elf.c for DYN executable when p_align==0 | Christos Zoulas wrote: | > In article <20180317225722.GA1538@neva>, | > Alexander Nasonov <al...@yandex.ru> wrote: | > >Coverity (CID 1427746) complains about a division by zero when | > >align is 0 in all PT_LOAD headers. | > >... | > >I would be nice to perform sanity checks of tainted executable | > >instead of panicing. | > | > Fixed, thanks. | | But it doesn't fix CID 1427746. Given that both 0 and 1 specify no alignment, | the fix is simple: | | - for (align = i = 0; i < eh->e_phnum; i++) | + align = 1; | + for (i = 0; i < eh->e_phnum; i++) | if (ph[i].p_type == PT_LOAD && ph[i].p_align > align) | align = ph[i].p_align; I missed that part, sorry. christos
Re: KASSERT in exec_elf.c for DYN executable when p_align==0
Christos Zoulas wrote: > In article <20180317225722.GA1538@neva>, > Alexander Nasonovwrote: > >Coverity (CID 1427746) complains about a division by zero when > >align is 0 in all PT_LOAD headers. > >... > >I would be nice to perform sanity checks of tainted executable > >instead of panicing. > > Fixed, thanks. But it doesn't fix CID 1427746. Given that both 0 and 1 specify no alignment, the fix is simple: - for (align = i = 0; i < eh->e_phnum; i++) + align = 1; + for (i = 0; i < eh->e_phnum; i++) if (ph[i].p_type == PT_LOAD && ph[i].p_align > align) align = ph[i].p_align; Alex
Re: KASSERT in exec_elf.c for DYN executable when p_align==0
In article <20180317225722.GA1538@neva>, Alexander Nasonovwrote: >Coverity (CID 1427746) complains about a division by zero when >align is 0 in all PT_LOAD headers. > >I tried reproducing the problem but the code in question is inside >'if (offset < epp->ep_vm_minaddr)' and it isn't easily reproducable. > >However, I hit KASSERT panic: > >"(offset & (align - 1)) == 0" file sys/kern/exec_elf.c, line 139. > >Steps to reproduce (on amd64 compiled with MKPIE=yes): > >bvi -s 0x0e2 /bin/echo # change 20 to 00 >bvi -s 0x11a /bin/echo # change 20 to 00 > >/bin/echo # boom! > >I would be nice to perform sanity checks of tainted executable >instead of panicing. Fixed, thanks. christos
Re: KASSERT in exec_elf.c for DYN executable when p_align==0
Alexander Nasonov wrote: > Steps to reproduce (on amd64 compiled with MKPIE=yes): > > bvi -s 0x0e2 /bin/echo # change 20 to 00 > bvi -s 0x11a /bin/echo # change 20 to 00 > > /bin/echo # boom! > > I would be nice to perform sanity checks of tainted executable > instead of panicing. Attached is a simple patch. I don't know (yet) if it works. Alex Index: exec_elf.c === RCS file: /cvsroot/src/sys/kern/exec_elf.c,v retrieving revision 1.94 diff -p -u -u -r1.94 exec_elf.c --- exec_elf.c 17 Mar 2018 00:30:50 - 1.94 +++ exec_elf.c 17 Mar 2018 23:10:43 - @@ -129,7 +129,8 @@ elf_placedynexec(struct exec_package *ep Elf_Addr align, offset; int i; - for (align = i = 0; i < eh->e_phnum; i++) + align = 1; + for (i = 0; i < eh->e_phnum; i++) if (ph[i].p_type == PT_LOAD && ph[i].p_align > align) align = ph[i].p_align; @@ -679,6 +680,12 @@ exec_elf_makecmds(struct lwp *l, struct for (i = 0; i < eh->e_phnum; i++) { pp = [i]; + if (pp->p_type == PT_LOAD && + (pp->p_align & (pp->p_align - 1)) != 0) { + DPRINTF("bad alignment %#jx", (uintmax_t)pp->p_align); + error = ENOEXEC; + goto bad; + } if (pp->p_type == PT_INTERP) { if (pp->p_filesz < 2 || pp->p_filesz > MAXPATHLEN) { DPRINTF("bad interpreter namelen %#jx",
KASSERT in exec_elf.c for DYN executable when p_align==0
Coverity (CID 1427746) complains about a division by zero when align is 0 in all PT_LOAD headers. I tried reproducing the problem but the code in question is inside 'if (offset < epp->ep_vm_minaddr)' and it isn't easily reproducable. However, I hit KASSERT panic: "(offset & (align - 1)) == 0" file sys/kern/exec_elf.c, line 139. Steps to reproduce (on amd64 compiled with MKPIE=yes): bvi -s 0x0e2 /bin/echo # change 20 to 00 bvi -s 0x11a /bin/echo # change 20 to 00 /bin/echo # boom! I would be nice to perform sanity checks of tainted executable instead of panicing. -- Alex