linux-kernel - Re: [GIT PULL] fs fix for v5.19-rc3

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] [day] [month] [year] [list]

Message-ID: <20220616075915.otq7xzchci5jbh6z@wittgenstein>
Date:   Thu, 16 Jun 2022 09:59:15 +0200
From:   Christian Brauner <brauner@...nel.org>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [GIT PULL] fs fix for v5.19-rc3

On Wed, Jun 15, 2022 at 09:31:32AM -0700, Linus Torvalds wrote:
> On Wed, Jun 15, 2022 at 3:39 AM Christian Brauner <brauner@...nel.org> wrote:
> >
> > This fixes an issue where we fail to change the group of a file when the caller
> > owns the file and is a member of the group to change to. This is only relevant
> > on idmapped mounts.
> 
> Pulled, but this code makes me go "Eww".
> 
> I do get the feeling that maybe we should have some kind of static
> type help here, with "unmapped" and "filesystem mapped" uid/gid values
> having different types, the same way we have kuid_t vs uid_t for
> user-namespace mapping.
> 
> Because it feels like the problem here is that "kgid_t" has different
> meanings depending on context.
> 
> In fact, just within that one function, we have *three* very different kgid_t's:
> 
>  - the argument to the function is a "kgid_t gid", which has *not*
> been mapped into the filesystem mapping (why not?)
> 
>  - the function itself declares a "kgid_t kgid", which is the inode
> group ID mapped into the mount
> 
>  - it how additionally has that thied "kgid_t mapped_gid", which is
> the first 'gid' mapped into the mount
> 
> and honestly, I find that hugely confusing. The naming doesn't exactly
> clarify things either (ie "gid" is a "kgid_t", not a "git_t", and so
> is "kgid" - not helpful).
> 
> And being confusing with no type system support isn't a great thing.
> 
> So I'm wondering if <linux/mnt_idmapping.h> might be able to do the
> same kind of things we do in <linux/uidgid.h>.
> 
> NOTE: I did not look into just how hugely painful that would be. Maybe
> the above is the rantings of a madman.

I'm actually looking into this and have been for a while. I had thought
about this from the start but just now have time to play with this!