Message-ID: <0583de50-1acd-4b61-911b-b1e9ed9a843e@auristor.com>
Date: Tue, 13 May 2025 11:44:51 -0400
From: Jeffrey E Altman <jaltman@...istor.com>
To: David Howells <dhowells@...hat.com>,
 Christian Brauner <brauner@...nel.org>
Cc: Alexander Viro <viro@...iv.linux.org.uk>,
 Etienne Champetier <champetier.etienne@...il.com>,
 Marc Dionne <marc.dionne@...istor.com>, Chet Ramey <chet.ramey@...e.edu>,
 Steve French <sfrench@...ba.org>, linux-afs@...ts.infradead.org,
 openafs-devel@...nafs.org, linux-cifs@...r.kernel.org,
 linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] afs, bash: Fix open(O_CREAT) on an extant AFS file in
 a sticky dir

I performed a review of the usage of vfsuid_eq_kuid() and vfsuid_eq().
I mostly agree with David's conclusions and add some additional insight 
into the behavior of AFS servers.

On 5/13/2025 4:30 AM, David Howells wrote:
> Christian Brauner <brauner@...nel.org> wrote:
>
>> There's a few other places where we compare vfsuids:
>>
>> * may_delete()
>>    -> check_sticky()
>>       -> __check_sticky()
>>
>> * may_follow_link()
>>
>> * may_linkat()
>>
>> * fsuidgid_has_mapping()
>>
>> Anyone of those need special treatment on AFS as well?
> That's a good question.  I think it might be better to switch back to the v1
> patch - which gives me two separate ops and provide a couple of vfs wrappers
> for them and use them more widely.
>
> So, perhaps:
>
> 	vfs_have_same_owner(inode1, inode2)
>
> which indicates if the two inodes have the same ownership and:
>
> 	vfs_is_owned_by_me(inode)
>
> which compares the inode's ownership to current_fsuid() by default.

The use of two distinct inode operations make the most sense to me.  An 
alternative is to provide one inode operation which sets two boolean 
output parameters:

int (*check_ownership)(struct inode *const inode, struct inode *const 
parent,int *is_owned_by_me, int *is_owned_by_parent); where 
'is_owned_by_me' or 'is_owned_by_parent' might be NULL if the answer is 
not required. However, I prefer David's suggestion.

> The following places need to be considered for being changed:
>
>   (*) chown_ok()
>   (*) chgrp_ok()
>
>       Should call vfs_is_owned_by_me().  Possibly these need to defer all their
>       checks to the network filesystem as the interpretation of the target
>       UID/GID depends on the netfs.

Since the late 1980s, afs servers do not permit changes to owner or 
group on files unless the caller is a member of the 
system:administrators group.  The file system clients cannot make this 
determination themselves.  If Linux wishes to further restrict the 
operation to current owner, then use of a vfs_is_owned_by_me() like 
inode operation should be used.

Something to consider for future AFS3 or YFS protocol changes is to 
report the right to chown|chgrp to the client as part of a the 
FetchStatus result set.

>   (*) do_coredump()
>
>       Should probably call vfs_is_owned_by_me() to check that the file created
>       is owned by the caller - but the check that's there might be sufficient.
I agree.
>   (*) inode_owner_or_capable()
>
>       Should call vfs_is_owned_by_me().
I agree.
>       I'm not sure whether the namespace
>       mapping makes sense in such a case, but it probably could be used.
>
>   (*) vfs_setlease()
>
>       Should call vfs_is_owned_by_me().  Actually, it should query if leasing
>       is permitted.
>
>       Also, setting locks could perhaps do with a permission call to the
>       filesystem driver as AFS, for example, has a lock permission bit in the
>       ACL, but since the AFS server checks that when the RPC call is made, it's
>       probably unnecessary.

The AFS server will grant locks based upon the following rules:

  * the caller is granted the PRSFS_LOCK right (Shared lock only)
  * the caller is granted the PRSFS_WRITE right (Shared or Exclusive lock)
  * the caller is the file owner and is granted the PRSFS_INSERT right
    (Shared or Exclusive lock)

The client has enough information to implement a lock permission check 
if there was such an inode operation.

>   (*) acl_permission_check()
>   (*) posix_acl_permission()
>
>       UIDs are part of these ACLs, so no change required.  AFS implements its
>       own ACLs and evaluates them in ->permission() and on the server.
acl_permission_check() and posix_acl_permission() will not be called for 
AFS.  However, it it probably worth adding the vfs_is_owned_by_me() to 
acl_permission_check() in case there is another network filesystem which 
requires non-uid ownership checks and wants to use generic_permission().
>   (*) may_follow_link()
>
>       Should call vfs_is_owned_by_me() and also vfs_have_same_owner() on the
>       the link and its parent dir.  The latter only applies on world-writable
>       sticky dirs.
I agree
>   (*) may_create_in_sticky()
>
>       The initial subject of this patch.  Should call vfs_is_owned_by_me() and
>       also vfs_have_same_owner() both.
I agree.
>   (*) __check_sticky()
>
>       Should call vfs_is_owned_by_me() on both the dir and the inode.
I agree.
>   (*) may_dedupe_file()
>
>       Should call vfs_is_owned_by_me().
I agree.
>
>   (*) IMA policy ops.
>
>       No idea.

I am not familiar with the Integrity Measurement Operations. However, 
looking at the usage of the ima_rule_entry fowner_op and fgroup_op 
operations, I do not believe the proposed vfs_is_owned_by_me() could be 
used to implement fowner_op.  If IMA should work filesystems which 
cannot rely upon local uid comparisons for owner and group, then I think 
the IMA fowner_op and fgroup_op would require an alternative 
implementation.  At the moment, IMA is unlikely to work properly with AFS.


> David

Jeffrey Altman




Download attachment "smime.p7s" of type "application/pkcs7-signature" (4276 bytes)

Powered by blists - more mailing lists