From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932290AbdJUUAn convert rfc822-to-8bit (ORCPT ); Sat, 21 Oct 2017 16:00:43 -0400 Received: from smtp-sh2.infomaniak.ch ([128.65.195.6]:43334 "EHLO smtp-sh2.infomaniak.ch" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932226AbdJUUAk (ORCPT ); Sat, 21 Oct 2017 16:00:40 -0400 Date: Sat, 21 Oct 2017 22:00:25 +0200 User-Agent: K-9 Mail for Android In-Reply-To: <94E1EFF2-281B-4ACD-98FF-D194F4F35EDB@dilger.ca> References: <20171021133947.19935-1-nicolas@belouin.fr> <94E1EFF2-281B-4ACD-98FF-D194F4F35EDB@dilger.ca> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT Subject: Re: [PATCH] fs: fix xattr permission checking error To: Andreas Dilger CC: Dave Kleikamp , linux-fsdevel , Linux Kernel Mailing List , jfs-discussion@lists.sourceforge.net From: Nicolas Belouin Message-ID: <3D009066-D662-4F7A-A3AA-B57A1A6AA522@belouin.fr> X-Antivirus: Dr.Web (R) for Unix mail servers drweb plugin ver.6.0.2.8 X-Antivirus-Code: 0x100000 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 wrote: >On Oct 21, 2017, at 7:39 AM, Nicolas Belouin >wrote: >> >> Fix an issue making trusted xattr world readable and other >> cap_sys_admin only >> >> Signed-off-by: Nicolas Belouin >> --- >> 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