lkml.org 
[lkml]   [2004]   [Oct]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: Fw: signed kernel modules?
Date

> I'd prefer to see:
> err = module_verify(hdr, len, &gpgsig_ok);
> if (err)
> goto free_hdr;

I've been moaned at for doing this before. Other people have told me they
prefer to see the value returned through the return value since there's enough
scope.

> And then have module_verify for the !CONFIG_MODULE_SIG case (in
> module-verify.h) simply be:

I think it should still check the ELF, even if we're not going to check a
signature. This permits us to drop a few checks later in the module loading
process.

> + tmp = (size_t) hdr->e_shentsize * (size_t) hdr->e_shnum;
> + elfcheck(tmp < size - hdr->e_shoff);
>
> Multiplicative overflow.

Not so in this ELF incarnation. The multiply parameters are both 16-bit values
which I cast to 32-bit values before multiplying. I could, I suppose, put
checks on this.

I've added a check to make sure hdr->e_shnum is less than SHN_LORESERVE.

> Also check that hdr->e_shentsize is sizeof(Elf_Shdr) since you assume that
> below.

Added.

> + mvdata->secsizes = kmalloc(hdr->e_shnum * sizeof(size_t), GFP_KERNEL);
> + memset(mvdata->secsizes, 0, hdr->e_shnum * sizeof(size_t));
>
> Multiplicative overflow again: we could kmalloc 0 bytes and overflow below.

A 16-bit value multiplied by a 32/64-bit value which 4 or 8. Where's the
overflow?

Try compiling and running:

#include <stdio.h>
int main() { printf("%zu\n", sizeof(sizeof(char))); return 0; }
> + secstop = mvdata->sections + mvdata->nsects;
>
> Subtler multiplicative overflow.

There's already a check in to make sure it won't overflow, given the
additional checks to limit e_shnum (which is unsigned 16 bits) and that
e_shentsize is correct.

> + if (section->sh_entsize > 0)
> + seccheck(section->sh_size % section->sh_entsize == 0);
>
> Divide by zero (thanks Alan!).

Not so. Look more closely, particularly at the if-statement.

> I think you have to check (as above) that st_name is nul terminated
> within size.

Added.

> I think you can overflow here. For REL and RELA sections, you don't
> check that sh_size is <= *secsize.

I've added checks that the sh_entsize is what I'm expecting. There's already a
check that the section size divides exactly by the ent-size (you claimed it
had a div-by-0 error above).

> That's all I found,

Thanks.

David
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2005-03-22 14:07    [from the cache]
©2003-2011 Jasper Spaans