linux-kernel - Re: [PATCH] security: new security_file_ioctl

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: <CAEjxPJ7kLwQAee+J1RJ_AvoVLkJR2L5dyZ0jFJHbazZANWgeyQ@mail.gmail.com>
Date: Wed, 20 Dec 2023 10:34:38 -0500
From: Stephen Smalley <stephen.smalley.work@...il.com>
To: Alfred Piccioni <alpic@...gle.com>
Cc: Paul Moore <paul@...l-moore.com>, Eric Paris <eparis@...isplace.org>, 
	LSM List <linux-security-module@...r.kernel.org>, 
	Linux FS Devel <linux-fsdevel@...r.kernel.org>, stable@...r.kernel.org, 
	SElinux list <selinux@...r.kernel.org>, linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] security: new security_file_ioctl_compat() hook

On Tue, Dec 19, 2023 at 4:11 AM Alfred Piccioni <alpic@...gle.com> wrote:
>
> Thanks for taking the time to review! Apologies for the number of
> small mistakes.

NP.

> > Also, IIRC, Paul prefers putting a pair of parentheses after function
> > names to distinguish them, so in the subject line
> > and description it should be security_file_ioctl_compat() and
> > security_file_ioctl(), and you should put a patch version
> > in the [PATCH] prefix e.g. [PATCH v3] to make clear that it is a later
> > version, and usually one doesn't capitalize SELinux
> > or the leading verb in the subject line (just "selinux: introduce").
>
> Changed title to lower-case, prefixed with security, changed slightly
> to fit in summary with new parentheses. Added [PATCH V3] to the
> subject.

Patch description still doesn't include the parentheses after each
function name but probably not worth re-spinning unless Paul says to
do so. I don't see the v3 in the subject line. Seemingly that in
combination with the fact that you replied to the original thread
confuses the b4 tool (b4.docs.kernel.org) such that b4 mbox/am/shazam
ends up selecting the v2 patch instead by default.

> > Actually, since this spans more than just SELinux, the prefix likely
> > needs to reflect that (e.g. security: introduce ...)
> > and the patch should go to the linux-security-module mailing list too
> > and perhaps linux-fsdevel for the ioctl change.
>
> Added cc 'selinux@...r.kernel.org' and cc
> 'linux-kernel@...r.kernel.org'. Thanks!

Just FYI, scripts/get_maintainer.pl /path/to/patch will provide an
over-approximation of who to include on the distribution for patches
based on MAINTAINERS and recent committers. That said, I generally
prune the set it provides. More art than science.

> > I didn't do an audit but does anything need to be updated for the BPF
> > LSM or does it auto-magically pick up new hooks?
>
> I'm unsure. I looked through the BPF LSM and I can't see any way it's
> picking up the file_ioctl hook to begin with. It appears to me
> skimming through the code that it automagically picks it up, but I'm
> not willing to bet the kernel on it.
>
> Do you know who would be a good person to ask about this to make sure?

Looks like it inherited it via the lsm_hook_defs.h.
$ nm security/bpf/hooks.o | grep ioctl
                 U bpf_lsm_file_ioctl
                 U bpf_lsm_file_ioctl_compat