linux-kernel - Re: [PATCH 1/1] x86/vector: Fix vector leak during CPU offline

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: <e7047fd5-8bcf-42aa-9729-125bb7304f35@linux.intel.com>
Date: Fri, 10 May 2024 12:48:15 -0700
From: Dave Hansen <dave.hansen@...ux.intel.com>
To: Dongli Zhang <dongli.zhang@...cle.com>, x86@...nel.org
Cc: tglx@...utronix.de, mingo@...hat.com, Borislavbp@...en8.de,
 dave.hansen@...ux.intel.com, hpa@...or.com, joe.jin@...cle.com,
 linux-kernel@...r.kernel.org, virtualization@...ts.linux.dev
Subject: Re: [PATCH 1/1] x86/vector: Fix vector leak during CPU offline

On 5/10/24 12:06, Dongli Zhang wrote:
>  	} else {
> +		/*
> +		 * This call borrows from the comments and implementation
> +		 * of apic_update_vector(): "If the target CPU is offline
> +		 * then the regular release mechanism via the cleanup
> +		 * vector is not possible and the vector can be immediately
> +		 * freed in the underlying matrix allocator.".
> +		 */
> +		irq_matrix_free(vector_matrix, apicd->prev_cpu,
> +				apicd->prev_vector, apicd->is_managed);
>  		apicd->prev_vector = 0;
>  	}

I know it's just two sites, but I'd much rather spend the space on a
helper function than a copy-and-pasted comment.  Wouldn't something like
this make it like stupidly obvious what's going on:

        if (cpu_online(cpu)) {
		...
        } else {
		irq_matrix_free_offline(apicd->prev_cpu,
					apicd->prev_vector,
					apicd->is_managed);
                apicd->prev_vector = 0;
        }

/* Free a vector when the target CPU is offline */
static void irq_matrix_free_offline(...)
{
	lockdep_assert_held(&vector_lock);
	WARN_ON_ONCE(!cpu_offline(apicd->prev_cpu));

	/*
	 * The regular release mechanism via the cleanup vector is not
	 * possible.  Immediately free the vector in the underlying
	 * matrix allocator.
 	 */
	irq_matrix_free(&whatever, cpu, vector, managed);
}

It would also be rather hard to screw up even if someone called it on an
online CPU because you'd get a nice warning.