[PATCH v1] genirq/msi: fix crash when handling Multi-MSI

[PATCH v1] genirq/msi: fix crash when handling Multi-MSI

[Tong Zhang]

pci_msi_domain_check_cap() could return 1 when domain does not support
multi MSI and user request multi MSI. This positive value will be used by
__pci_enable_msi_range(). In previous refactor, this positive value is
handled as error case which will cause kernel crash.

[    1.197953] BUG: KASAN: use-after-free in __pci_enable_msi_range+0x234/0x320
[    1.198327] Freed by task 1:
[    1.198327]  kfree+0x8f/0x2b0
[    1.198327]  msi_free_msi_descs_range+0xf5/0x130
[    1.198327]  msi_domain_alloc_irqs_descs_locked+0x8d/0xa0
[    1.198327]  __pci_enable_msi_range+0x1a4/0x320
[    1.198327]  pci_alloc_irq_vectors_affinity+0x135/0x1a0
[    1.198327]  pcie_port_device_register+0x4a1/0x5c0
[    1.198327]  pcie_portdrv_probe+0x50/0x100

Fixes: 0f62d941acf9 ("genirq/msi: Provide msi_domain_alloc/free_irqs_descs_locked()")

Signed-off-by: Tong Zhang <ztong0001@gmail.com>
---
 kernel/irq/msi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
index 2bdfce5edafd..57b1447a3bf1 100644
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -935,7 +935,7 @@ int msi_domain_alloc_irqs_descs_locked(struct irq_domain *domain, struct device
 		return ret;
 
 	ret = ops->domain_alloc_irqs(domain, dev, nvec);
-	if (ret)
+	if (ret < 0)
 		msi_domain_free_irqs_descs_locked(domain, dev);
 	return ret;
 }
-- 
2.25.1

Re: [PATCH v1] genirq/msi: fix crash when handling Multi-MSI

[Marc Zyngier]

On Mon, 17 Jan 2022 09:27:59 +0000,
Tong Zhang <ztong0001@gmail.com> wrote:
> 
> pci_msi_domain_check_cap() could return 1 when domain does not support
> multi MSI and user request multi MSI. This positive value will be used by
> __pci_enable_msi_range(). In previous refactor, this positive value is
> handled as error case which will cause kernel crash.
> 
> [    1.197953] BUG: KASAN: use-after-free in __pci_enable_msi_range+0x234/0x320
> [    1.198327] Freed by task 1:
> [    1.198327]  kfree+0x8f/0x2b0
> [    1.198327]  msi_free_msi_descs_range+0xf5/0x130
> [    1.198327]  msi_domain_alloc_irqs_descs_locked+0x8d/0xa0
> [    1.198327]  __pci_enable_msi_range+0x1a4/0x320
> [    1.198327]  pci_alloc_irq_vectors_affinity+0x135/0x1a0
> [    1.198327]  pcie_port_device_register+0x4a1/0x5c0
> [    1.198327]  pcie_portdrv_probe+0x50/0x100

I'm sorry, but you'll have to be a bit clearer in your commit message,
because I cannot relate what you describe with the patch.

The real issue seems to be that a domain_alloc_irqs callback can
return a positive, non-zero value, and I don't think this is expected.

How about this instead? If I am barking up the wrong tree, please
provide a more accurate description of the problem you are seeing.

Thanks,

	M.

diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
index 2bdfce5edafd..da8bb6135627 100644
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -878,8 +878,10 @@ int __msi_domain_alloc_irqs(struct irq_domain *domain, struct device *dev,
 		virq = __irq_domain_alloc_irqs(domain, -1, desc->nvec_used,
 					       dev_to_node(dev), &arg, false,
 					       desc->affinity);
-		if (virq < 0)
-			return msi_handle_pci_fail(domain, desc, allocated);
+		if (virq < 0) {
+			ret = msi_handle_pci_fail(domain, desc, allocated);
+			return ret < 0 ? ret : 0;
+		}
 
 		for (i = 0; i < desc->nvec_used; i++) {
 			irq_set_msi_desc_off(virq, i, desc);

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v1] genirq/msi: fix crash when handling Multi-MSI

[Tong Zhang]

Hello,

ops->msi_check could point to pci_msi_domain_check_cap that is the
function in question

hence we can have following call stack

pci_msi_domain_check_cap (used by ops->msi_check(domain, info, dev))
msi_domain_prepare_irqs
__msi_domain_alloc_irqs
msi_domain_alloc_irqs_descs_locked

What I am suggesting is commit 0f62d941acf9 changed how this return
value is being handled and created a UAF

- Tong


On Mon, Jan 17, 2022 at 2:00 AM Marc Zyngier <maz@kernel.org> wrote:
>
> On Mon, 17 Jan 2022 09:27:59 +0000,
> Tong Zhang <ztong0001@gmail.com> wrote:
> >
> > pci_msi_domain_check_cap() could return 1 when domain does not support
> > multi MSI and user request multi MSI. This positive value will be used by
> > __pci_enable_msi_range(). In previous refactor, this positive value is
> > handled as error case which will cause kernel crash.
> >
> > [    1.197953] BUG: KASAN: use-after-free in __pci_enable_msi_range+0x234/0x320
> > [    1.198327] Freed by task 1:
> > [    1.198327]  kfree+0x8f/0x2b0
> > [    1.198327]  msi_free_msi_descs_range+0xf5/0x130
> > [    1.198327]  msi_domain_alloc_irqs_descs_locked+0x8d/0xa0
> > [    1.198327]  __pci_enable_msi_range+0x1a4/0x320
> > [    1.198327]  pci_alloc_irq_vectors_affinity+0x135/0x1a0
> > [    1.198327]  pcie_port_device_register+0x4a1/0x5c0
> > [    1.198327]  pcie_portdrv_probe+0x50/0x100
>
> I'm sorry, but you'll have to be a bit clearer in your commit message,
> because I cannot relate what you describe with the patch.
>
> The real issue seems to be that a domain_alloc_irqs callback can
> return a positive, non-zero value, and I don't think this is expected.
>
> How about this instead? If I am barking up the wrong tree, please
> provide a more accurate description of the problem you are seeing.
>
> Thanks,
>
>         M.
>
> diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
> index 2bdfce5edafd..da8bb6135627 100644
> --- a/kernel/irq/msi.c
> +++ b/kernel/irq/msi.c
> @@ -878,8 +878,10 @@ int __msi_domain_alloc_irqs(struct irq_domain *domain, struct device *dev,
>                 virq = __irq_domain_alloc_irqs(domain, -1, desc->nvec_used,
>                                                dev_to_node(dev), &arg, false,
>                                                desc->affinity);
> -               if (virq < 0)
> -                       return msi_handle_pci_fail(domain, desc, allocated);
> +               if (virq < 0) {
> +                       ret = msi_handle_pci_fail(domain, desc, allocated);
> +                       return ret < 0 ? ret : 0;
> +               }
>
>                 for (i = 0; i < desc->nvec_used; i++) {
>                         irq_set_msi_desc_off(virq, i, desc);
>
> --
> Without deviation from the norm, progress is not possible.

Re: [PATCH v1] genirq/msi: fix crash when handling Multi-MSI

[Marc Zyngier]

Please avoid top-posting.

On Mon, 17 Jan 2022 10:10:13 +0000,
Tong Zhang <ztong0001@gmail.com> wrote:
> 
> Hello,
> 
> ops->msi_check could point to pci_msi_domain_check_cap that is the
> function in question
> 
> hence we can have following call stack
> 
> pci_msi_domain_check_cap (used by ops->msi_check(domain, info, dev))
> msi_domain_prepare_irqs
> __msi_domain_alloc_irqs
> msi_domain_alloc_irqs_descs_locked
> 
> What I am suggesting is commit 0f62d941acf9 changed how this return
> value is being handled and created a UAF

OK, this makes more sense.

But msi_domain_prepare_irqs() shouldn't fail in this case, and we
should proceed with the allocation of at least one vector, which isn't
happening here.

Also, if __msi_domain_alloc_irqs() is supposed to return the number of
irqs allocated, it isn't doing it consistently.

Thomas, can you shed some light on what is the intended behaviour
here?

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v1] genirq/msi: fix crash when handling Multi-MSI

[Thomas Gleixner]

On Mon, Jan 17 2022 at 11:36, Marc Zyngier wrote:
> On Mon, 17 Jan 2022 10:10:13 +0000,
> Tong Zhang <ztong0001@gmail.com> wrote:
>> pci_msi_domain_check_cap (used by ops->msi_check(domain, info, dev))
>> msi_domain_prepare_irqs
>> __msi_domain_alloc_irqs
>> msi_domain_alloc_irqs_descs_locked
>> 
>> What I am suggesting is commit 0f62d941acf9 changed how this return
>> value is being handled and created a UAF
>
> OK, this makes more sense.
>
> But msi_domain_prepare_irqs() shouldn't fail in this case, and we
> should proceed with the allocation of at least one vector, which isn't
> happening here.
>
> Also, if __msi_domain_alloc_irqs() is supposed to return the number of
> irqs allocated, it isn't doing it consistently.
>
> Thomas, can you shed some light on what is the intended behaviour
> here?

Let me stare at it.

Re: [PATCH v1] genirq/msi: fix crash when handling Multi-MSI

[Thomas Gleixner]

On Tue, Jan 18 2022 at 15:39, Thomas Gleixner wrote:
> On Mon, Jan 17 2022 at 11:36, Marc Zyngier wrote:
>> On Mon, 17 Jan 2022 10:10:13 +0000,
>> Tong Zhang <ztong0001@gmail.com> wrote:
>>> pci_msi_domain_check_cap (used by ops->msi_check(domain, info, dev))
>>> msi_domain_prepare_irqs
>>> __msi_domain_alloc_irqs
>>> msi_domain_alloc_irqs_descs_locked
>>> 
>>> What I am suggesting is commit 0f62d941acf9 changed how this return
>>> value is being handled and created a UAF
>>
>> OK, this makes more sense.
>>
>> But msi_domain_prepare_irqs() shouldn't fail in this case, and we
>> should proceed with the allocation of at least one vector, which isn't
>> happening here.
>>
>> Also, if __msi_domain_alloc_irqs() is supposed to return the number of
>> irqs allocated, it isn't doing it consistently.
>>
>> Thomas, can you shed some light on what is the intended behaviour
>> here?
>
> Let me stare at it.

It's a subtle issue I overlooked. The UAF is due to

err:
	pci_msi_unmask(entry, msi_multi_mask(entry));

in msi_capability_init() because the core has torn down and freed the
entry already.

The proposed patch "fixes" the issue for the PCI/MSI case, but could
cause a memory leak for other callers.

Not sure yet what the proper fix is, but that has to wait until tomorrow
when brain becomes awake again.

Thanks,

        tglx

[PATCH] PCI/MSI: Prevent UAF in error path

[Thomas Gleixner]

When the core MSI allocation fails, then the PCI/MSI code uses an already
freed MSI descriptor to unmask the MSI mask register in order to bring it back
into reset state.

Remove MSI_FLAG_FREE_MSI_DESCS from the PCI/MSI irqdomain flags and let the
PCI/MSI code free the MSI descriptors after usage.

Fixes: 0f62d941acf9 ("genirq/msi: Provide msi_domain_alloc/free_irqs_descs_locked()")
Reported-by: Tong Zhang <ztong0001@gmail.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/pci/msi/irqdomain.c |    4 ++--
 drivers/pci/msi/legacy.c    |    1 -
 2 files changed, 2 insertions(+), 3 deletions(-)

--- a/drivers/pci/msi/irqdomain.c
+++ b/drivers/pci/msi/irqdomain.c
@@ -28,6 +28,7 @@ void pci_msi_teardown_msi_irqs(struct pc
 		msi_domain_free_irqs_descs_locked(domain, &dev->dev);
 	else
 		pci_msi_legacy_teardown_msi_irqs(dev);
+	msi_free_msi_descs(&dev->dev);
 }
 
 /**
@@ -171,8 +172,7 @@ struct irq_domain *pci_msi_create_irq_do
 	if (info->flags & MSI_FLAG_USE_DEF_CHIP_OPS)
 		pci_msi_domain_update_chip_ops(info);
 
-	info->flags |= MSI_FLAG_ACTIVATE_EARLY | MSI_FLAG_DEV_SYSFS |
-		       MSI_FLAG_FREE_MSI_DESCS;
+	info->flags |= MSI_FLAG_ACTIVATE_EARLY | MSI_FLAG_DEV_SYSFS;
 	if (IS_ENABLED(CONFIG_GENERIC_IRQ_RESERVATION_MODE))
 		info->flags |= MSI_FLAG_MUST_REACTIVATE;
 
--- a/drivers/pci/msi/legacy.c
+++ b/drivers/pci/msi/legacy.c
@@ -77,5 +77,4 @@ void pci_msi_legacy_teardown_msi_irqs(st
 {
 	msi_device_destroy_sysfs(&dev->dev);
 	arch_teardown_msi_irqs(dev);
-	msi_free_msi_descs(&dev->dev);
 }

Re: [PATCH] PCI/MSI: Prevent UAF in error path

[Bjorn Helgaas]

On Wed, Jan 19, 2022 at 06:54:52PM +0100, Thomas Gleixner wrote:
> When the core MSI allocation fails, then the PCI/MSI code uses an already
> freed MSI descriptor to unmask the MSI mask register in order to bring it back
> into reset state.
> 
> Remove MSI_FLAG_FREE_MSI_DESCS from the PCI/MSI irqdomain flags and let the
> PCI/MSI code free the MSI descriptors after usage.
> 
> Fixes: 0f62d941acf9 ("genirq/msi: Provide msi_domain_alloc/free_irqs_descs_locked()")
> Reported-by: Tong Zhang <ztong0001@gmail.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

What does "UAF" stand for?  Ah, "use after free" I guess?

Let me know if I should take this.  Otherwise I assume it'll go
whereever 0f62d941acf9 went.

> ---
>  drivers/pci/msi/irqdomain.c |    4 ++--
>  drivers/pci/msi/legacy.c    |    1 -
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> --- a/drivers/pci/msi/irqdomain.c
> +++ b/drivers/pci/msi/irqdomain.c
> @@ -28,6 +28,7 @@ void pci_msi_teardown_msi_irqs(struct pc
>  		msi_domain_free_irqs_descs_locked(domain, &dev->dev);
>  	else
>  		pci_msi_legacy_teardown_msi_irqs(dev);
> +	msi_free_msi_descs(&dev->dev);
>  }
>  
>  /**
> @@ -171,8 +172,7 @@ struct irq_domain *pci_msi_create_irq_do
>  	if (info->flags & MSI_FLAG_USE_DEF_CHIP_OPS)
>  		pci_msi_domain_update_chip_ops(info);
>  
> -	info->flags |= MSI_FLAG_ACTIVATE_EARLY | MSI_FLAG_DEV_SYSFS |
> -		       MSI_FLAG_FREE_MSI_DESCS;
> +	info->flags |= MSI_FLAG_ACTIVATE_EARLY | MSI_FLAG_DEV_SYSFS;
>  	if (IS_ENABLED(CONFIG_GENERIC_IRQ_RESERVATION_MODE))
>  		info->flags |= MSI_FLAG_MUST_REACTIVATE;
>  
> --- a/drivers/pci/msi/legacy.c
> +++ b/drivers/pci/msi/legacy.c
> @@ -77,5 +77,4 @@ void pci_msi_legacy_teardown_msi_irqs(st
>  {
>  	msi_device_destroy_sysfs(&dev->dev);
>  	arch_teardown_msi_irqs(dev);
> -	msi_free_msi_descs(&dev->dev);
>  }

Re: [PATCH] PCI/MSI: Prevent UAF in error path

[Tong Zhang]

On Wed, Jan 19, 2022 at 9:54 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> When the core MSI allocation fails, then the PCI/MSI code uses an already
> freed MSI descriptor to unmask the MSI mask register in order to bring it back
> into reset state.
>
> Remove MSI_FLAG_FREE_MSI_DESCS from the PCI/MSI irqdomain flags and let the
> PCI/MSI code free the MSI descriptors after usage.
>
> Fixes: 0f62d941acf9 ("genirq/msi: Provide msi_domain_alloc/free_irqs_descs_locked()")
> Reported-by: Tong Zhang <ztong0001@gmail.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  drivers/pci/msi/irqdomain.c |    4 ++--
>  drivers/pci/msi/legacy.c    |    1 -
>  2 files changed, 2 insertions(+), 3 deletions(-)
>
> --- a/drivers/pci/msi/irqdomain.c
> +++ b/drivers/pci/msi/irqdomain.c
> @@ -28,6 +28,7 @@ void pci_msi_teardown_msi_irqs(struct pc
>                 msi_domain_free_irqs_descs_locked(domain, &dev->dev);
>         else
>                 pci_msi_legacy_teardown_msi_irqs(dev);
> +       msi_free_msi_descs(&dev->dev);
>  }
>
>  /**
> @@ -171,8 +172,7 @@ struct irq_domain *pci_msi_create_irq_do
>         if (info->flags & MSI_FLAG_USE_DEF_CHIP_OPS)
>                 pci_msi_domain_update_chip_ops(info);
>
> -       info->flags |= MSI_FLAG_ACTIVATE_EARLY | MSI_FLAG_DEV_SYSFS |
> -                      MSI_FLAG_FREE_MSI_DESCS;
> +       info->flags |= MSI_FLAG_ACTIVATE_EARLY | MSI_FLAG_DEV_SYSFS;
>         if (IS_ENABLED(CONFIG_GENERIC_IRQ_RESERVATION_MODE))
>                 info->flags |= MSI_FLAG_MUST_REACTIVATE;
>
> --- a/drivers/pci/msi/legacy.c
> +++ b/drivers/pci/msi/legacy.c
> @@ -77,5 +77,4 @@ void pci_msi_legacy_teardown_msi_irqs(st
>  {
>         msi_device_destroy_sysfs(&dev->dev);
>         arch_teardown_msi_irqs(dev);
> -       msi_free_msi_descs(&dev->dev);
>  }

Tested-by: Tong Zhang <ztong0001@gmail.com>

Tested on my setup, it no longer crashes.
Thanks!
- Tong

[tip: irq/urgent] PCI/MSI: Prevent UAF in error path

[tip-bot2 for Thomas Gleixner]

The following commit has been merged into the irq/urgent branch of tip:

Commit-ID:     a0af3d1104f752b6d0dba71788e3fddd67c857a7
Gitweb:        https://git.kernel.org/tip/a0af3d1104f752b6d0dba71788e3fddd67c857a7
Author:        Thomas Gleixner <tglx@linutronix.de>
AuthorDate:    Wed, 19 Jan 2022 18:54:52 +01:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Fri, 21 Jan 2022 02:14:46 +01:00

PCI/MSI: Prevent UAF in error path

When the core MSI allocation fails, then the PCI/MSI code uses an already
freed MSI descriptor to unmask the MSI mask register in order to bring it back
into reset state.

Remove MSI_FLAG_FREE_MSI_DESCS from the PCI/MSI irqdomain flags and let the
PCI/MSI code free the MSI descriptors after usage.

Fixes: 0f62d941acf9 ("genirq/msi: Provide msi_domain_alloc/free_irqs_descs_locked()")
Reported-by: Tong Zhang <ztong0001@gmail.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Tong Zhang <ztong0001@gmail.com>
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
Link: https://lore.kernel.org/r/87r1938vbn.ffs@tglx
---
 drivers/pci/msi/irqdomain.c | 4 ++--
 drivers/pci/msi/legacy.c    | 1 -
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/msi/irqdomain.c b/drivers/pci/msi/irqdomain.c
index 0d63541..e9cf318 100644
--- a/drivers/pci/msi/irqdomain.c
+++ b/drivers/pci/msi/irqdomain.c
@@ -28,6 +28,7 @@ void pci_msi_teardown_msi_irqs(struct pci_dev *dev)
 		msi_domain_free_irqs_descs_locked(domain, &dev->dev);
 	else
 		pci_msi_legacy_teardown_msi_irqs(dev);
+	msi_free_msi_descs(&dev->dev);
 }
 
 /**
@@ -171,8 +172,7 @@ struct irq_domain *pci_msi_create_irq_domain(struct fwnode_handle *fwnode,
 	if (info->flags & MSI_FLAG_USE_DEF_CHIP_OPS)
 		pci_msi_domain_update_chip_ops(info);
 
-	info->flags |= MSI_FLAG_ACTIVATE_EARLY | MSI_FLAG_DEV_SYSFS |
-		       MSI_FLAG_FREE_MSI_DESCS;
+	info->flags |= MSI_FLAG_ACTIVATE_EARLY | MSI_FLAG_DEV_SYSFS;
 	if (IS_ENABLED(CONFIG_GENERIC_IRQ_RESERVATION_MODE))
 		info->flags |= MSI_FLAG_MUST_REACTIVATE;
 
diff --git a/drivers/pci/msi/legacy.c b/drivers/pci/msi/legacy.c
index cdbb468..db761ad 100644
--- a/drivers/pci/msi/legacy.c
+++ b/drivers/pci/msi/legacy.c
@@ -77,5 +77,4 @@ void pci_msi_legacy_teardown_msi_irqs(struct pci_dev *dev)
 {
 	msi_device_destroy_sysfs(&dev->dev);
 	arch_teardown_msi_irqs(dev);
-	msi_free_msi_descs(&dev->dev);
 }