| Message ID | 20230925062133.14170-1-dinghao.liu@zju.edu.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI: xgene-msi: Fix a potential UAF in xgene_msi_probe | expand |
On Mon, 25 Sep 2023 07:21:32 +0100, Dinghao Liu <dinghao.liu@zju.edu.cn> wrote: > > xgene_allocate_domains() will call irq_domain_remove() to free > msi->inner_domain on failure. However, its caller, xgene_msi_probe(), > will also call irq_domain_remove() through xgene_msi_remove() on the > same failure, which may lead to a use-after-free. Set the freed pointer > to NULL to fix this issue. > > Fixes: dcd19de36775 ("PCI: xgene: Add APM X-Gene v1 PCIe MSI/MSIX termination driver") > Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn> > --- > drivers/pci/controller/pci-xgene-msi.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/pci/controller/pci-xgene-msi.c b/drivers/pci/controller/pci-xgene-msi.c > index 3ce38dfd0d29..c0192c5ff0f3 100644 > --- a/drivers/pci/controller/pci-xgene-msi.c > +++ b/drivers/pci/controller/pci-xgene-msi.c > @@ -253,6 +253,7 @@ static int xgene_allocate_domains(struct xgene_msi *msi) > > if (!msi->msi_domain) { > irq_domain_remove(msi->inner_domain); > + msi->inner_domain = NULL; > return -ENOMEM; > } Why can't we just drop the irq_domain_remove() call here instead, and simply rely on xgene_msi_remove() to do the right thing? Something like the untested patch below. Thanks, M. diff --git a/drivers/pci/controller/pci-xgene-msi.c b/drivers/pci/controller/pci-xgene-msi.c index 0234e528b9a5..f98c9eb7bebf 100644 --- a/drivers/pci/controller/pci-xgene-msi.c +++ b/drivers/pci/controller/pci-xgene-msi.c @@ -251,10 +251,8 @@ static int xgene_allocate_domains(struct xgene_msi *msi) &xgene_msi_domain_info, msi->inner_domain); - if (!msi->msi_domain) { - irq_domain_remove(msi->inner_domain); + if (!msi->msi_domain) return -ENOMEM; - } return 0; }
> On Mon, 25 Sep 2023 07:21:32 +0100, > Dinghao Liu <dinghao.liu@zju.edu.cn> wrote: > > > > xgene_allocate_domains() will call irq_domain_remove() to free > > msi->inner_domain on failure. However, its caller, xgene_msi_probe(), > > will also call irq_domain_remove() through xgene_msi_remove() on the > > same failure, which may lead to a use-after-free. Set the freed pointer > > to NULL to fix this issue. > > > > Fixes: dcd19de36775 ("PCI: xgene: Add APM X-Gene v1 PCIe MSI/MSIX termination driver") > > Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn> > > --- > > drivers/pci/controller/pci-xgene-msi.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/pci/controller/pci-xgene-msi.c b/drivers/pci/controller/pci-xgene-msi.c > > index 3ce38dfd0d29..c0192c5ff0f3 100644 > > --- a/drivers/pci/controller/pci-xgene-msi.c > > +++ b/drivers/pci/controller/pci-xgene-msi.c > > @@ -253,6 +253,7 @@ static int xgene_allocate_domains(struct xgene_msi *msi) > > > > if (!msi->msi_domain) { > > irq_domain_remove(msi->inner_domain); > > + msi->inner_domain = NULL; > > return -ENOMEM; > > } > > Why can't we just drop the irq_domain_remove() call here instead, and > simply rely on xgene_msi_remove() to do the right thing? Something > like the untested patch below. > > Thanks, > > M. > > diff --git a/drivers/pci/controller/pci-xgene-msi.c b/drivers/pci/controller/pci-xgene-msi.c > index 0234e528b9a5..f98c9eb7bebf 100644 > --- a/drivers/pci/controller/pci-xgene-msi.c > +++ b/drivers/pci/controller/pci-xgene-msi.c > @@ -251,10 +251,8 @@ static int xgene_allocate_domains(struct xgene_msi *msi) > &xgene_msi_domain_info, > msi->inner_domain); > > - if (!msi->msi_domain) { > - irq_domain_remove(msi->inner_domain); > + if (!msi->msi_domain) > return -ENOMEM; > - } > > return 0; > } Thanks for your advice! This patch is more concise. I will resend a new patch soon. Regards, Dinghao
diff --git a/drivers/pci/controller/pci-xgene-msi.c b/drivers/pci/controller/pci-xgene-msi.c index 3ce38dfd0d29..c0192c5ff0f3 100644 --- a/drivers/pci/controller/pci-xgene-msi.c +++ b/drivers/pci/controller/pci-xgene-msi.c @@ -253,6 +253,7 @@ static int xgene_allocate_domains(struct xgene_msi *msi) if (!msi->msi_domain) { irq_domain_remove(msi->inner_domain); + msi->inner_domain = NULL; return -ENOMEM; }
xgene_allocate_domains() will call irq_domain_remove() to free msi->inner_domain on failure. However, its caller, xgene_msi_probe(), will also call irq_domain_remove() through xgene_msi_remove() on the same failure, which may lead to a use-after-free. Set the freed pointer to NULL to fix this issue. Fixes: dcd19de36775 ("PCI: xgene: Add APM X-Gene v1 PCIe MSI/MSIX termination driver") Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn> --- drivers/pci/controller/pci-xgene-msi.c | 1 + 1 file changed, 1 insertion(+)