Re: KASSERT in exec_elf.c for DYN executable when p_align==0

[Christos Zoulas]

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

[Alexander Nasonov]

Christos Zoulas wrote:
> In article <20180317225722.GA1538@neva>,
> Alexander Nasonov   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;

Alex

Re: KASSERT in exec_elf.c for DYN executable when p_align==0

[Christos Zoulas]

In article <20180317225722.GA1538@neva>,
Alexander Nasonov   wrote:
>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]

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

[Alexander Nasonov]

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