linux-kernel - Re: [PATCH] genirq: procfs: Make smp_affinity read-only for interrupts marked with IRQD_AFFINITY

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: <875xrrgicx.ffs@tglx>
Date: Fri, 23 Aug 2024 21:16:30 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Jeff Xie <jeff.xie@...ux.dev>
Cc: linux-kernel@...r.kernel.org, xiehuan09@...il.com, Jeff Xie
 <jeff.xie@...ux.dev>
Subject: Re: [PATCH] genirq: procfs: Make smp_affinity read-only for
 interrupts marked with IRQD_AFFINITY_MANAGED flag

On Tue, Aug 20 2024 at 10:09, Jeff Xie wrote:
> Currently, due to the interrupt subsystem introduced this commit 9c2555835bb3
> ("genirq: Introduce IRQD_AFFINITY_MANAGED flag"),

This is not really a proper sentence.

> an error is reported when a
> system administrator modifies the smp_affinity for the virtio_blk driver.
> For example:
>
> jeff-labs:/proc/irq/26 # echo 2 > ./smp_affinity
> -bash: echo: write error: Input/output error

That should obviously return -EPERM for managed interrupts.

> However, checking the permissions of smp_affinity/smp_affinity_list shows that
> they are set to rw. System administrators are strongly complaining about this issue.

System administrators complain strongly about a lot of things. Such
complaints are not necessarily a technical reason to change the code.

A proper reason is to argue, that the kernel already knows at the time
of interrupt allocation that the affinity cannot be controlled by
userspace and therefore creating the file with write permissions is
wrong.

> jeff-labs:/proc/irq/26 # ls -l
> total 0
> -r--r--r-- 1 root root 0 Aug 20 01:32 affinity_hint
> -r--r--r-- 1 root root 0 Aug 20 01:32 effective_affinity
> -r--r--r-- 1 root root 0 Aug 20 01:32 effective_affinity_list
> -r--r--r-- 1 root root 0 Aug 20 01:32 node
> -rw-r--r-- 1 root root 0 Aug 20 01:32 smp_affinity
> -rw-r--r-- 1 root root 0 Aug 20 01:32 smp_affinity_list
> -r--r--r-- 1 root root 0 Aug 20 01:32 spurious
> dr-xr-xr-x 2 root root 0 Aug 20 01:32 virtio3-req.0

We can see that from the code, no?

> Therefore, the permissions of smp_affinity/smp_affinity_list should be changed to read-only.

Should? Tell what the solution is:

Therefore set the file permissions to read-only for such interrupts.

And please format you change log so that it has linebreaks around 75
characters.

>  
>  #ifdef CONFIG_SMP
>  	/* create /proc/irq/<irq>/smp_affinity */
> -	proc_create_data("smp_affinity", 0644, desc->dir,
> +	if (unlikely(irqd_affinity_is_managed(&desc->irq_data)))

This unlikely is a pointless exercise as this is not a hotpath
operation. Also please switch to S_IRUGO / S_IWUSR and simplify the
whole thing to:

	umode_t umode = S_IRUGO;

	if (!irqd_affinity_is_managed(&desc->irq_data))
		umode |= S_IWUSR;
	proc_create_data("smp_affinity", umode, desc->dir, &irq_affinity_proc_ops, irqp);

Thanks,

        tglx