| Message ID | 20160602081333.GA25805@veci.piliscsaba.szeredi.hu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <vgoyal@redhat.com> > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> > 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 <vgoyal@redhat.com> 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)) { -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- 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)) {
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 <vgoyal@redhat.com> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> Fixes: b96809173e94 ("security_d_instantiate(): move to the point prior to attaching dentry to inode") --- fs/overlayfs/inode.c | 26 ++++++-------------------- 1 file changed, 6 insertions(+), 20 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html