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

[Kai-Heng Feng]

at 17:52, Kai-Heng Feng <kai.heng.feng at 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 at 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 at lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team