| Message ID | 20180605095203.6902-2-kai.heng.feng@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix null pointer dereference in AMD MP2 driver | expand |
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
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
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(-)