| Message ID | 20221212123348.169903-1-brauner@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [GIT,PULL] vfsuid updates for v6.2 | expand |
On Mon, Dec 12, 2022 at 4:34 AM Christian Brauner <brauner@kernel.org> wrote: > > This pull request converts all remaining places that still make use of non-type > safe idmapping helpers to rely on the new type safe vfs{g,u}id based helpers. > Afterwards it removes all the old non-type safe helpers. So I've pulled this, but I'm not entirely happy about some of those crazy helpers. In particular, the whole "ordering" helpers are really not something that should be used in general, I feel. I'm talking about vfsuid_gt_kuid() and friends - it's an entirely insane operation and makes no sense at all. Yes, yes, I understand why they exist (those crazy IMA rules), but I feel that those functions *really* shouldn't be exposed to anybody else. IOW, making those insane functions available in <linux/idmapping.h> really seems wrong to me. They are crazy special cases, and I think they should exist purely in that crazy ima_security file. Again - I've pulled this, but I'm hoping to see a future commit that limits that craziness to the only user, in the hope that this disease will never spread. Linus
The pull request you sent on Mon, 12 Dec 2022 13:33:48 +0100:
> ssh://git@gitolite.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping.git tags/fs.vfsuid.conversion.standalone.v6.2
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/e4236f97688afc21151bfc050acfce9ac3b56f6b
Thank you!
On Mon, Dec 12, 2022 at 07:28:59PM -0800, Linus Torvalds wrote: > On Mon, Dec 12, 2022 at 4:34 AM Christian Brauner <brauner@kernel.org> wrote: > > > > This pull request converts all remaining places that still make use of non-type > > safe idmapping helpers to rely on the new type safe vfs{g,u}id based helpers. > > Afterwards it removes all the old non-type safe helpers. > > So I've pulled this, but I'm not entirely happy about some of those > crazy helpers. > > In particular, the whole "ordering" helpers are really not something > that should be used in general, I feel. I'm talking about > vfsuid_gt_kuid() and friends - it's an entirely insane operation and > makes no sense at all. Oh yes, I very much agree. > > Yes, yes, I understand why they exist (those crazy IMA rules), but I I would've really liked to have avoided their existence altogether but I have no clear idea what ima is doing with these comparisons. And everytime we do wider scoped vfs work I spend about 1 or 2 good weeks in security/ just to understand what all the various security modules do, audit callchains and then come up with something that doesn't break half of them. And often this means unpleasant compromises in the vfs layer which I really don't like. And just to be clear, I don't want to be on of those "LSMs are bad" people. I do really think they provide additional value. But I think it's fair to acknowledge that the hook infrastructure with multiple LSMs makes the vfs and developers pay when reworking codepaths. And the fact that some things that are LSM-like (ima etc.) have separate hooks doesn't help either. > feel that those functions *really* shouldn't be exposed to anybody > else. > > IOW, making those insane functions available in <linux/idmapping.h> > really seems wrong to me. They are crazy special cases, and I think > they should exist purely in that crazy ima_security file. > > Again - I've pulled this, but I'm hoping to see a future commit that > limits that craziness to the only user, in the hope that this disease > will never spread. Let me see what I can do about this. Hopefully I can still find something during the merge window.
On 13/12/2022 04.28, Linus Torvalds wrote: > On Mon, Dec 12, 2022 at 4:34 AM Christian Brauner <brauner@kernel.org> wrote: >> >> This pull request converts all remaining places that still make use of non-type >> safe idmapping helpers to rely on the new type safe vfs{g,u}id based helpers. >> Afterwards it removes all the old non-type safe helpers. > > So I've pulled this, but I'm not entirely happy about some of those > crazy helpers. > > In particular, the whole "ordering" helpers are really not something > that should be used in general, I feel. I'm talking about > vfsuid_gt_kuid() and friends - it's an entirely insane operation and > makes no sense at all. > > Yes, yes, I understand why they exist (those crazy IMA rules), but I > feel that those functions *really* shouldn't be exposed to anybody > else. > > IOW, making those insane functions available in <linux/idmapping.h> > really seems wrong to me. They are crazy special cases, and I think > they should exist purely in that crazy ima_security file. Yeah. Aside from assigning any semantics to < or > of [ug]ids, which is something IMA apparently wants to do, taking the address of a static inline in the first place is a code smell; that obviously forces the compiler to emit a copy in the current TU. But the code compares stored pointers to addresses of those static inlines, which would be completely broken if this didn't happen to all be contained in a single TU. That's quite subtle, and probably fowner_op would be better as an enum. Rasmus
Hey Linus, /* Summary */ Last cycle we introduced the vfs{g,u}id_t types and associated helpers to gain type safety when dealing with idmapped mounts. That initial pull request back then already converted a lot of places over but there were still some left, This pull request converts all remaining places that still make use of non-type safe idmapping helpers to rely on the new type safe vfs{g,u}id based helpers. Afterwards it removes all the old non-type safe helpers. Note that this pull request has the setgid inheritance branch merged in as the setgid inheritance branch unifies multiple open-coded checks into a single helper making the conversion here easier. I've sent a pull request for that work rearlier so it's on the list and in your inbox before this one. The lore url is: https://lore.kernel.org/lkml/20221212112053.99208-1-brauner@kernel.org In case you don't want to pull "setgid inheritance updates for v6.2" but still would like to pull the remaining vfs{g,u}id_t conversions (That would be greatly appreciated as it gets rid of duplicated functionality between the different helpers.) I prepared the tag fs.vfsuid.conversion.standalone.v6.2 This tag only contains all the vfs{g,u}id_t patches without any of the "setgid inheritance updates for v6.2" patches. ssh://git@gitolite.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping.git tags/fs.vfsuid.conversion.standalone.v6.2 /* Testing */ clang: Ubuntu clang version 15.0.2-1 gcc: gcc (Ubuntu 12.2.0-3ubuntu1) 12.2.0 All patches are based on v6.1-rc1 and have been sitting in linux-next. No build failures or warnings were observed. The vfsuid conversionn portion passes all old and new tests in fstests, selftests, and LTP pass without regressions. /* Conflicts */ At the time of creating this PR no merge conflicts were reported from linux-next and no merge conflicts showed up doing a test-merge with current mainline. /* Conflicts */ At the time of creating this PR no merge conflicts were reported from linux-next and no merge conflicts showed up doing a test-merge with current mainline. The following changes since commit 9abf2313adc1ca1b6180c508c25f22f9395cc780: Linux 6.1-rc1 (2022-10-16 15:36:24 -0700) are available in the Git repository at: ssh://git@gitolite.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping.git tags/fs.vfsuid.conversion.v6.2 __Alternatively__, a standalone version without the setgid patches merged in can be found at: ssh://git@gitolite.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping.git tags/fs.vfsuid.conversion.standalone.v6.2 for you to fetch changes up to eb7718cdb73c6b0c93002f8f73f4dd4701f8d2bb: fs: remove unused idmapping helpers (2022-10-26 10:03:34 +0200) Please consider pulling these changes from the signed fs.vfsuid.conversion.v6.2 or fs.vfsuid.conversion.standalone.v6.2 tag. Thanks! Christian ---------------------------------------------------------------- fs.vfsuid.conversion.v6.2 ---------------------------------------------------------------- Amir Goldstein (2): ovl: remove privs in ovl_copyfile() ovl: remove privs in ovl_fallocate() Christian Brauner (12): attr: add in_group_or_capable() fs: move should_remove_suid() attr: add setattr_should_drop_sgid() attr: use consistent sgid stripping checks mnt_idmapping: add missing helpers fs: use type safe idmapping helpers caps: use type safe idmapping helpers apparmor: use type safe idmapping helpers ima: use type safe idmapping helpers fuse: port to vfs{g,u}id_t and associated helpers ovl: port to vfs{g,u}id_t and associated helpers fs: remove unused idmapping helpers Documentation/trace/ftrace.rst | 2 +- fs/attr.c | 74 +++++++++++++++++++++++--- fs/coredump.c | 4 +- fs/exec.c | 16 +++--- fs/fuse/acl.c | 2 +- fs/fuse/file.c | 2 +- fs/inode.c | 72 ++++++++++++-------------- fs/internal.h | 10 +++- fs/namei.c | 40 +++++++-------- fs/ocfs2/file.c | 4 +- fs/open.c | 8 +-- fs/overlayfs/file.c | 28 ++++++++-- fs/overlayfs/util.c | 9 +++- fs/remap_range.c | 2 +- fs/stat.c | 7 ++- include/linux/fs.h | 36 +------------ include/linux/mnt_idmapping.h | 100 ++++++++++++------------------------ kernel/capability.c | 4 +- security/apparmor/domain.c | 8 +-- security/apparmor/file.c | 4 +- security/apparmor/lsm.c | 25 ++++++--- security/commoncap.c | 51 +++++++++--------- security/integrity/ima/ima_policy.c | 34 ++++++------ 23 files changed, 289 insertions(+), 253 deletions(-)