Re: [patch 3/5] Add MMC password protection (lock/unlock) support V2

[Chris White]

On Thursday 29 December 2005 03:40, Anderson Lizardo wrote:

Something that sort of caught my eye while looking at this (I generally don't 
post here so I'm bitting the bullet and hoping I don't screw up), it seems 
that this is an experimental driver, but doesn't contain any sort of uniquely 
seperated verbose debug information.  Let me try and narrow that down:

> +int mmc_key_instantiate(struct key *key, const void *data, size_t datalen)
> +{
> +	struct mmc_key_payload *mpayload;
> +	struct device *dev;
> +	struct mmc_card *card;
> +	int ret;
> +
> +	ret = -EINVAL;
> +	if (datalen <= 0 || datalen > MMC_KEYLEN_MAXBYTES || !data)
> +		goto error;

Right here something about the data being passed to the function is invalid.

> +	ret = key_payload_reserve(key, datalen);
> +	if (ret < 0)
> +		goto error;
> +
> +	ret = -ENOMEM;
> +	mpayload = kmalloc(sizeof(*mpayload) + datalen, GFP_KERNEL);
> +	if (!mpayload)
> +		goto error;

Unable to allocate mpayload structure, or something of the like.

> +	/* attach the data */
> +	mpayload->datalen = datalen;
> +	memcpy(mpayload->data, data, datalen);
> +	rcu_assign_pointer(key->payload.data, mpayload);
> +
> +	ret = -EINVAL;
> +	dev = bus_find_device(&mmc_bus_type, NULL, NULL, mmc_match_lockable);
> +	if (!dev)
> +		goto error;

Unable to locate device.

> +	card = dev_to_mmc_card(dev);
> +	if (mmc_card_locked(card)) {
> +		ret = mmc_lock_unlock(card, key, MMC_LOCK_MODE_UNLOCK);
> +		mmc_remove_card(card);
> +		mmc_register_card(card);
> +	} else
> +		ret = mmc_lock_unlock(card, key, MMC_LOCK_MODE_SET_PWD);
> +	if (ret)
> +		ret = -EKEYREJECTED;

Key was rejected, though I suppose EKEYREJECTED pretty much states that.

[snip snip]
> +
> +/*
> + * update a mmc key
> + * - the key's semaphore is write-locked
> + */
> +int mmc_key_update(struct key *key, const void *data, size_t datalen)
> +{
> +	struct mmc_key_payload *mpayload, *zap;
> +	struct device *dev;
> +	struct mmc_card *card;
> +	int ret;
> +
> +	ret = -EINVAL;
> +	if (datalen <= 0 || datalen > MMC_KEYLEN_MAXBYTES || !data)
> +		goto error;

See above about invalid data

> +	/* construct a replacement payload */
> +	ret = -ENOMEM;
> +	mpayload = kmalloc(sizeof(*mpayload) + datalen, GFP_KERNEL);
> +	if (!mpayload)
> +		goto error;

This code almost seemed similiar to mmc_key_instantiate.. I almost wonder if 
the code could be consolidated into a single function with some sort of 
update conditional code.  With that the debug information wouldn't be 
duplicated. so snip

> +#ifdef	CONFIG_MMC_PASSWORDS
> +		else {
> +			ret = register_key_type(&mmc_key_type);
> +			if (ret) {

Something about the registration failing.

> +				class_unregister(&mmc_host_class);
> +				bus_unregister(&mmc_bus_type);
> +			}
> +		}
> +#endif
>  	}
>  	return ret;
>  }
> @@ -345,6 +501,9 @@ static void __exit mmc_exit(void)
>  {
>  	class_unregister(&mmc_host_class);
>  	bus_unregister(&mmc_bus_type);
> +#ifdef	CONFIG_MMC_PASSWORDS
> +	unregister_key_type(&mmc_key_type);
> +#endif
>  }
>
>  module_init(mmc_init);

That was mainly it.  The verbose debug information is more of a "this would be 
nice" sort of thing.  Just from a user's perspective of debuggin experimental 
drivers, this sort of thing is always nice.  The code duplication in 
mmc_key_instantiate/update still catches my eye though, there may be a 
functional code flow to this that I'm not aware of, so again I bite the 
bullet.  Best hope that I don't make a fool of myself :).

Chris White

Attachment: pgp00000.pgp
Description: PGP signature