`On Wed, 23 May 2012 15:20:47 +0100, David Howells <dhowells@redhat.com> wrote:> Rusty Russell <rusty@rustcorp.com.au> wrote:> > >         That's pretty weird.  Why not put the "@This Is A Crypto Signed> > Module\n" before the signature?  Then module-size is implied: everything> > before that signature.  The signature size is implied: everything after> > that signature.> > This makes it simpler.  No scanning required.  The magic number can only be in> one place and you can find it by dead reckoning.Scanning isn't complicated.  Slow, sure, but I doubt you can reallymeasure it when you're doing crypto.Compare:(1) Your scheme signing looks something like this:        gpg --sign \$m > \$m.sig        MSIZE=`ls -l \$m | awk '{ print \$5 }'`        SSIZE=`ls -l \$m.sig | awk '{ print \$5 }'`                printf '@%-10i@%-10i@This Is A Crypto Signed Module' \$MSIZE \$SSIZE >> \$m(2) Your verification scheme looks like this: +	magic_size = sizeof(modsign_magic) - 1; +	if (size <= 11 + 11 + magic_size) +		return 1; + +	if (memcmp(data + size - magic_size, modsign_magic, magic_size) != 0) +		return 1; +	size -= 11 + 11 + magic_size; + +	cp = data + size; +	if (cp[ 0] != '@' && cp[ 9] != '@' && cp[10] != '\n' && +	    cp[11] != '@' && cp[20] != '@' && cp[21] != '\n') +		return -ELIBBAD; +	mod_size = simple_strtoul(cp + 1, &end, 10); +	if (mod_size > size || (*end != ' ' && *end != '@')) +		return -ELIBBAD; +	sig_size = simple_strtoul(cp + 12, &end, 10); +	if (sig_size > size || (*end != ' ' && *end != '@')) +		return -ELIBBAD; + +	pr_devel("sig at %zu, size %zu (to %zu)\n", mod_size, sig_size, size); +	if (size - mod_size != sig_size) +		return -ELIBBAD;Now, the scheme I suggested looks like this:(1) Signing:        gpg --sign \$m > \$m.sig        (echo @This Is A Crypto Signed Module; cat \$m.sig) >> \$m(2) Verification:        size_t i;        if (i < modsign_magic)                return 1;        for (i = size - modsign_magic;             memcmp(data + i, modsign_magic, magic_size) != 0);             i++) {                if (i == 0)                        return 1;        }        /* module: "data", size "i".         * sig: "data + i + magic_size", size "size - (i + magic_size)" */> >         In fact, I'd modify this slightly, to allow multiple signatures.> > This would work nicely with a deterministic strip.  Find the signatures> > backward, and truncate as they fail.> > Why would you want multiple signatures?  That just complicates things.The code above stays pretty simple; if the signature fails, you set sizeto i, and loop again.  As I said, if you know exactly how you're goingto strip the modules, you can avoid storing the stripped module andsimply append both signatures.> If you're in FIPS mode, you probably have to panic if any of them fail.I had to look up what FIPS was, so I'm not qualified to comment.> I suppose I may as well punt the signature detection and removal to userspace> and pass the signature as an argument to init_module() as Dmitry suggested.> Then the signature need not be in the file at all (he wants to use an xattr or> hardware, I think).  mkinitrd and rpmbuild/kernel spec have to be changed to> accommodate enablement of these patches, so why not module-init-tools, dracut> and busybox whilst we're at it?In some ways that is cleaner, but it's also nice to avoid adding anothersyscall.Cheers,Rusty.`