diff mbox

[v2,5/5] mmc: mxcmmc: fix race conditions for host->req and host->data access

Message ID 1364768585-5177-6-git-send-email-agust@denx.de (mailing list archive)
State New, archived
Headers show

Commit Message

Anatolij Gustschin March 31, 2013, 10:23 p.m. UTC
mxcmci_dma_callback() is invoked by DMA drivers in soft-irq
context and can be interrupted by the mxcmci_irq() interrupt
which can finish the mmc request or data transfer and set
host->req or host->data pointers to NULL. Then mxcmci_data_done()
crashes with a null pointer dereferences. Protect all accesses
to host->req and host->data by spin locks.

Also check host->data pointer in mxcmci_watchdog() before
dereferencing it.

Signed-off-by: Anatolij Gustschin <agust@denx.de>
---
v2:
 - only rebased

 drivers/mmc/host/mxcmmc.c |   31 ++++++++++++++++++++++++-------
 1 files changed, 24 insertions(+), 7 deletions(-)

Comments

Sascha Hauer April 2, 2013, 6:58 a.m. UTC | #1
On Mon, Apr 01, 2013 at 12:23:05AM +0200, Anatolij Gustschin wrote:
> mxcmci_dma_callback() is invoked by DMA drivers in soft-irq
> context and can be interrupted by the mxcmci_irq() interrupt
> which can finish the mmc request or data transfer and set
> host->req or host->data pointers to NULL. Then mxcmci_data_done()
> crashes with a null pointer dereferences. Protect all accesses
> to host->req and host->data by spin locks.
> 
> Also check host->data pointer in mxcmci_watchdog() before
> dereferencing it.
> 
> Signed-off-by: Anatolij Gustschin <agust@denx.de>

This looks like a bugfix which should be first in this series.

Sascha

> ---
> v2:
>  - only rebased
> 
>  drivers/mmc/host/mxcmmc.c |   31 ++++++++++++++++++++++++-------
>  1 files changed, 24 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mmc/host/mxcmmc.c b/drivers/mmc/host/mxcmmc.c
> index 2861e0d..23deb2a 100644
> --- a/drivers/mmc/host/mxcmmc.c
> +++ b/drivers/mmc/host/mxcmmc.c
> @@ -694,24 +694,40 @@ static void mxcmci_datawork(struct work_struct *work)
>  
>  static void mxcmci_data_done(struct mxcmci_host *host, unsigned int stat)
>  {
> -	struct mmc_data *data = host->data;
> +	struct mmc_request *req;
>  	int data_error;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&host->lock, flags);
>  
> -	if (!data)
> +	if (!host->data) {
> +		spin_unlock_irqrestore(&host->lock, flags);
>  		return;
> +	}
> +
> +	if (!host->req) {
> +		spin_unlock_irqrestore(&host->lock, flags);
> +		return;
> +	}
> +
> +	req = host->req;
> +	if (!req->stop)
> +		host->req = NULL; /* we will handle finish req below */
>  
>  	data_error = mxcmci_finish_data(host, stat);
>  
> +	spin_unlock_irqrestore(&host->lock, flags);
> +
>  	mxcmci_read_response(host, stat);
>  	host->cmd = NULL;
>  
> -	if (host->req->stop) {
> -		if (mxcmci_start_cmd(host, host->req->stop, 0)) {
> -			mxcmci_finish_request(host, host->req);
> +	if (req->stop) {
> +		if (mxcmci_start_cmd(host, req->stop, 0)) {
> +			mxcmci_finish_request(host, req);
>  			return;
>  		}
>  	} else {
> -		mxcmci_finish_request(host, host->req);
> +		mxcmci_finish_request(host, req);
>  	}
>  }
>  
> @@ -1005,7 +1021,8 @@ static void mxcmci_watchdog(unsigned long data)
>  
>  	/* Mark transfer as erroneus and inform the upper layers */
>  
> -	host->data->error = -ETIMEDOUT;
> +	if (host->data)
> +		host->data->error = -ETIMEDOUT;
>  	host->req = NULL;
>  	host->cmd = NULL;
>  	host->data = NULL;
> -- 
> 1.7.5.4
> 
>
diff mbox

Patch

diff --git a/drivers/mmc/host/mxcmmc.c b/drivers/mmc/host/mxcmmc.c
index 2861e0d..23deb2a 100644
--- a/drivers/mmc/host/mxcmmc.c
+++ b/drivers/mmc/host/mxcmmc.c
@@ -694,24 +694,40 @@  static void mxcmci_datawork(struct work_struct *work)
 
 static void mxcmci_data_done(struct mxcmci_host *host, unsigned int stat)
 {
-	struct mmc_data *data = host->data;
+	struct mmc_request *req;
 	int data_error;
+	unsigned long flags;
+
+	spin_lock_irqsave(&host->lock, flags);
 
-	if (!data)
+	if (!host->data) {
+		spin_unlock_irqrestore(&host->lock, flags);
 		return;
+	}
+
+	if (!host->req) {
+		spin_unlock_irqrestore(&host->lock, flags);
+		return;
+	}
+
+	req = host->req;
+	if (!req->stop)
+		host->req = NULL; /* we will handle finish req below */
 
 	data_error = mxcmci_finish_data(host, stat);
 
+	spin_unlock_irqrestore(&host->lock, flags);
+
 	mxcmci_read_response(host, stat);
 	host->cmd = NULL;
 
-	if (host->req->stop) {
-		if (mxcmci_start_cmd(host, host->req->stop, 0)) {
-			mxcmci_finish_request(host, host->req);
+	if (req->stop) {
+		if (mxcmci_start_cmd(host, req->stop, 0)) {
+			mxcmci_finish_request(host, req);
 			return;
 		}
 	} else {
-		mxcmci_finish_request(host, host->req);
+		mxcmci_finish_request(host, req);
 	}
 }
 
@@ -1005,7 +1021,8 @@  static void mxcmci_watchdog(unsigned long data)
 
 	/* Mark transfer as erroneus and inform the upper layers */
 
-	host->data->error = -ETIMEDOUT;
+	if (host->data)
+		host->data->error = -ETIMEDOUT;
 	host->req = NULL;
 	host->cmd = NULL;
 	host->data = NULL;