linux-kernel - Re: Regression on Macchiatobin from the irqchip driver

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87frqvh9wz.ffs@tglx>
Date: Fri, 23 Aug 2024 11:21:16 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Maxime Chevallier <maxime.chevallier@...tlin.com>, Andrew Lunn
 <andrew@...n.ch>, Gregory Clement <gregory.clement@...tlin.com>, Sebastian
 Hesselbarth <sebastian.hesselbarth@...il.com>, Russell King
 <linux@...linux.org.uk>
Cc: linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
 Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: Regression on Macchiatobin from the irqchip driver

On Wed, Aug 21 2024 at 16:50, Maxime Chevallier wrote:
> By looking at the msi_lib_irq_domain_select() implementation however, I
> notice that it appears to be expected that these ops can be NULL by
> looking at the check in the return line :
>
> 	return ops && !!(ops->bus_select_mask & busmask);
>
> However, the line above dereferences the ops pointer without prior
> check :
>
> 	/* Handle pure domain searches */
> 	if (bus_token == ops->bus_select_token)
> 		return 1;

Oops.

> As I said, this area of the kernel isn't very familiar to me, but I got
> my board to boot with the following patch :
>
> --- a/drivers/irqchip/irq-msi-lib.c
> +++ b/drivers/irqchip/irq-msi-lib.c
> @@ -128,6 +128,9 @@ int msi_lib_irq_domain_select(struct irq_domain *d, struct irq_fwspec *fwspec,
>         const struct msi_parent_ops *ops = d->msi_parent_ops;
>         u32 busmask = BIT(bus_token);
>  
> +       if (!ops)
> +               return 0;
> +
>         if (fwspec->fwnode != d->fwnode || fwspec->param_count != 0)
>                 return 0;
>  
> @@ -135,6 +138,6 @@ int msi_lib_irq_domain_select(struct irq_domain *d, struct irq_fwspec *fwspec,
>         if (bus_token == ops->bus_select_token)
>                 return 1;
>  
> -       return ops && !!(ops->bus_select_mask & busmask);
> +       return !!(ops->bus_select_mask & busmask);
>
> ----------------------------
>
> I have zero confidence that this is the correct solution to the issue
> so feel free to ditch that solution :) I'll gladly test any
> patch for that on the MCBIN.

It obviously is the proper solution check after use is pretty pointless
as you demonstrated. Care to send a proper patch?

Thanks,

        tglx