[v2] mmc: sdhci: fix incorrect command used in tuning

Commit Message

Chris Ball Nov. 5, 2012, 7:45 p.m. UTC

Hi Aaron,

On Tue, Jul 03 2012, Aaron Lu wrote:
>  		 */
>  		if ((host->flags & SDHCI_NEEDS_RETUNING) &&
>  		    !(present_state & (SDHCI_DOING_WRITE | SDHCI_DOING_READ))) {
> +			/* eMMC uses cmd21 while sd and sdio use cmd19 */
> +			tuning_opcode = mmc->card->type == MMC_TYPE_MMC ?
> +				MMC_SEND_TUNING_BLOCK_HS200 :
> +				MMC_SEND_TUNING_BLOCK;

This is causing a NULL deref crash here when run on host controllers
with no card inserted -- mmc->card is NULL, as you'd expect, yet it's
dereferenced anyway.  Maybe the system you tested it on only has an
eMMC, so you never noticed that it crashes if there's no card present?
Or maybe it's abnormal for host controllers to set SDHCI_NEEDS_RETUNING
when there's no card present?

In any case, this has hit 3.[345]-stable now, so we're causing crashes
for people who weren't seeing a crash before they pulled -stable.  :(
The patch below just checks mmc->card before dereferencing it -- does
this look like the correct fix to you?


Subject: [PATCH] mmc: sdhci: Fix NULL dereference in sdhci_request() tuning code

Commit 473b095a72a9 ("mmc: sdhci: fix incorrect command used in tuning")
introduced a NULL dereference if an SD 3.0 host controller raises the
SDHCI_NEEDS_TUNING flag while no card is inserted.

Signed-off-by: Chris Ball <[e-mail]>
---
 drivers/mmc/host/sdhci.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

Comments

Chris Ball Nov. 9, 2012, 1:46 a.m. UTC | #1

Hi Aaron,

On Mon, Nov 05 2012, Chris Ball wrote:
> On Tue, Jul 03 2012, Aaron Lu wrote:
>>  		 */
>>  		if ((host->flags & SDHCI_NEEDS_RETUNING) &&
>>  		    !(present_state & (SDHCI_DOING_WRITE | SDHCI_DOING_READ))) {
>> +			/* eMMC uses cmd21 while sd and sdio use cmd19 */
>> +			tuning_opcode = mmc->card->type == MMC_TYPE_MMC ?
>> +				MMC_SEND_TUNING_BLOCK_HS200 :
>> +				MMC_SEND_TUNING_BLOCK;
>
> This is causing a NULL deref crash here when run on host controllers
> with no card inserted -- mmc->card is NULL, as you'd expect, yet it's
> dereferenced anyway.  Maybe the system you tested it on only has an
> eMMC, so you never noticed that it crashes if there's no card present?
> Or maybe it's abnormal for host controllers to set SDHCI_NEEDS_RETUNING
> when there's no card present?

I'm going to send my fix to Linus (and hence stable@) tomorrow morning,
since it's important to remove this crash.  Let me know what you think
when you get a chance.

Thanks,

- Chris.

Patch

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 07a5346..2e1175d 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1294,16 +1294,19 @@  static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
 		 */
 		if ((host->flags & SDHCI_NEEDS_RETUNING) &&
 		    !(present_state & (SDHCI_DOING_WRITE | SDHCI_DOING_READ))) {
-			/* eMMC uses cmd21 while sd and sdio use cmd19 */
-			tuning_opcode = mmc->card->type == MMC_TYPE_MMC ?
-				MMC_SEND_TUNING_BLOCK_HS200 :
-				MMC_SEND_TUNING_BLOCK;
-			spin_unlock_irqrestore(&host->lock, flags);
-			sdhci_execute_tuning(mmc, tuning_opcode);
-			spin_lock_irqsave(&host->lock, flags);
-
-			/* Restore original mmc_request structure */
-			host->mrq = mrq;
+			if (mmc->card) {
+				/* eMMC uses cmd21 but sd and sdio use cmd19 */
+				tuning_opcode =
+					mmc->card->type == MMC_TYPE_MMC ?
+					MMC_SEND_TUNING_BLOCK_HS200 :
+					MMC_SEND_TUNING_BLOCK;
+				spin_unlock_irqrestore(&host->lock, flags);
+				sdhci_execute_tuning(mmc, tuning_opcode);
+				spin_lock_irqsave(&host->lock, flags);
+
+				/* Restore original mmc_request structure */
+				host->mrq = mrq;
+			}
 		}
 
 		if (mrq->sbc && !(host->flags & SDHCI_AUTO_CMD23))