linux-kernel - Re: [patch 3/6] [RFC] Add MMC Password Protection (lock/unlock) support V5

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <45447914.7070101@drzeus.cx>
Date:	Sun, 29 Oct 2006 10:49:08 +0100
From:	Pierre Ossman <drzeus-list@...eus.cx>
To:	Carlos Aguiar <carlos.aguiar@...t.org.br>
CC:	linux-kernel@...r.kernel.org,
	linux-omap-open-source@...ux.omap.com,
	David Brownell <david-b@...bell.net>,
	Tony Lindgren <tony@...mide.com>,
	Russell King <rmk+lkml@....linux.org.uk>,
	ilias.biris@...t.org.br
Subject: Re: [patch 3/6] [RFC] Add MMC Password Protection (lock/unlock) support
 V5

Carlos Aguiar wrote:
> @@ -1071,10 +1074,10 @@ static void mmc_check_cards(struct mmc_h
>  		cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
>  
>  		err = mmc_wait_for_cmd(host, &cmd, CMD_RETRIES);
> -		if (err == MMC_ERR_NONE)
> +		if (err != MMC_ERR_NONE) {
> +			mmc_card_set_dead(card);
>  			continue;
> -
> -		mmc_card_set_dead(card);
> +		}
>  	}
>  }
>  
>   

This seems like a pointless change.

> @@ -1160,6 +1163,139 @@ static void mmc_setup(struct mmc_host *h
>  		mmc_read_scrs(host);
>  }
>  
> +/* Calculate the minimal blksz_bits to hold x bytes. */
> +static inline int blksz_bits(unsigned x)
> +{
> +	return fls(x-1);
> +}
> +
>   

blksz_bits is gone, so this is superfluous. Note Russell's comment about
looking at the MMC_CAP_BYTEBLOCK capability, something you do not do
right now.

> +/**
> + *	mmc_lock_unlock - send LOCK_UNLOCK command to a specific card.
> + *	@card: card to which the LOCK_UNLOCK command should be sent
> + *	@key: key containing the MMC password
> + *	@mode: LOCK_UNLOCK mode
> + *
> + */
> +int mmc_lock_unlock(struct mmc_card *card, struct key *key, int mode)
> +{
> +	struct mmc_request mrq;
> +	struct mmc_command cmd;
> +	struct mmc_data data;
> +	struct scatterlist sg;
> +	struct mmc_key_payload *mpayload;
> +	unsigned long erase_timeout;
> +	int err, data_size;
> +	u8 *data_buf;
> +
> +	mpayload = NULL;
> +	data_size = 1;
> +	if (mode != MMC_LOCK_MODE_ERASE) {
> +		mpayload = rcu_dereference(key->payload.data);
> +		data_size = 2 + mpayload->datalen;
> +	}
> +
> +	data_buf = kmalloc(data_size, GFP_KERNEL);
>   

For something that can be at most 34 bytes, a kmalloc seems excessive.
Put it on the stack. Just remember to have checks so we do not overflow.

> +	if (!data_buf)
> +		return -ENOMEM;
> +	memset(data_buf, 0, data_size);
> +
> +	data_buf[0] = mode;
> +	if (mode != MMC_LOCK_MODE_ERASE) {
> +		data_buf[1] = mpayload->datalen;
> +		memcpy(data_buf + 2, mpayload->data, mpayload->datalen);
> +	}
> +
> +	err = mmc_card_claim_host(card);
> +	if (err != MMC_ERR_NONE) {
> +		mmc_card_set_dead(card);
> +		goto out;
> +	}
>   

Locking should be done outside this function to avoid races.

Also, why mark the card as dead because you cannot select it? It might
just be a temporary failure.

> +
> +	memset(&cmd, 0, sizeof(struct mmc_command));
> +
> +	cmd.opcode = MMC_SET_BLOCKLEN;
> +	cmd.arg = data_size;
> +	cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
> +	err = mmc_wait_for_cmd(card->host, &cmd, CMD_RETRIES);
> +	if (err != MMC_ERR_NONE) {
> +		mmc_card_set_dead(card);
> +		goto error;
> +	}
>   

Same here.

> +
> +	sg_init_one(&sg, data_buf, data_size);
> +	err = mmc_wait_for_req(card->host, &mrq);
> +	if (err != MMC_ERR_NONE) {
> +		if(err != MMC_ERR_INVALID)
> +			mmc_card_set_dead(card);
> +		goto error;
> +	}
>   

Dito.

> +
> +EXPORT_SYMBOL(mmc_lock_unlock);
>   

Why would anything but mmc_core need to use this?

>  
>  /**
>   *	mmc_detect_change - process change of state on a MMC socket
> Index: linux-2.6.18/include/linux/mmc/card.h
> ===================================================================
> --- linux-2.6.18.orig/include/linux/mmc/card.h	2006-10-20 12:37:42.000000000 -0400
> +++ linux-2.6.18/include/linux/mmc/card.h	2006-10-20 12:38:18.000000000 -0400
> @@ -117,4 +117,8 @@ static inline int mmc_card_claim_host(st
>  
>  #define mmc_card_release_host(c)	mmc_release_host((c)->host)
>  
> +struct key;
> +
> +extern int mmc_lock_unlock(struct mmc_card *card, struct key *key, int mode);
> +
>  #endif
>   

Which means this should go away (and put in drivers/mmc/mmc.h).

Rgds

-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/