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
