| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <YqioNIOaAWfgX5sk@kroah.com>
Date: Tue, 14 Jun 2022 17:24:36 +0200
From: Greg KH <gregkh@...uxfoundation.org>
To: Julian Vetter <jvetter@...rayinc.com>
Cc: rafael@...nel.org, linux-kernel@...r.kernel.org,
ysionneau@...rayinc.com, jmaselbas@...rayinc.com
Subject: Re: [PATCH v2 2/2] msi: Add sanity check if more than MAX_DEV_MSIS
MSIs are requested
On Tue, Jun 14, 2022 at 05:14:11PM +0200, Julian Vetter wrote:
> If a device requests more than MAX_DEV_MSIS the MSI index will collide with
> the devid and might cause Linux to compute twice the same virtual interrupt
> number for two different devices.
>
> Signed-off-by: Julian Vetter <jvetter@...rayinc.com>
> Reviewed-by: Yann Sionneau <ysionneau@...rayinc.com>
> ---
> Changes v1->v2:
> - Took Greg Kroah-Hartman's comments into account
> - Replaced WARN_ON() by proper error handling
>
> drivers/base/platform-msi.c | 8 +++++++-
> drivers/bus/fsl-mc/fsl-mc-msi.c | 3 ++-
> drivers/irqchip/irq-ti-sci-inta.c | 3 ++-
> drivers/pci/controller/vmd.c | 3 ++-
> drivers/pci/msi/irqdomain.c | 3 ++-
> include/linux/msi.h | 2 +-
> kernel/irq/msi.c | 12 +++++++++---
> 7 files changed, 25 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/base/platform-msi.c b/drivers/base/platform-msi.c
> index 4b0b2fe3a7ff..8330153c0067 100644
> --- a/drivers/base/platform-msi.c
> +++ b/drivers/base/platform-msi.c
> @@ -43,10 +43,16 @@ static irq_hw_number_t platform_msi_calc_hwirq(struct msi_desc *desc)
> return (devid << (32 - DEV_ID_SHIFT)) | desc->msi_index;
> }
>
> -static void platform_msi_set_desc(msi_alloc_info_t *arg, struct msi_desc *desc)
> +static int platform_msi_set_desc(msi_alloc_info_t *arg, struct msi_desc *desc)
> {
> arg->desc = desc;
> arg->hwirq = platform_msi_calc_hwirq(desc);
> +
> + if (desc->msi_index >= MAX_DEV_MSIS) {
> + dev_err(desc->dev, "Number of msis exceeds %d\n", MAX_DEV_MSIS);
> + return -EINVAL;
> + }
> + return 0;
> }
>
> static int platform_msi_init(struct irq_domain *domain,
> diff --git a/drivers/bus/fsl-mc/fsl-mc-msi.c b/drivers/bus/fsl-mc/fsl-mc-msi.c
> index 0cfe859a4ac4..63b5ae8de391 100644
> --- a/drivers/bus/fsl-mc/fsl-mc-msi.c
> +++ b/drivers/bus/fsl-mc/fsl-mc-msi.c
> @@ -32,12 +32,13 @@ static irq_hw_number_t fsl_mc_domain_calc_hwirq(struct fsl_mc_device *dev,
> return (irq_hw_number_t)(desc->msi_index + (dev->icid * 10000));
> }
>
> -static void fsl_mc_msi_set_desc(msi_alloc_info_t *arg,
> +static int fsl_mc_msi_set_desc(msi_alloc_info_t *arg,
> struct msi_desc *desc)
> {
> arg->desc = desc;
> arg->hwirq = fsl_mc_domain_calc_hwirq(to_fsl_mc_device(desc->dev),
> desc);
> + return 0;
> }
> #else
> #define fsl_mc_msi_set_desc NULL
> diff --git a/drivers/irqchip/irq-ti-sci-inta.c b/drivers/irqchip/irq-ti-sci-inta.c
> index 5fdbb4358dd0..0c26690c074b 100644
> --- a/drivers/irqchip/irq-ti-sci-inta.c
> +++ b/drivers/irqchip/irq-ti-sci-inta.c
> @@ -589,13 +589,14 @@ static struct irq_chip ti_sci_inta_msi_irq_chip = {
> .flags = IRQCHIP_SUPPORTS_LEVEL_MSI,
> };
>
> -static void ti_sci_inta_msi_set_desc(msi_alloc_info_t *arg,
> +static int ti_sci_inta_msi_set_desc(msi_alloc_info_t *arg,
> struct msi_desc *desc)
> {
> struct platform_device *pdev = to_platform_device(desc->dev);
>
> arg->desc = desc;
> arg->hwirq = TO_HWIRQ(pdev->id, desc->msi_index);
> + return 0;
> }
>
> static struct msi_domain_ops ti_sci_inta_msi_ops = {
> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index 94a14a3d7e55..929ba2a991b5 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -296,9 +296,10 @@ static int vmd_msi_prepare(struct irq_domain *domain, struct device *dev,
> return 0;
> }
>
> -static void vmd_set_desc(msi_alloc_info_t *arg, struct msi_desc *desc)
> +static int vmd_set_desc(msi_alloc_info_t *arg, struct msi_desc *desc)
> {
> arg->desc = desc;
> + return 0;
> }
>
> static struct msi_domain_ops vmd_msi_domain_ops = {
> diff --git a/drivers/pci/msi/irqdomain.c b/drivers/pci/msi/irqdomain.c
> index e9cf318e6670..3191f15a07b8 100644
> --- a/drivers/pci/msi/irqdomain.c
> +++ b/drivers/pci/msi/irqdomain.c
> @@ -108,11 +108,12 @@ static int pci_msi_domain_check_cap(struct irq_domain *domain,
> return 0;
> }
>
> -static void pci_msi_domain_set_desc(msi_alloc_info_t *arg,
> +static int pci_msi_domain_set_desc(msi_alloc_info_t *arg,
> struct msi_desc *desc)
> {
> arg->desc = desc;
> arg->hwirq = pci_msi_domain_calc_hwirq(desc);
> + return 0;
> }
>
> static struct msi_domain_ops pci_msi_domain_ops_default = {
> diff --git a/include/linux/msi.h b/include/linux/msi.h
> index fc918a658d48..17228d6e8d85 100644
> --- a/include/linux/msi.h
> +++ b/include/linux/msi.h
> @@ -355,7 +355,7 @@ struct msi_domain_ops {
> int (*msi_prepare)(struct irq_domain *domain,
> struct device *dev, int nvec,
> msi_alloc_info_t *arg);
> - void (*set_desc)(msi_alloc_info_t *arg,
> + int (*set_desc)(msi_alloc_info_t *arg,
> struct msi_desc *desc);
> int (*domain_alloc_irqs)(struct irq_domain *domain,
> struct device *dev, int nvec);
> diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
> index a9ee535293eb..a20c85334b58 100644
> --- a/kernel/irq/msi.c
> +++ b/kernel/irq/msi.c
> @@ -592,10 +592,11 @@ static int msi_domain_ops_prepare(struct irq_domain *domain, struct device *dev,
> return 0;
> }
>
> -static void msi_domain_ops_set_desc(msi_alloc_info_t *arg,
> +static int msi_domain_ops_set_desc(msi_alloc_info_t *arg,
> struct msi_desc *desc)
> {
> arg->desc = desc;
> + return 0;
> }
>
> static int msi_domain_ops_init(struct irq_domain *domain,
> @@ -726,7 +727,10 @@ int msi_domain_populate_irqs(struct irq_domain *domain, struct device *dev,
> desc = xa_load(&dev->msi.data->__store, virq);
> desc->irq = virq;
>
> - ops->set_desc(arg, desc);
> + ret = ops->set_desc(arg, desc);
> + if (ret)
> + goto fail;
> +
> ret = irq_domain_alloc_irqs_hierarchy(domain, virq, 1, arg);
> if (ret)
> goto fail;
> @@ -888,7 +892,9 @@ int __msi_domain_alloc_irqs(struct irq_domain *domain, struct device *dev,
> }
>
> msi_for_each_desc(desc, dev, MSI_DESC_NOTASSOCIATED) {
> - ops->set_desc(&arg, desc);
> + ret = ops->set_desc(&arg, desc);
> + if (ret)
> + return ret;
Shouldn't you unwind anything that was already allocated before this
call?
thanks,
greg k-h
Powered by blists - more mailing lists