[SRU,OEM-A,1/1] UBUNTU: SAUCE: i2c: Fix null pointer dereference in AMD MP2 driver

Message ID 20180605095203.6902-2-kai.heng.feng@canonical.com
State New
Headers show
Series
  • Fix null pointer dereference in AMD MP2 driver
Related show

Commit Message

Kai-Heng Feng June 5, 2018, 9:52 a.m.
BugLink: https://bugs.launchpad.net/bugs/1775152

The eventval in private data may race in amd_mp2_irq_isr() and
amd_mp2_pci_work(). Squash the latter into the former, so we can make
sure the data is guarded by the lock in the context.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 drivers/i2c/busses/i2c-amd-pci-mp2.c | 53 +++++++++++++---------------
 drivers/i2c/busses/i2c-amd-pci-mp2.h |  2 --
 2 files changed, 24 insertions(+), 31 deletions(-)

Comments

Kai-Heng Feng June 16, 2018, 10:28 a.m. | #1
at 17:52, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote:

> BugLink: https://bugs.launchpad.net/bugs/1775152
>
> The eventval in private data may race in amd_mp2_irq_isr() and
> amd_mp2_pci_work(). Squash the latter into the former, so we can make
> sure the data is guarded by the lock in the context.

This patch doesn't really solve the root cause. I'll send a new patch that  
really works.

Kai-Heng

>
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
>  drivers/i2c/busses/i2c-amd-pci-mp2.c | 53 +++++++++++++---------------
>  drivers/i2c/busses/i2c-amd-pci-mp2.h |  2 --
>  2 files changed, 24 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-amd-pci-mp2.c  
> b/drivers/i2c/busses/i2c-amd-pci-mp2.c
> index 2266bf156853..9aa12fddf4d5 100644
> --- a/drivers/i2c/busses/i2c-amd-pci-mp2.c
> +++ b/drivers/i2c/busses/i2c-amd-pci-mp2.c
> @@ -237,14 +237,33 @@ int amd_i2c_register_cb(struct pci_dev *dev, const  
> struct amd_i2c_pci_ops *ops,
>  }
>  EXPORT_SYMBOL_GPL(amd_i2c_register_cb);
>
> -static void amd_mp2_pci_work(struct work_struct *work)
> +static irqreturn_t amd_mp2_irq_isr(int irq, void *dev)
>  {
> -	struct amd_mp2_dev *privdata = mp2_dev(work);
> +	struct amd_mp2_dev *privdata = dev;
> +	u32 val = 0;
> +	unsigned long flags;
>  	u32 readdata = 0;
>  	int i = 0;
> -	int sts = privdata->eventval.r.status;
> -	int res = privdata->eventval.r.response;
> -	int len = privdata->eventval.r.length;
> +	int sts;
> +	int res;
> +	int len;
> +
> +	raw_spin_lock_irqsave(&privdata->lock, flags);
> +	val = readl(privdata->mmio + AMD_P2C_MSG1);
> +	if (val != 0) {
> +		writel(0, privdata->mmio + AMD_P2C_MSG_INTEN);
> +		privdata->eventval.ul = val;
> +	} else {
> +		val = readl(privdata->mmio + AMD_P2C_MSG2);
> +		if (val != 0) {
> +			writel(0, privdata->mmio + AMD_P2C_MSG_INTEN);
> +			privdata->eventval.ul = val;
> +		}
> +	}
> +
> +	sts = privdata->eventval.r.status;
> +	res = privdata->eventval.r.response;
> +	len = privdata->eventval.r.length;
>
>  	if (res == command_success && sts == i2c_readcomplete_event) {
>  		if (privdata->ops->read_complete) {
> @@ -272,26 +291,6 @@ static void amd_mp2_pci_work(struct work_struct *work)
>  	} else {
>  		dev_err(ndev_dev(privdata), "ERROR!!nothing to be handled !\n");
>  	}
> -}
> -
> -static irqreturn_t amd_mp2_irq_isr(int irq, void *dev)
> -{
> -	struct amd_mp2_dev *privdata = dev;
> -	u32 val = 0;
> -	unsigned long  flags;
> -
> -	raw_spin_lock_irqsave(&privdata->lock, flags);
> -	val = readl(privdata->mmio + AMD_P2C_MSG1);
> -	if (val != 0) {
> -		writel(0, privdata->mmio + AMD_P2C_MSG_INTEN);
> -		privdata->eventval.ul = val;
> -	} else {
> -		val = readl(privdata->mmio + AMD_P2C_MSG2);
> -		if (val != 0) {
> -			writel(0, privdata->mmio + AMD_P2C_MSG_INTEN);
> -			privdata->eventval.ul = val;
> -		}
> -	}
>
>  	raw_spin_unlock_irqrestore(&privdata->lock, flags);
>  	if (!privdata->ops)
> @@ -301,7 +300,6 @@ static irqreturn_t amd_mp2_irq_isr(int irq, void *dev)
>  		return IRQ_HANDLED;
>
>  	privdata->requested = false;
> -	schedule_delayed_work(&privdata->work, 0);
>
>  	return IRQ_HANDLED;
>  }
> @@ -536,8 +534,6 @@ static int amd_mp2_pci_probe(struct pci_dev *pdev,
>  		goto err_pci_init;
>  	dev_dbg(&pdev->dev, "pci init done.\n");
>
> -	INIT_DELAYED_WORK(&privdata->work, amd_mp2_pci_work);
> -
>  	amd_mp2_init_debugfs(privdata);
>  	dev_info(&pdev->dev, "MP2 device registered.\n");
>  	return 0;
> @@ -555,7 +551,6 @@ static void amd_mp2_pci_remove(struct pci_dev *pdev)
>
>  	amd_mp2_deinit_debugfs(privdata);
>  	amd_mp2_clear_reg(privdata);
> -	cancel_delayed_work_sync(&privdata->work);
>  	free_irq(pdev->irq, privdata);
>  	pci_intx(pdev, 0);
>  	amd_mp2_pci_deinit(privdata);
> diff --git a/drivers/i2c/busses/i2c-amd-pci-mp2.h  
> b/drivers/i2c/busses/i2c-amd-pci-mp2.h
> index a84389122885..f9380c494287 100644
> --- a/drivers/i2c/busses/i2c-amd-pci-mp2.h
> +++ b/drivers/i2c/busses/i2c-amd-pci-mp2.h
> @@ -229,7 +229,6 @@ struct amd_mp2_dev {
>  	struct i2c_write_config write_cfg;
>  	union i2c_cmd_base i2c_cmd_base;
>  	const struct amd_i2c_pci_ops *ops;
> -	struct delayed_work work;
>  	void *i2c_dev_ctx;
>  	bool	requested;
>  	raw_spinlock_t lock;
> @@ -246,6 +245,5 @@ int amd_i2c_register_cb(struct pci_dev *pdev, const  
> struct amd_i2c_pci_ops *ops,
>  #define ndev_pdev(ndev) ((ndev)->pdev)
>  #define ndev_name(ndev) pci_name(ndev_pdev(ndev))
>  #define ndev_dev(ndev) (&ndev_pdev(ndev)->dev)
> -#define mp2_dev(__work) container_of(__work, struct amd_mp2_dev,  
> work.work)
>
>  #endif
> -- 
> 2.17.0
>
>
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

Patch

diff --git a/drivers/i2c/busses/i2c-amd-pci-mp2.c b/drivers/i2c/busses/i2c-amd-pci-mp2.c
index 2266bf156853..9aa12fddf4d5 100644
--- a/drivers/i2c/busses/i2c-amd-pci-mp2.c
+++ b/drivers/i2c/busses/i2c-amd-pci-mp2.c
@@ -237,14 +237,33 @@  int amd_i2c_register_cb(struct pci_dev *dev, const struct amd_i2c_pci_ops *ops,
 }
 EXPORT_SYMBOL_GPL(amd_i2c_register_cb);
 
-static void amd_mp2_pci_work(struct work_struct *work)
+static irqreturn_t amd_mp2_irq_isr(int irq, void *dev)
 {
-	struct amd_mp2_dev *privdata = mp2_dev(work);
+	struct amd_mp2_dev *privdata = dev;
+	u32 val = 0;
+	unsigned long flags;
 	u32 readdata = 0;
 	int i = 0;
-	int sts = privdata->eventval.r.status;
-	int res = privdata->eventval.r.response;
-	int len = privdata->eventval.r.length;
+	int sts;
+	int res;
+	int len;
+
+	raw_spin_lock_irqsave(&privdata->lock, flags);
+	val = readl(privdata->mmio + AMD_P2C_MSG1);
+	if (val != 0) {
+		writel(0, privdata->mmio + AMD_P2C_MSG_INTEN);
+		privdata->eventval.ul = val;
+	} else {
+		val = readl(privdata->mmio + AMD_P2C_MSG2);
+		if (val != 0) {
+			writel(0, privdata->mmio + AMD_P2C_MSG_INTEN);
+			privdata->eventval.ul = val;
+		}
+	}
+
+	sts = privdata->eventval.r.status;
+	res = privdata->eventval.r.response;
+	len = privdata->eventval.r.length;
 
 	if (res == command_success && sts == i2c_readcomplete_event) {
 		if (privdata->ops->read_complete) {
@@ -272,26 +291,6 @@  static void amd_mp2_pci_work(struct work_struct *work)
 	} else {
 		dev_err(ndev_dev(privdata), "ERROR!!nothing to be handled !\n");
 	}
-}
-
-static irqreturn_t amd_mp2_irq_isr(int irq, void *dev)
-{
-	struct amd_mp2_dev *privdata = dev;
-	u32 val = 0;
-	unsigned long  flags;
-
-	raw_spin_lock_irqsave(&privdata->lock, flags);
-	val = readl(privdata->mmio + AMD_P2C_MSG1);
-	if (val != 0) {
-		writel(0, privdata->mmio + AMD_P2C_MSG_INTEN);
-		privdata->eventval.ul = val;
-	} else {
-		val = readl(privdata->mmio + AMD_P2C_MSG2);
-		if (val != 0) {
-			writel(0, privdata->mmio + AMD_P2C_MSG_INTEN);
-			privdata->eventval.ul = val;
-		}
-	}
 
 	raw_spin_unlock_irqrestore(&privdata->lock, flags);
 	if (!privdata->ops)
@@ -301,7 +300,6 @@  static irqreturn_t amd_mp2_irq_isr(int irq, void *dev)
 		return IRQ_HANDLED;
 
 	privdata->requested = false;
-	schedule_delayed_work(&privdata->work, 0);
 
 	return IRQ_HANDLED;
 }
@@ -536,8 +534,6 @@  static int amd_mp2_pci_probe(struct pci_dev *pdev,
 		goto err_pci_init;
 	dev_dbg(&pdev->dev, "pci init done.\n");
 
-	INIT_DELAYED_WORK(&privdata->work, amd_mp2_pci_work);
-
 	amd_mp2_init_debugfs(privdata);
 	dev_info(&pdev->dev, "MP2 device registered.\n");
 	return 0;
@@ -555,7 +551,6 @@  static void amd_mp2_pci_remove(struct pci_dev *pdev)
 
 	amd_mp2_deinit_debugfs(privdata);
 	amd_mp2_clear_reg(privdata);
-	cancel_delayed_work_sync(&privdata->work);
 	free_irq(pdev->irq, privdata);
 	pci_intx(pdev, 0);
 	amd_mp2_pci_deinit(privdata);
diff --git a/drivers/i2c/busses/i2c-amd-pci-mp2.h b/drivers/i2c/busses/i2c-amd-pci-mp2.h
index a84389122885..f9380c494287 100644
--- a/drivers/i2c/busses/i2c-amd-pci-mp2.h
+++ b/drivers/i2c/busses/i2c-amd-pci-mp2.h
@@ -229,7 +229,6 @@  struct amd_mp2_dev {
 	struct i2c_write_config write_cfg;
 	union i2c_cmd_base i2c_cmd_base;
 	const struct amd_i2c_pci_ops *ops;
-	struct delayed_work work;
 	void *i2c_dev_ctx;
 	bool	requested;
 	raw_spinlock_t lock;
@@ -246,6 +245,5 @@  int amd_i2c_register_cb(struct pci_dev *pdev, const struct amd_i2c_pci_ops *ops,
 #define ndev_pdev(ndev) ((ndev)->pdev)
 #define ndev_name(ndev) pci_name(ndev_pdev(ndev))
 #define ndev_dev(ndev) (&ndev_pdev(ndev)->dev)
-#define mp2_dev(__work) container_of(__work, struct amd_mp2_dev, work.work)
 
 #endif