[PATCH] irqchip/gic: prevent buffer overflow in gic_ipi_send_mask()

[PATCH] irqchip/gic: prevent buffer overflow in gic_ipi_send_mask()

[Sergey Shtylyov]

ARM GIC arch v2 spec claims support for just 8 CPU interfaces.  However,
looking at the GIC driver's irq_set_affinity() method, it seems that the
passed CPU mask may contain the logical CPU #s beyond 8, and that method
filters them out before reading gic_cpu_map[], bailing out with -EINVAL.
Such check should also be performed in the ipi_send_mask() method where
it uses for_each_cpu(), in order to prevent accessing beyond the end of
gic_cpu_map[] there as well...

Found by Linux Verification Center (linuxtesting.org) with the Svace static
analysis tool.

Fixes: 384a290283fd ("ARM: gic: use a private mapping for CPU target interfaces")
Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>

---
The patch is against the irq/core branch of the tip.git repo...

 drivers/irqchip/irq-gic.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Index: tip/drivers/irqchip/irq-gic.c
===================================================================
--- tip.orig/drivers/irqchip/irq-gic.c
+++ tip/drivers/irqchip/irq-gic.c
@@ -832,8 +832,11 @@ static void gic_ipi_send_mask(struct irq
 	gic_lock_irqsave(flags);
 
 	/* Convert our logical CPU mask into a physical one. */
-	for_each_cpu(cpu, mask)
+	for_each_cpu(cpu, mask) {
+		if (cpu >= NR_GIC_CPU_IF)
+			break;
 		map |= gic_cpu_map[cpu];
+	}
 
 	/*
 	 * Ensure that stores to Normal memory are visible to the

Re: [PATCH] irqchip/gic: prevent buffer overflow in gic_ipi_send_mask()

[Thomas Gleixner]

On Wed, Sep 04 2024 at 23:23, Sergey Shtylyov wrote:
> ARM GIC arch v2 spec claims support for just 8 CPU interfaces.  However,
> looking at the GIC driver's irq_set_affinity() method, it seems that the
> passed CPU mask may contain the logical CPU #s beyond 8, and that method
> filters them out before reading gic_cpu_map[], bailing out with
> -EINVAL.

The reasoning is correct in theory, but in reality it's a non problem.

Simply because processors which use this GIC version cannot have more
than 8 cores.

That means num_possible_cpus() <= 8 so the cpumask handed in cannot have
bits >= 8 set. Ergo for_each_cpu() can't return a bit which is >= 8.

Thanks

        tglx

Re: [PATCH] irqchip/gic: prevent buffer overflow in gic_ipi_send_mask()

[Marc Zyngier]

On Thu, 05 Sep 2024 08:29:32 +0100,
Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> On Wed, Sep 04 2024 at 23:23, Sergey Shtylyov wrote:
> > ARM GIC arch v2 spec claims support for just 8 CPU interfaces.  However,
> > looking at the GIC driver's irq_set_affinity() method, it seems that the
> > passed CPU mask may contain the logical CPU #s beyond 8, and that method
> > filters them out before reading gic_cpu_map[], bailing out with
> > -EINVAL.
> 
> The reasoning is correct in theory, but in reality it's a non problem.
> 
> Simply because processors which use this GIC version cannot have more
> than 8 cores.
> 
> That means num_possible_cpus() <= 8 so the cpumask handed in cannot have
> bits >= 8 set. Ergo for_each_cpu() can't return a bit which is >= 8.

That.

The irq_set_affinity() check exists because the affinity can be
provided by userspace, and used to be be *anything*. Since
33de0aa4bae98, the affinity that the driver gets is narrowed to what
is actually *online*.

So we could actually relax the check in the driver (not that it really
matters).

Thanks,

	M.

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

Re: [PATCH] irqchip/gic: prevent buffer overflow in gic_ipi_send_mask()

[Sergey Shtylyov]

On 9/5/24 10:47 AM, Marc Zyngier wrote:
[...]

>>> ARM GIC arch v2 spec claims support for just 8 CPU interfaces.  However,
>>> looking at the GIC driver's irq_set_affinity() method, it seems that the
>>> passed CPU mask may contain the logical CPU #s beyond 8, and that method
>>> filters them out before reading gic_cpu_map[], bailing out with
>>> -EINVAL.
>>
>> The reasoning is correct in theory, but in reality it's a non problem.
>>
>> Simply because processors which use this GIC version cannot have more
>> than 8 cores.
>>
>> That means num_possible_cpus() <= 8 so the cpumask handed in cannot have
>> bits >= 8 set. Ergo for_each_cpu() can't return a bit which is >= 8.
> 
> That.

   That? :-)

> The irq_set_affinity() check exists because the affinity can be
> provided by userspace, and used to be be *anything*. Since

   In this case you mean gic_set_affinity(), right?

> 33de0aa4bae98, the affinity that the driver gets is narrowed to what
> is actually *online*.

   What I haven't quite understood from my (cursory) looking at the GICv2
spec (and the GIC driver) is why only one CPU (with a lowest #) is selected
from *mask_val before writing to GICD_GIC_DIST_TARGET, while the spec holds
that an IRQ can be forwarded to any set of 8 CPU interfaces...
 
> So we could actually relax the check in the driver (not that it really
> matters).

   Well, maybe in my copious free time... :-)

> Thanks,
> 
> 	M.

MBR, Sergey

Re: [PATCH] irqchip/gic: prevent buffer overflow in gic_ipi_send_mask()

[Sergey Shtylyov]

On 9/6/24 11:29 PM, Sergey Shtylyov wrote:
[...]

>>>> ARM GIC arch v2 spec claims support for just 8 CPU interfaces.  However,
>>>> looking at the GIC driver's irq_set_affinity() method, it seems that the
>>>> passed CPU mask may contain the logical CPU #s beyond 8, and that method
>>>> filters them out before reading gic_cpu_map[], bailing out with
>>>> -EINVAL.
>>>
>>> The reasoning is correct in theory, but in reality it's a non problem.
>>>
>>> Simply because processors which use this GIC version cannot have more
>>> than 8 cores.
>>>
>>> That means num_possible_cpus() <= 8 so the cpumask handed in cannot have
>>> bits >= 8 set. Ergo for_each_cpu() can't return a bit which is >= 8.

[...]

>> 33de0aa4bae98, the affinity that the driver gets is narrowed to what
>> is actually *online*.
> 
>    What I haven't quite understood from my (cursory) looking at the GICv2
> spec (and the GIC driver) is why only one CPU (with a lowest #) is selected
> from *mask_val before writing to GICD_GIC_DIST_TARGET, while the spec holds

   Sorry, meant to type GIC_DIST_TARGET (or GICD_ITARGETSRn, as the spec
calls it)...

[...]

>> Thanks,
>>
>> 	M.

MBR, Sergey

Re: [PATCH] irqchip/gic: prevent buffer overflow in gic_ipi_send_mask()

[Marc Zyngier]

On Fri, 06 Sep 2024 21:29:47 +0100,
Sergey Shtylyov <s.shtylyov@omp.ru> wrote:
> 
> On 9/5/24 10:47 AM, Marc Zyngier wrote:
> [...]
> 
> >>> ARM GIC arch v2 spec claims support for just 8 CPU interfaces.  However,
> >>> looking at the GIC driver's irq_set_affinity() method, it seems that the
> >>> passed CPU mask may contain the logical CPU #s beyond 8, and that method
> >>> filters them out before reading gic_cpu_map[], bailing out with
> >>> -EINVAL.
> >>
> >> The reasoning is correct in theory, but in reality it's a non problem.
> >>
> >> Simply because processors which use this GIC version cannot have more
> >> than 8 cores.
> >>
> >> That means num_possible_cpus() <= 8 so the cpumask handed in cannot have
> >> bits >= 8 set. Ergo for_each_cpu() can't return a bit which is >= 8.
> > 
> > That.
> 
>    That? :-)

What Thomas explained.

> 
> > The irq_set_affinity() check exists because the affinity can be
> > provided by userspace, and used to be be *anything*. Since
> 
>    In this case you mean gic_set_affinity(), right?

Yes.

> 
> > 33de0aa4bae98, the affinity that the driver gets is narrowed to what
> > is actually *online*.
> 
>    What I haven't quite understood from my (cursory) looking at the GICv2
> spec (and the GIC driver) is why only one CPU (with a lowest #) is selected
> from *mask_val before writing to GICD_GIC_DIST_TARGET, while the spec holds
> that an IRQ can be forwarded to any set of 8 CPU interfaces...

Because on all the existing implementations, having more than a single
target in GICD_ITARGETSRn results in all the targeted CPUs to be
interrupted, with the guarantee that only one will see the actual
interrupt (the read from GICC_IAR returns a value that is not 0x3ff),
and everyone else will only see a spurious interrupt (0x3ff). This is
because the distributor does not track which CPU is actually in a
position to handle the interrupt.

While this can be, under limited circumstances, beneficial from an
interrupt servicing latency, it is always bad from a global throughput
perspective. You end-up thrashing CPU caches, generating odd latencies
in unsuspecting code, and in general with disappointing performance.

Thankfully, GIC (v1/v2) is a dead horse, and v3 doesn't have this
particular problem (it replaced it with a bigger one in the form of
1:n distribution).

	M.

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

Re: [PATCH] irqchip/gic: prevent buffer overflow in gic_ipi_send_mask()

[Sergey Shtylyov]

On 9/8/24 12:37 PM, Marc Zyngier wrote:
[...]

>>>>> ARM GIC arch v2 spec claims support for just 8 CPU interfaces.  However,
>>>>> looking at the GIC driver's irq_set_affinity() method, it seems that the
>>>>> passed CPU mask may contain the logical CPU #s beyond 8, and that method
>>>>> filters them out before reading gic_cpu_map[], bailing out with
>>>>> -EINVAL.
>>>>
>>>> The reasoning is correct in theory, but in reality it's a non problem.
>>>>
>>>> Simply because processors which use this GIC version cannot have more
>>>> than 8 cores.
>>>>
>>>> That means num_possible_cpus() <= 8 so the cpumask handed in cannot have
>>>> bits >= 8 set. Ergo for_each_cpu() can't return a bit which is >= 8.

[...]

>>> 33de0aa4bae98, the affinity that the driver gets is narrowed to what
>>> is actually *online*.
>>
>>    What I haven't quite understood from my (cursory) looking at the GICv2
>> spec (and the GIC driver) is why only one CPU (with a lowest #) is selected
>> from *mask_val before writing to GICD_GIC_DIST_TARGET, while the spec holds
>> that an IRQ can be forwarded to any set of 8 CPU interfaces...
> 
> Because on all the existing implementations, having more than a single
> target in GICD_ITARGETSRn results in all the targeted CPUs to be
> interrupted, with the guarantee that only one will see the actual
> interrupt (the read from GICC_IAR returns a value that is not 0x3ff),
> and everyone else will only see a spurious interrupt (0x3ff). This is
> because the distributor does not track which CPU is actually in a
> position to handle the interrupt.

   Ah! Previously I was only familiar with the x86 {I/O,local} APICs,
and my recollection was that they somehow manage to negotiate that
matter over the APIC bus... but my knowledge it pretty dated, I've
had almost no part in the x86 Linux development. :-(

> While this can be, under limited circumstances, beneficial from an
> interrupt servicing latency, it is always bad from a global throughput
> perspective. You end-up thrashing CPU caches, generating odd latencies
> in unsuspecting code, and in general with disappointing performance.
> 
> Thankfully, GIC (v1/v2) is a dead horse, and v3 doesn't have this
> particular problem (it replaced it with a bigger one in the form of
> 1:n distribution).

   GICv2 spec does talk about 1-N and N-N interrupt handling modes;
at the same time, I can't find such words in the GICv3/4 spec. :-)

   Thanks a lot for your explanations! Despite being involved in the
ARM dev't since 2008, I have a limited knowledge of the ARM low-level
things... :-(

> 	M.

MBR, Sergey

Re: [PATCH] irqchip/gic: prevent buffer overflow in gic_ipi_send_mask()

[Marc Zyngier]

On Mon, 09 Sep 2024 20:48:24 +0100,
Sergey Shtylyov <s.shtylyov@omp.ru> wrote:
> 
>    GICv2 spec does talk about 1-N and N-N interrupt handling modes;
> at the same time, I can't find such words in the GICv3/4 spec. :-)

See 1.2.3 "Models for handling interrupts" and the pointers this
section contains. The major difference is that 1:N is enabled on a
per-CPU basis instead of a per interrupt basis. Needless to say, Linux
doesn't support this, although I've been pondering it at times.

	M.

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

Re: [PATCH] irqchip/gic: prevent buffer overflow in gic_ipi_send_mask()

[Sergey Shtylyov]

On 9/5/24 10:29 AM, Thomas Gleixner wrote:
[...]

>> ARM GIC arch v2 spec claims support for just 8 CPU interfaces.  However,
>> looking at the GIC driver's irq_set_affinity() method, it seems that the
>> passed CPU mask may contain the logical CPU #s beyond 8, and that method

   s/8/7/, of course... :-<

>> filters them out before reading gic_cpu_map[], bailing out with
>> -EINVAL.
> 
> The reasoning is correct in theory, but in reality it's a non problem.

   Frankly, before finalizing this patch I had tried to ascertain whether
cpumask could contain CPUs with the logical #s higher than 8 but that was
taking way too much time and I gave up... :-)

> Simply because processors which use this GIC version cannot have more
> than 8 cores.

   And big.LITTLE not involved?

> That means num_possible_cpus() <= 8 so the cpumask handed in cannot have
> bits >= 8 set. Ergo for_each_cpu() can't return a bit which is >= 8.

   Perhaps adding WARN_ON() would make some sense though? :-)

> Thanks
> 
>         tglx

MBR, Sergey

Re: [PATCH] irqchip/gic: prevent buffer overflow in gic_ipi_send_mask()

[Marc Zyngier]

On Mon, 09 Sep 2024 20:23:21 +0100,
Sergey Shtylyov <s.shtylyov@omp.ru> wrote:
> 
> On 9/5/24 10:29 AM, Thomas Gleixner wrote:
> [...]
> 
> >> ARM GIC arch v2 spec claims support for just 8 CPU interfaces.  However,
> >> looking at the GIC driver's irq_set_affinity() method, it seems that the
> >> passed CPU mask may contain the logical CPU #s beyond 8, and that method
> 
>    s/8/7/, of course... :-<
> 
> >> filters them out before reading gic_cpu_map[], bailing out with
> >> -EINVAL.
> > 
> > The reasoning is correct in theory, but in reality it's a non problem.
> 
>    Frankly, before finalizing this patch I had tried to ascertain whether
> cpumask could contain CPUs with the logical #s higher than 8 but that was
> taking way too much time and I gave up... :-)

You can't really work it out form the source code. The trick is that
the integration requirements prevent you from doing so. It is as
simple as that. People have built GICv2-like interrupt controllers
with more than 8 CPUs, but it is a different beast (see the hip04
driver for a good laugh).

Another possible hack is to have 2 GICs side by side, and connect up
to 8 CPUs to each. But then you cannot IPI one cluster from another,
and you end-up with the hilarious situation that plagued the Axxia
SoC, which Linux never really supported, because this is utter
nonsense.

> 
> > Simply because processors which use this GIC version cannot have more
> > than 8 cores.
> 
>    And big.LITTLE not involved?

In what sense? Asymmetric configurations don't impact the number of
CPU interfaces that can be connected to a single GIC.

> 
> > That means num_possible_cpus() <= 8 so the cpumask handed in cannot have
> > bits >= 8 set. Ergo for_each_cpu() can't return a bit which is >= 8.
> 
>    Perhaps adding WARN_ON() would make some sense though? :-)

But why? If someone builds something that cannot work, they have
bigger problems than an extra bit in a bitmap, and the kernel is not a
validation suite for idiotic integration (though I wonder at times).

	M.

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