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 <[email protected]>
---
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
On Mon, 17 Jan 2022 09:27:59 +0000,
Tong Zhang <[email protected]> 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.
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 <[email protected]> wrote:
>
> On Mon, 17 Jan 2022 09:27:59 +0000,
> Tong Zhang <[email protected]> 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.
Please avoid top-posting.
On Mon, 17 Jan 2022 10:10:13 +0000,
Tong Zhang <[email protected]> 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.
On Mon, Jan 17 2022 at 11:36, Marc Zyngier wrote:
> On Mon, 17 Jan 2022 10:10:13 +0000,
> Tong Zhang <[email protected]> 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.
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 <[email protected]> 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
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 <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
---
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);
}
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 <[email protected]>
> Signed-off-by: Thomas Gleixner <[email protected]>
Acked-by: Bjorn Helgaas <[email protected]>
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);
> }
On Wed, Jan 19, 2022 at 9:54 AM Thomas Gleixner <[email protected]> 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 <[email protected]>
> Signed-off-by: Thomas Gleixner <[email protected]>
> ---
> 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 <[email protected]>
Tested on my setup, it no longer crashes.
Thanks!
- Tong
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 <[email protected]>
AuthorDate: Wed, 19 Jan 2022 18:54:52 +01:00
Committer: Thomas Gleixner <[email protected]>
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 <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Tested-by: Tong Zhang <[email protected]>
Acked-by: Bjorn Helgaas <[email protected]>
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);
}