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 <[email protected]> wrote: >On Oct 21, 2017, at 7:39 AM, Nicolas Belouin <[email protected]> >wrote: >> >> Fix an issue making trusted xattr world readable and other >> cap_sys_admin only >> >> Signed-off-by: Nicolas Belouin <[email protected]> >> --- >> 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
