Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S270025AbUJNLDT (ORCPT ); Thu, 14 Oct 2004 07:03:19 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S270026AbUJNLDT (ORCPT ); Thu, 14 Oct 2004 07:03:19 -0400 Received: from mx1.redhat.com ([66.187.233.31]:18616 "EHLO mx1.redhat.com") by vger.kernel.org with ESMTP id S270025AbUJNLDO (ORCPT ); Thu, 14 Oct 2004 07:03:14 -0400 From: David Howells In-Reply-To: <1097707986.14303.36.camel@localhost.localdomain> References: <1097707986.14303.36.camel@localhost.localdomain> <1097626296.4013.34.camel@localhost.localdomain> <1096544201.8043.816.camel@localhost.localdomain> <1096411448.3230.22.camel@localhost.localdomain> <1092403984.29463.11.camel@bach> <1092369784.25194.225.camel@bach> <20040812092029.GA30255@devserv.devel.redhat.com> <20040811211719.GD21894@kroah.com> <1092097278.20335.51.camel@bach> <20040810002741.GA7764@kroah.com> <1092189167.22236.67.camel@bach> <19388.1092301990@redhat.com> <30797.1092308768@redhat.com> <20040812111853.GB25950@devserv.devel.redhat.com> <20040812200917.GD2952@kroah.com> <26280.1092388799@redhat.com> <27175.1095936746@redhat.com> <30591.1096451074@redhat.com> <10345.1097507482@redhat.com> <1097507755.318.332.camel@hades.cambridge.redhat.com> <1097534090.16153.7.camel@localhost.localdomain> <1097570159.5788.1089.camel@baythorne.infradead.org> <27277.1097702318@redhat.com> To: Rusty Russell Cc: David Woodhouse , Greg KH , Arjan van de Ven , Joy Latten , lkml - Kernel Mailing List Subject: Re: Fw: signed kernel modules? User-Agent: EMH/1.14.1 SEMI/1.14.5 (Awara-Onsen) FLIM/1.14.5 (Demachiyanagi) APEL/10.6 Emacs/21.3 (i386-redhat-linux-gnu) MULE/5.0 (SAKAKI) MIME-Version: 1.0 (generated by SEMI 1.14.5 - "Awara-Onsen") Content-Type: text/plain; charset=US-ASCII Date: Thu, 14 Oct 2004 12:02:52 +0100 Message-ID: <16127.1097751772@redhat.com> Sender: linux-kernel-owner@vger.kernel.org X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2527 Lines: 83 > 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 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/