Re: Fw: signed kernel modules?

[David Howells <dhowells@redhat.com>]

> 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