Re: [PATCH v2 8/8] state: backend_raw: add hamc support

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

On Wed, Oct 21, 2015 at 10:56:01AM +0200, Marc Kleine-Budde wrote:
> On 10/21/2015 09:13 AM, Sascha Hauer wrote:
> >> +static int state_backend_raw_file_init_digest(struct state *state, struct state_backend_raw *backend_raw)
> >> +{
> >> +	struct digest *digest;
> >> +	const char *algo;
> >> +	const unsigned char *key;
> >> +	int key_len, ret;
> >> +
> >> +	ret = of_property_read_string(state->root, "algo", &algo);
> > 
> > This needs an update to Documentation/devicetree/bindings/barebox/barebox,state.rst
> 
> ok
> 
> >> +	if (ret == -EINVAL)	/* -EINVAL == does not exist */
> >> +		return 0;
> > 
> > -EINVAL is such a widespread error value. Maybe better explicitly test
> > for existence with of_find_property?
> 
> ok
> 
> >> +	else if (ret)
> >> +		return ret;
> >> +
> >> +	ret = keystore_get_secret(state->name, &key, &key_len);
> >> +	if (ret == -ENOENT)	/* -ENOENT == does not exist */
> >> +		return -EPROBE_DEFER;
> >> +	else if (ret)
> >> +		return ret;
> >> +
> >> +	digest = digest_alloc(algo);
> >> +	if (!digest) {
> >> +		dev_info(&state->dev, "algo %s not found - probe deferred\n", algo);
> >> +		return -EPROBE_DEFER;
> >> +	}
> >> +
> >> +	ret = digest_set_key(digest, key, key_len);
> >> +	if (ret) {
> >> +		digest_free(digest);
> >> +		return ret;
> >> +	}
> >> +
> >> +	backend_raw->backend.digest = digest;
> >> +	backend_raw->size_full = digest_length(digest);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>  /*
> >>   * state_backend_raw_file - create a raw file backend store for a state instance
> >>   *
> >> @@ -1534,8 +1627,14 @@ int state_backend_raw_file(struct state *state, const char *of_path,
> >>  		return -EINVAL;
> >>  
> >>  	backend_raw = xzalloc(sizeof(*backend_raw));
> >> -	backend = &backend_raw->backend;
> >>  
> >> +	ret = state_backend_raw_file_init_digest(state, backend_raw);
> >> +	if (ret) {
> >> +		free(backend_raw);
> >> +		return ret;
> >> +	}
> > 
> > Maybe better make this configurable with correct dependencies
> > (CONFIG_CRYPTO_KEYSTORE, CONFIG_DIGEST) rather than depending on the
> > user selecting the implicit dependencies manually?
> 
> There are noops so that it compile even if KEYSTORE and DIGEST is not
> selected.

I know, but letting the probe defer indefinitely is not a nice way to
tell the user that he misconfigured barebox. Also it's easy to add
something to the HMAC state code that doesn't have noops in the next
step without realizing it.

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 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox