Re: [PATCH] ubifs: support offline signed images

[Sascha Hauer <s.hauer@pengutronix.de>]

Hi Richard,

Thanks for review.

On Sun, May 05, 2019 at 11:13:38PM +0200, Richard Weinberger wrote:
> ----- Ursprüngliche Mail -----
> > Von: "Sascha Hauer" <s.hauer@pengutronix.de>
> > An: "linux-mtd" <linux-mtd@lists.infradead.org>
> > CC: "richard" <richard@nod.at>, kernel@pengutronix.de, "Sascha Hauer" <s.hauer@pengutronix.de>
> > Gesendet: Montag, 1. April 2019 16:30:01
> > Betreff: [PATCH] ubifs: support offline signed images
> 
> > HMACs can only be generated on the system the UBIFS image is running on.
> 
> Because at mkfs.ubifs time you don't have the key which might be hardware
> specific, right?

Yes, right.

> > +int ubifs_sb_verify_signature(struct ubifs_info *c,
> > +			      const struct ubifs_sb_node *sup)
> > +{
> > +	int err;
> > +	struct ubifs_scan_leb *sleb;
> > +	struct ubifs_scan_node *snod;
> > +	const struct ubifs_sig_node *signode;
> > +
> > +	err = ubifs_leb_read(c, UBIFS_SB_LNUM, c->sbuf, 0, c->leb_size, 1);
> 
> This line looks wrong here, leftover from old code? :)

Yup, can be removed.

> 
> > +	sleb = ubifs_scan(c, UBIFS_SB_LNUM, UBIFS_SB_NODE_SZ, c->sbuf, 0);
> > +	if (IS_ERR(sleb)) {
> > +		err = PTR_ERR(sleb);
> > +		return err;
> > +	}
> > +
> > +	if (sleb->nodes_cnt == 0) {
> > +		ubifs_err(c, "Unable to find signature node");
> > +		err = -EINVAL;
> > +		goto out_destroy;
> > +	}
> > +
> > +	snod = list_entry(sleb->nodes.next, struct ubifs_scan_node, list);
> 
> list_first_entry()?

Yes.

> 
> > +	if (snod->type != UBIFS_SIG_NODE) {
> > +		ubifs_err(c, "Signature node is of wrong type");
> > +		err = -EINVAL;
> > +		goto out_destroy;
> > +	}
> > +
> > +	signode = snod->node;
> > +
> > +	err = verify_pkcs7_signature(sup, sizeof(struct ubifs_sb_node),
> > +				     signode->sig, le32_to_cpu(signode->len),
> 
> signode->len is not verified, please embed a check on the length.

Ok.

> 
> > +				     NULL, VERIFYING_UNSPECIFIED_SIGNATURE,
> > +				     NULL, NULL);
> > +
> > +	if (err)
> > +		ubifs_err(c, "Failed to verify signature");
> > +	else
> > +		ubifs_msg(c, "Successfully verified super block signature");
> 
> While this is good news, is it really worth telling the user every time?

No, will remove.

> 
> > +out_destroy:
> > +	ubifs_scan_destroy(sleb);
> > +
> > +	return err;
> > +}
> > +
> > /**
> >  * ubifs_init_authentication - initialize UBIFS authentication support
> >  * @c: UBIFS file-system description object
> > @@ -499,3 +561,22 @@ int ubifs_hmac_wkm(struct ubifs_info *c, u8 *hmac)
> > 		return err;
> > 	return 0;
> > }
> > +
> > +/*
> > + * ubifs_hmac_zero - test if a HMAC is zero
> > + * @c: UBIFS file-system description object
> > + * @hmac: the HMAC to test
> > + *
> > + * This function tests if a HMAC is zero and returns true if it is
> > + * and false otherwise.
> > + */
> > +bool ubifs_hmac_zero(struct ubifs_info *c, const u8 *hmac)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < c->hmac_desc_len; i++)
> > +		if (hmac[i] != 0)
> > +			return false;
> > +
> > +	return true;
> > +}
> 
> Isn't there an helper available?
> Maybe based on memcmp()?

Indeed there's memchr_inv() - Find an unmatching character in an area of memory.

> 
> > diff --git a/fs/ubifs/master.c b/fs/ubifs/master.c
> > index 5ea51bbd14c7..5655414a76a7 100644
> > --- a/fs/ubifs/master.c
> > +++ b/fs/ubifs/master.c
> > @@ -60,6 +60,40 @@ int ubifs_compare_master_node(struct ubifs_info *c, void *m1,
> > void *m2)
> > 	return 0;
> > }
> > 
> > diff --git a/fs/ubifs/sb.c b/fs/ubifs/sb.c
> > index 67fac1e8adfb..3dd195dbd852 100644
> > --- a/fs/ubifs/sb.c
> > +++ b/fs/ubifs/sb.c
> > @@ -761,18 +770,12 @@ int ubifs_read_superblock(struct ubifs_info *c)
> > 	c->old_leb_cnt = c->leb_cnt;
> > 	if (c->leb_cnt < c->vi.size && c->leb_cnt < c->max_leb_cnt) {
> > 		c->leb_cnt = min_t(int, c->max_leb_cnt, c->vi.size);
> > -		if (c->ro_mount)
> > -			dbg_mnt("Auto resizing (ro) from %d LEBs to %d LEBs",
> > -				c->old_leb_cnt,	c->leb_cnt);
> > -		else {
> > -			dbg_mnt("Auto resizing (sb) from %d LEBs to %d LEBs",
> > -				c->old_leb_cnt, c->leb_cnt);
> > -			sup->leb_cnt = cpu_to_le32(c->leb_cnt);
> > -			err = ubifs_write_sb_node(c, sup);
> > -			if (err)
> > -				goto out;
> > -			c->old_leb_cnt = c->leb_cnt;
> > -		}
> > +		sup->leb_cnt = cpu_to_le32(c->leb_cnt);
> > +
> > +		c->superblock_need_write = 1;
> > +
> > +		dbg_mnt("Auto resizing from %d LEBs to %d LEBs",
> > +			c->old_leb_cnt,	c->leb_cnt);
> 
> Hmm, I don't fully understand the logic here.
> What happens to c->old_leb_cnt and the ro_mount case?

c->old_leb_cnt is only used by the code to determine if the leb_cnt has
changed, i.e. in ubifs_remount_rw() we have:

	if (c->old_leb_cnt != c->leb_cnt) {
		...
		err = ubifs_write_sb_node(c, sup);
	}

We now have the explicit c->superblock_need_write flag which has the
same purpose. c->old_leb_cnt is no longer used, it should have been
removed in this patch, but I missed it. Will do in v2.

In the ro_mount case nothing changes really. The code will set
c->superblock_need_write and based on that flag we will write the
updated superblock when we are going readwrite later. The same happened
before this patch based on (c->old_leb_cnt != c->leb_cnt), only that
I now the master node is written first and the sb_node afterwards.

> > + * struct ubifs_sig_node - node for signing other nodes
> > + * @ch: common header
> > + * @len: The length of the signature data
> > + * @sig: The signature data
> > + */
> > +struct ubifs_sig_node {
> > +	struct ubifs_ch ch;
> > +	__le32 len;
> > +	__u8 sig[];
> > +} __packed;
> > +
> 
> Can we please have a version or type field?
> Just in case we want to support more than PKCS#7 at some point.
> This new node is not used at lot, so we can waste a little space...

Sure, I'll add a type field.

> 
> Did you check, what is the typical length of a signature?
> Maybe adding more padding fields is worth it.

The signature is 670 bytes in my case. It's the x509 file the Kernel
generates for module signing. I don't know how typical that size is.
What I know is that there is no upper limit for the size of a x509 file,
not sure if that can become a problem some day.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/