From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1161022AbcFBMul (ORCPT ); Thu, 2 Jun 2016 08:50:41 -0400 Received: from mx1.redhat.com ([209.132.183.28]:47097 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751338AbcFBMuk (ORCPT ); Thu, 2 Jun 2016 08:50:40 -0400 Date: Thu, 2 Jun 2016 08:50:38 -0400 From: Vivek Goyal To: Miklos Szeredi Cc: Al Viro , linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] ovl: xattr filter fix Message-ID: <20160602125038.GA31383@redhat.com> References: <20160602081333.GA25805@veci.piliscsaba.szeredi.hu> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160602081333.GA25805@veci.piliscsaba.szeredi.hu> User-Agent: Mutt/1.6.1 (2016-04-27) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Thu, 02 Jun 2016 12:50:39 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Jun 02, 2016 at 10:13:33AM +0200, Miklos Szeredi wrote: > a) ovl_need_xattr_filter() is wrong, we can have multiple lower layers > overlaid, all of which (except the lowest one) honouring the > "trusted.overlay.opaque" xattr. So need to filter everything except the > bottom and the pure-upper layer. > > b) we no longer can assume that inode is attached to dentry in > get/setxattr. > > This patch unconditionally filters private xattrs to fix both of the above. > Performance impact for get/removexattrs is likely in the noise. > > For listxattrs it might be measurable in pathological cases, but I very > much hope nobody cares. If they do, we'll fix it then. > > Reported-by: Vivek Goyal > Signed-off-by: Miklos Szeredi > Fixes: b96809173e94 ("security_d_instantiate(): move to the point prior to attaching dentry to inode") Hi Miklos, This patch fixes the issue for me. Thanks. Reviewed-by: Vivek Goyal Vivek > --- > fs/overlayfs/inode.c | 26 ++++++-------------------- > 1 file changed, 6 insertions(+), 20 deletions(-) > > --- a/fs/overlayfs/inode.c > +++ b/fs/overlayfs/inode.c > @@ -238,41 +238,27 @@ int ovl_setxattr(struct dentry *dentry, > return err; > } > > -static bool ovl_need_xattr_filter(struct dentry *dentry, > - enum ovl_path_type type) > -{ > - if ((type & (__OVL_PATH_PURE | __OVL_PATH_UPPER)) == __OVL_PATH_UPPER) > - return S_ISDIR(dentry->d_inode->i_mode); > - else > - return false; > -} > - > ssize_t ovl_getxattr(struct dentry *dentry, struct inode *inode, > const char *name, void *value, size_t size) > { > - struct path realpath; > - enum ovl_path_type type = ovl_path_real(dentry, &realpath); > + struct dentry *realdentry = ovl_dentry_real(dentry); > > - if (ovl_need_xattr_filter(dentry, type) && ovl_is_private_xattr(name)) > + if (ovl_is_private_xattr(name)) > return -ENODATA; > > - return vfs_getxattr(realpath.dentry, name, value, size); > + return vfs_getxattr(realdentry, name, value, size); > } > > ssize_t ovl_listxattr(struct dentry *dentry, char *list, size_t size) > { > - struct path realpath; > - enum ovl_path_type type = ovl_path_real(dentry, &realpath); > + struct dentry *realdentry = ovl_dentry_real(dentry); > ssize_t res; > int off; > > - res = vfs_listxattr(realpath.dentry, list, size); > + res = vfs_listxattr(realdentry, list, size); > if (res <= 0 || size == 0) > return res; > > - if (!ovl_need_xattr_filter(dentry, type)) > - return res; > - > /* filter out private xattrs */ > for (off = 0; off < res;) { > char *s = list + off; > @@ -302,7 +288,7 @@ int ovl_removexattr(struct dentry *dentr > goto out; > > err = -ENODATA; > - if (ovl_need_xattr_filter(dentry, type) && ovl_is_private_xattr(name)) > + if (ovl_is_private_xattr(name)) > goto out_drop_write; > > if (!OVL_TYPE_UPPER(type)) {