Re: [oss-security] Linux kernel: memory leak in arch/powerpc/platforms/powernv/opal-irqchip.c: opal_event_init()

[Michael Ellerman <mpe@ellerman.id.au>]

Solar Designer <solar@openwall.com> writes:
> Hi,
>
> This bug report is misaddressed.  Per upstream's preference and common
> sense (given how many issue reports there are against the Linux kernel),
> most Linux kernel (maybe-)issues should first be reported to Linux
> kernel maintainers/lists or (if you're reasonably sure of significant
> security relevance) to the Linux kernel security team, and only then (if
> relevant) maybe also to general security lists.  Occasional exceptions
> exist, such as for publicly exploited issues, but if you're new to this
> chances are that you should play by the rules, not claim an exception.

This was also reported to security@kernel.org, and I replied saying
there was no real security impact and that it should be reported to
linuxppc-dev@lists.ozlabs.org.

> On Mon, Sep 02, 2024 at 09:54:52AM +0800, 2639161967 wrote:
>> in the newest linux release version, in&nbsp;/arch/powerpc/platforms/powernv/opal-irqchip.c&nbsp;file , the&nbsp;
>> opal_event_init function, the variable "name"defined in line 270, and is alloced memory in line 274 or 276, but not free, cause many times memory leak, and most old release versions have the problem.
>
> The code in question is:
>
> int __init opal_event_init(void)
> {
> [...]
> 	/* Install interrupt handlers */
> 	for (i = 0; i < opal_irq_count; i++) {
> 		struct resource *r = &opal_irqs[i];
> 		const char *name;
>
> 		/* Prefix name */
> 		if (r->name && strlen(r->name))
> 			name = kasprintf(GFP_KERNEL, "opal-%s", r->name);
> 		else
> 			name = kasprintf(GFP_KERNEL, "opal");
>
> 		if (!name)
> 			continue;
> 		/* Install interrupt handler */
> 		rc = request_irq(r->start, opal_interrupt, r->flags & IRQD_TRIGGER_MASK,
> 				 name, NULL);
> 		if (rc) {
> 			pr_warn("Error %d requesting OPAL irq %d\n", rc, (int)r->start);
> 			continue;
> 		}
> 	}
...
>
> I'm CC'ing this to some maintainers in case they want to add to the "if
> (rc) {" block, if only to silence static analysis tools, but other than
> that I see nothing to do on this report.

If the request_irq() fails it's either some sort of firmware
misconfiguration, or ENOMEM, both which are possibly fatal to the
machine.

But still, it would be reasonable to kfree() name in the error path, as
you say if only to keep the static analysis clean.

Thanks for looking at it and sorry for the noise on your lists.

cheers