On Tue, Nov 09, 2021 at 03:57:12PM +0100, Christian Brauner wrote: > From: Christian Brauner <christian.brauner@xxxxxxxxxx> > > When calling setattr_prepare() to determine the validity of the attributes the > ia_{g,u}id fields contain the value that will be written to inode->i_{g,u}id. > When the {g,u}id attribute of the file isn't altered and the caller's fs{g,u}id > matches the current {g,u}id attribute the attribute change is allowed. > > The value in ia_{g,u}id does already account for idmapped mounts and will have > taken the relevant idmapping into account. So in order to verify that the > {g,u}id attribute isn't changed we simple need to compare the ia_{g,u}id value > against the inode's i_{g,u}id value. > > This only has any meaning for idmapped mounts as idmapping helpers are > idempotent without them. And for idmapped mounts this really only has a meaning > when circular idmappings are used, i.e. mappings where e.g. id 1000 is mapped > to id 1001 and id 1001 is mapped to id 1000. Such ciruclar mappings can e.g. be > useful when sharing the same home directory between multiple users at the same > time. > > As an example consider a directory with two files: /source/file1 owned by > {g,u}id 1000 and /source/file2 owned by {g,u}id 1001. Assume we create an > idmapped mount at /target with an idmapping that maps files owned by {g,u}id > 1000 to being owned by {g,u}id 1001 and files owned by {g,u}id 1001 to being > owned by {g,u}id 1000. In effect, the idmapped mount at /target switches the > ownership of /source/file1 and source/file2, i.e. /target/file1 will be owned > by {g,u}id 1001 and /target/file2 will be owned by {g,u}id 1000. > > This means that a user with fs{g,u}id 1000 must be allowed to setattr > /target/file2 from {g,u}id 1000 to {g,u}id 1000. Similar, a user with fs{g,u}id > 1001 must be allowed to setattr /target/file1 from {g,u}id 1001 to {g,u}id > 1001. Conversely, a user with fs{g,u}id 1000 must fail to setattr /target/file1 > from {g,u}id 1001 to {g,u}id 1000. And a user with fs{g,u}id 1001 must fail to > setattr /target/file2 from {g,u}id 1000 to {g,u}id 1000. Both cases must fail > with EPERM for non-capable callers. > > Before this patch we could end up denying legitimate attribute changes and > allowing invalid attribute changes when circular mappings are used. To even get > into this situation the caller must've been privileged both to create that > mapping and to create that idmapped mount. > > This hasn't been seen in the wild anywhere but came up when expanding the > testsuite during work on a series of hardening patches. All idmapped fstests > pass without any regressions and we add new tests to verify the behavior of > circular mappings. > > Fixes: 2f221d6f7b88 ("attr: handle idmapped mounts") > Cc: Seth Forshee <seth.forshee@xxxxxxxxxxxxxxxx> > Cc: Christoph Hellwig <hch@xxxxxx> > Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx> > Cc: stable@xxxxxxxxxxxxxxx > CC: linux-fsdevel@xxxxxxxxxxxxxxx > Signed-off-by: Christian Brauner <christian.brauner@xxxxxxxxxx> This looks right to me. Acked-by: Seth Forshee <sforshee@xxxxxxxxxxxxxxxx>