Re: [PATCH 1/1] ubifs: support authentication, for ro mount, when no key is given

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

On Fri, Jun 26, 2020 at 09:27:14AM +0200, Torben Hohn wrote:
> On Fri, Jun 26, 2020 at 06:31:20AM +0200, Sascha Hauer wrote:
> > Hi Torben,
> > 
> > On Thu, Jun 25, 2020 at 05:59:27PM +0200, Torben Hohn wrote:
> > > diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
> > > index 7fc2f3f07c16..ec95f1f50e5e 100644
> > > --- a/fs/ubifs/super.c
> > > +++ b/fs/ubifs/super.c
> > > @@ -1291,6 +1291,17 @@ static int mount_ubifs(struct ubifs_info *c)
> > >  			err = -EINVAL;
> > >  			goto out_free;
> > >  		}
> > > +	} else if (c->auth_hash_name) {
> > > +		if (IS_ENABLED(CONFIG_UBIFS_FS_AUTHENTICATION)) {
> > > +			err = ubifs_init_authentication_read_only(c);
> > > +			if (err)
> > > +				goto out_free;
> > > +		} else {
> > > +			ubifs_err(c, "auth_hash_name, but UBIFS is built without"
> > > +				  " authentication support");
> > > +			err = -EINVAL;
> > > +			goto out_free;
> > > +		}
> > >  	}
> > 
> > In case we don't have a key available for HMAC and can only verify the
> > FS is correctly signed then we have to be sure that we are mounting
> > readonly. This means the above needs an additional check for
> > c->ro_mount.
> 
> Indeed, i had that check in authenticate_sb_node() in an earlier
> version, and forgot to add it here.
> 
> Will do.
> 
> > 
> > Once we can be sure that UBIFS is in readonly mode when we can't do HMAC
> > then there's no point in adding a ubifs_authenticated_write(), because
> > the places where you call it will never be hit in a readonly mounted
> > filesystem.
> 
> The point is making sure, that it really is never hit in a readonly
> filesystem. Now, and in the future. If we miss one point, we might
> trigger the hmac code with an empty hmac. Although it might just crash.

If that's your point then you can add a ubifs_assert(c, c->ro_mount) at
those places. This has the advantage that it triggers not only in
authenticated mode, but also in unauthenticated mode. Please add this
assertion explicitly and not indirectly in ubifs_authenticated_write().
This function has a strange semantics, the name suggests that it returns
the status of authenticated write. It's quite unexpected to me that it
triggers a warning when called with only readonly authentication
available.

Regards,
 Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
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/