Re: Fw: signed kernel modules?

["Rusty Russell (IBM)" <rusty@au1.ibm.com>]

On Tue, 2004-10-12 at 18:35, David Woodhouse wrote:
> We know _precisely_ what the kernel looks at -- we wrote its linker. It
> really isn't that hard.

Write the code.  Then come back and tell me it "isn't that hard".

Let me make this clear: I refuse to include any solution which doesn't
protect against accidental, as well as deliberate, corruption.  This
means your "canonicalization" code has to be very, very paranoid about
not trusting the data until the signature is verified.  The current code
does very simple checks then completely trusts the module contents,
especially the section headers: to make signatures worth anything, your
code must not do this.

Here's the level of paranoia required for the simplest case, that of
signing the entire module.  The last Howells patches I saw didn't even
do any of *this*, let alone checking the rest of the module:

+static int in_range(Elf_Ehdr *hdr,
+		    unsigned long len,
+		    unsigned long offset,
+		    unsigned long elemsize,
+		    unsigned long num)
+{
+	/* We're careful with wrap here. */
+	if (offset > len)
+		return 0;
+	if (elemsize * num / num != elemsize)
+		return 0;
+	if (elemsize * num > len - offset)
+		return 0;
+	return 1;
+}
+
+/* Lots of checking: we don't trust anything until signature matched. */
+static int check_modsig(Elf_Ehdr *hdr, unsigned long len)
+{
+	Elf_Shdr *sechdrs;
+	unsigned long i, stroff;
+
+	/* Section headers must all be in range. */
+	if (!in_range(hdr, len, hdr->e_shoff, sizeof(Elf_Shdr), hdr->e_shnum))
+		return -EINVAL;
+
+	/* Index of section which contains headers must be good. */
+	if (hdr->e_shstrndx >= hdr->e_shnum)
+		return -EINVAL;
+
+	sechdrs = (void *)hdr + hdr->e_shoff;
+	stroff = sechdrs[hdr->e_shstrndx].sh_offset;
+
+	for (i = 1; i < hdr->e_shnum; i++) {
+		if (!in_range(hdr, len, stroff+sechdrs[i].sh_name, 1,
+			      sizeof("module_sig")))
+			continue;
+		if (strcmp((char *)hdr + stroff+sechdrs[i].sh_name,
+			   "module_sig") != 0)
+			continue;
+		if (!in_range(hdr, len, sechdrs[i].sh_offset,
+			      sechdrs[i].sh_size, 1))
+			return -EINVAL;
+		return calc_signature(hdr, len, sechdrs[i].sh_offset,
+				      sechdrs[i].sh_size);
+	}
+	tainted |= TAINT_FORCED_MODULE;
+	return 0;
+}
+#else
+static int check_modsig(Elf_Ehdr *hdr, unsigned int len)
+{
+	return 0;
+}
+#endif /* CONFIG_MODULE_SIG */
+
 #ifdef CONFIG_SMP
 /* Number of blocks used and allocated. */
 static unsigned int pcpu_num_used, pcpu_num_allocated;
@@ -1522,6 +1614,9 @@ static struct module *load_module(void _
 	if (len < hdr->e_shoff + hdr->e_shnum * sizeof(Elf_Shdr))
 		goto truncated;
 
+	if ((err = check_modsig(hdr, len)) != 0)
+		goto free_hdr;
+
 	/* Convenience variables */
 	sechdrs = (void *)hdr + hdr->e_shoff;
 	secstrings = (void *)hdr + sechdrs[hdr->e_shstrndx].sh_offset;

> > Nor do I have to re-iterate the points from the discussion for someone
> > who hasn't bothered reading it.  But I did.
> 
> Sorry, I didn't think the discussion had been in public. While I'm sure
> I _could_ read mail in David's inbox, I feel it would be somewhat
> impolite. It's not that I "haven't bothered". :)

Sorry, thought you were CC'd the whole time.  My mistake.

Rusty.