Re: [PATCH] fs: fix xattr permission checking error

[Nicolas Belouin]

Indeed, I was mistaken here.
Thanks for the review.
I will do the "strncmp() ==0" in another patch as it is particularly unclear in this context.

On October 21, 2017 9:48:16 PM GMT+02:00, Andreas Dilger <adilger@xxxxxxxxx> wrote:
>On Oct 21, 2017, at 7:39 AM, Nicolas Belouin <nicolas@xxxxxxxxxx>
>wrote:
>> 
>> Fix an issue making trusted xattr world readable and other
>> cap_sys_admin only
>> 
>> Signed-off-by: Nicolas Belouin <nicolas@xxxxxxxxxx>
>> ---
>> fs/hfsplus/xattr.c | 2 +-
>> fs/jfs/xattr.c     | 5 ++---
>> 2 files changed, 3 insertions(+), 4 deletions(-)
>> 
>> diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
>> index d37bb88dc746..ae03a19196ef 100644
>> --- a/fs/hfsplus/xattr.c
>> +++ b/fs/hfsplus/xattr.c
>> @@ -604,7 +604,7 @@ static inline int can_list(const char
>*xattr_name)
>> 	if (!xattr_name)
>> 		return 0;
>> 
>> -	return strncmp(xattr_name, XATTR_TRUSTED_PREFIX,
>> +	return !strncmp(xattr_name, XATTR_TRUSTED_PREFIX,
>> 			XATTR_TRUSTED_PREFIX_LEN) ||
>> 				capable(CAP_SYS_ADMIN);
>
>I don't think this is correct.  This means "you can list the xattr if
>it IS 'trusted.*', OR if you have sysadmin privilege", so non-trusted
>xattrs could not be listed by regular users.
>
>As can be seen by this defect, the use of "strncmp()" with an explicit
>boolean return code is confusing and subject to errors, in particular
>"strncmp()" returning 0 (false) means the strings MATCH.  My preference
>is to explicitly check "strncmp() == 0" for the match, as this is more
>clear to the reader that strncmp() has a non-standard return
>convention.
>
>To my reading, the original logic is correct, which is "you can list
>the xattr if it is not 'trusted.*' OR if you have sysadmin privilege",
>but it could be improved like:
>
>	return strncmp(xattr_name, XATTR_TRUSTED_PREFIX,
>		       XATTR_TRUSTED_PREFIX_LEN) != 0 ||
>		capable(CAP_SYS_ADMIN);
>
>> diff --git a/fs/jfs/xattr.c b/fs/jfs/xattr.c
>> index c60f3d32ee91..1c46573a96ed 100644
>> --- a/fs/jfs/xattr.c
>> +++ b/fs/jfs/xattr.c
>> @@ -858,9 +858,8 @@ ssize_t __jfs_getxattr(struct inode *inode, const
>char *name, void *data,
>>  */
>> static inline int can_list(struct jfs_ea *ea)
>> {
>> -	return (strncmp(ea->name, XATTR_TRUSTED_PREFIX,
>> -			    XATTR_TRUSTED_PREFIX_LEN) ||
>> -		capable(CAP_SYS_ADMIN));
>> +	return (!strncmp(ea->name, XATTR_TRUSTED_PREFIX,
>> +			 XATTR_TRUSTED_PREFIX_LEN) || capable(CAP_SYS_ADMIN));
>> }
>
>I think the original code is also correct here, and your patch is
>adding
>a bug.
>
>Cheers, Andreas

Nicolas