| Message ID | 20201217205808.14756-1-pboris@amazon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for getting and setting SACLs | expand |
Minor comments inline... On Fri, Dec 18, 2020 at 2:30 AM Boris Protopopov <pboris@amazon.com> wrote: > > Fix passing of the additional security info via version > operations. Force new open when getting SACL and avoid > reuse of files that were previously open without > sufficient privileges to access SACLs. > > Signed-off-by: Boris Protopopov <pboris@amazon.com> > --- > > After further testing, I found that the security info was not being > passed correctly to opts->get_acl and opts->get_acl_by_fid(). Also, > it turned out that files open for read were being used to fetch > SACL without proper privileges. This patch fixes these issues, and > is meant to be squashed (comments dropped) with the earlier patch. > > fs/cifs/cifsacl.c | 10 +++++----- > fs/cifs/smb2ops.c | 4 ++-- > fs/cifs/smb2pdu.c | 4 +++- > fs/cifs/xattr.c | 10 ++++------ > 4 files changed, 14 insertions(+), 14 deletions(-) > > diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c > index 353394d9ada8..6baa121952ce 100644 > --- a/fs/cifs/cifsacl.c > +++ b/fs/cifs/cifsacl.c > @@ -1245,7 +1245,7 @@ cifs_acl_to_fattr(struct cifs_sb_info *cifs_sb, struct cifs_fattr *fattr, > int rc = 0; > struct tcon_link *tlink = cifs_sb_tlink(cifs_sb); > struct smb_version_operations *ops; > - const u32 unused = 0; > + const u32 info = 0; > > cifs_dbg(NOISY, "converting ACL to mode for %s\n", path); > > @@ -1255,9 +1255,9 @@ cifs_acl_to_fattr(struct cifs_sb_info *cifs_sb, struct cifs_fattr *fattr, > ops = tlink_tcon(tlink)->ses->server->ops; > > if (pfid && (ops->get_acl_by_fid)) > - pntsd = ops->get_acl_by_fid(cifs_sb, pfid, &acllen, unused); > + pntsd = ops->get_acl_by_fid(cifs_sb, pfid, &acllen, info); > else if (ops->get_acl) > - pntsd = ops->get_acl(cifs_sb, inode, path, &acllen, unused); > + pntsd = ops->get_acl(cifs_sb, inode, path, &acllen, info); > else { > cifs_put_tlink(tlink); > return -EOPNOTSUPP; > @@ -1295,7 +1295,7 @@ id_mode_to_cifs_acl(struct inode *inode, const char *path, __u64 nmode, > struct tcon_link *tlink = cifs_sb_tlink(cifs_sb); > struct smb_version_operations *ops; > bool mode_from_sid, id_from_sid; > - const u32 unused = 0; > + const u32 info = 0; > > if (IS_ERR(tlink)) > return PTR_ERR(tlink); > @@ -1311,7 +1311,7 @@ id_mode_to_cifs_acl(struct inode *inode, const char *path, __u64 nmode, > return -EOPNOTSUPP; > } > > - pntsd = ops->get_acl(cifs_sb, inode, path, &secdesclen, unused); > + pntsd = ops->get_acl(cifs_sb, inode, path, &secdesclen, info); > if (IS_ERR(pntsd)) { > rc = PTR_ERR(pntsd); > cifs_dbg(VFS, "%s: error %d getting sec desc\n", __func__, rc); > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c > index d28a29728fb1..f5e198860c16 100644 > --- a/fs/cifs/smb2ops.c > +++ b/fs/cifs/smb2ops.c > @@ -3315,9 +3315,9 @@ get_smb2_acl(struct cifs_sb_info *cifs_sb, > struct cifs_ntsd *pntsd = NULL; > struct cifsFileInfo *open_file = NULL; > > - if (inode) > + if (inode && !(info & SACL_SECINFO)) > open_file = find_readable_file(CIFS_I(inode), true); > - if (!open_file) > + if (!open_file || (info & SACL_SECINFO)) > return get_smb2_acl_by_path(cifs_sb, path, pacllen, info); > > pntsd = get_smb2_acl_by_fid(cifs_sb, &open_file->fid, pacllen, info); Why not have an if (info & SACL_SECINFO) return get_smb2_acl_by_path... right at the beginning? Looks cleaner that way IMO. > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c > index 0aeb63694306..b207e1eb6803 100644 > --- a/fs/cifs/smb2pdu.c > +++ b/fs/cifs/smb2pdu.c > @@ -3472,8 +3472,10 @@ SMB311_posix_query_info(const unsigned int xid, struct cifs_tcon *tcon, > int > SMB2_query_acl(const unsigned int xid, struct cifs_tcon *tcon, > u64 persistent_fid, u64 volatile_fid, > - void **data, u32 *plen, u32 additional_info) > + void **data, u32 *plen, u32 extra_info) > { > + __u32 additional_info = OWNER_SECINFO | GROUP_SECINFO | DACL_SECINFO | > + extra_info; I still prefer having these flags set by the caller. That way, the caller has the flexibility to query only the subset needed. > *plen = 0; > > return query_info(xid, tcon, persistent_fid, volatile_fid, > diff --git a/fs/cifs/xattr.c b/fs/cifs/xattr.c > index 9318a2acf4ee..6b658a1172ef 100644 > --- a/fs/cifs/xattr.c > +++ b/fs/cifs/xattr.c > @@ -340,21 +340,19 @@ static int cifs_xattr_get(const struct xattr_handler *handler, > * fetch owner, DACL, and SACL if asked for full descriptor, > * fetch owner and DACL otherwise > */ > - u32 acllen, additional_info = 0; > + u32 acllen, extra_info; > struct cifs_ntsd *pacl; > > if (pTcon->ses->server->ops->get_acl == NULL) > goto out; /* rc already EOPNOTSUPP */ > > if (handler->flags == XATTR_CIFS_NTSD_FULL) { > - additional_info = OWNER_SECINFO | GROUP_SECINFO | > - DACL_SECINFO | SACL_SECINFO; > + extra_info = SACL_SECINFO; > } else { > - additional_info = OWNER_SECINFO | GROUP_SECINFO | > - DACL_SECINFO; > + extra_info = 0; > } > pacl = pTcon->ses->server->ops->get_acl(cifs_sb, > - inode, full_path, &acllen, additional_info); > + inode, full_path, &acllen, extra_info); > if (IS_ERR(pacl)) { > rc = PTR_ERR(pacl); > cifs_dbg(VFS, "%s: error %zd getting sec desc\n", > -- > 2.18.4 >
No objections on my part as far as adding 'if (info & SACL_SECINFO) return'.
I had originally those flags sent from the caller, but that was
confusing at the top level (e.g. in cifsacl.c), so I have opted to
passing only "extra" flags ("additional" was already taken).
-------------
...
> > --- a/fs/cifs/smb2ops.c
> > +++ b/fs/cifs/smb2ops.c
> > @@ -3315,9 +3315,9 @@ get_smb2_acl(struct cifs_sb_info *cifs_sb,
> > struct cifs_ntsd *pntsd = NULL;
> > struct cifsFileInfo *open_file = NULL;
> >
> > - if (inode)
> > + if (inode && !(info & SACL_SECINFO))
> > open_file = find_readable_file(CIFS_I(inode), true);
> > - if (!open_file)
> > + if (!open_file || (info & SACL_SECINFO))
> > return get_smb2_acl_by_path(cifs_sb, path, pacllen, info);
> >
> > pntsd = get_smb2_acl_by_fid(cifs_sb, &open_file->fid, pacllen, info);
> Why not have an if (info & SACL_SECINFO) return
> get_smb2_acl_by_path... right at the beginning?
> Looks cleaner that way IMO.
>
...
> > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> > index 0aeb63694306..b207e1eb6803 100644
> > --- a/fs/cifs/smb2pdu.c
> > +++ b/fs/cifs/smb2pdu.c
> > @@ -3472,8 +3472,10 @@ SMB311_posix_query_info(const unsigned int xid, struct cifs_tcon *tcon,
> > int
> > SMB2_query_acl(const unsigned int xid, struct cifs_tcon *tcon,
> > u64 persistent_fid, u64 volatile_fid,
> > - void **data, u32 *plen, u32 additional_info)
> > + void **data, u32 *plen, u32 extra_info)
> > {
> > + __u32 additional_info = OWNER_SECINFO | GROUP_SECINFO | DACL_SECINFO |
> > + extra_info;
> I still prefer having these flags set by the caller. That way, the
> caller has the flexibility to query only the subset needed.
> --
> -Shyam
Lightly updated the patch to shrink patch2 by naming a variable the same in patch 1, to avoid a rename of a local variable in patch2. And fixed one trivial checkpatch warning, and a trivial rebase issue. Tentatively merged into cifs-2.6.git for-next pending more review(s) and also the usual regression testing. On Thu, Dec 17, 2020 at 3:00 PM Boris Protopopov <pboris@amazon.com> wrote: > > Fix passing of the additional security info via version > operations. Force new open when getting SACL and avoid > reuse of files that were previously open without > sufficient privileges to access SACLs. > > Signed-off-by: Boris Protopopov <pboris@amazon.com> > --- > > After further testing, I found that the security info was not being > passed correctly to opts->get_acl and opts->get_acl_by_fid(). Also, > it turned out that files open for read were being used to fetch > SACL without proper privileges. This patch fixes these issues, and > is meant to be squashed (comments dropped) with the earlier patch. > > fs/cifs/cifsacl.c | 10 +++++----- > fs/cifs/smb2ops.c | 4 ++-- > fs/cifs/smb2pdu.c | 4 +++- > fs/cifs/xattr.c | 10 ++++------ > 4 files changed, 14 insertions(+), 14 deletions(-) > > diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c > index 353394d9ada8..6baa121952ce 100644 > --- a/fs/cifs/cifsacl.c > +++ b/fs/cifs/cifsacl.c > @@ -1245,7 +1245,7 @@ cifs_acl_to_fattr(struct cifs_sb_info *cifs_sb, struct cifs_fattr *fattr, > int rc = 0; > struct tcon_link *tlink = cifs_sb_tlink(cifs_sb); > struct smb_version_operations *ops; > - const u32 unused = 0; > + const u32 info = 0; > > cifs_dbg(NOISY, "converting ACL to mode for %s\n", path); > > @@ -1255,9 +1255,9 @@ cifs_acl_to_fattr(struct cifs_sb_info *cifs_sb, struct cifs_fattr *fattr, > ops = tlink_tcon(tlink)->ses->server->ops; > > if (pfid && (ops->get_acl_by_fid)) > - pntsd = ops->get_acl_by_fid(cifs_sb, pfid, &acllen, unused); > + pntsd = ops->get_acl_by_fid(cifs_sb, pfid, &acllen, info); > else if (ops->get_acl) > - pntsd = ops->get_acl(cifs_sb, inode, path, &acllen, unused); > + pntsd = ops->get_acl(cifs_sb, inode, path, &acllen, info); > else { > cifs_put_tlink(tlink); > return -EOPNOTSUPP; > @@ -1295,7 +1295,7 @@ id_mode_to_cifs_acl(struct inode *inode, const char *path, __u64 nmode, > struct tcon_link *tlink = cifs_sb_tlink(cifs_sb); > struct smb_version_operations *ops; > bool mode_from_sid, id_from_sid; > - const u32 unused = 0; > + const u32 info = 0; > > if (IS_ERR(tlink)) > return PTR_ERR(tlink); > @@ -1311,7 +1311,7 @@ id_mode_to_cifs_acl(struct inode *inode, const char *path, __u64 nmode, > return -EOPNOTSUPP; > } > > - pntsd = ops->get_acl(cifs_sb, inode, path, &secdesclen, unused); > + pntsd = ops->get_acl(cifs_sb, inode, path, &secdesclen, info); > if (IS_ERR(pntsd)) { > rc = PTR_ERR(pntsd); > cifs_dbg(VFS, "%s: error %d getting sec desc\n", __func__, rc); > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c > index d28a29728fb1..f5e198860c16 100644 > --- a/fs/cifs/smb2ops.c > +++ b/fs/cifs/smb2ops.c > @@ -3315,9 +3315,9 @@ get_smb2_acl(struct cifs_sb_info *cifs_sb, > struct cifs_ntsd *pntsd = NULL; > struct cifsFileInfo *open_file = NULL; > > - if (inode) > + if (inode && !(info & SACL_SECINFO)) > open_file = find_readable_file(CIFS_I(inode), true); > - if (!open_file) > + if (!open_file || (info & SACL_SECINFO)) > return get_smb2_acl_by_path(cifs_sb, path, pacllen, info); > > pntsd = get_smb2_acl_by_fid(cifs_sb, &open_file->fid, pacllen, info); > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c > index 0aeb63694306..b207e1eb6803 100644 > --- a/fs/cifs/smb2pdu.c > +++ b/fs/cifs/smb2pdu.c > @@ -3472,8 +3472,10 @@ SMB311_posix_query_info(const unsigned int xid, struct cifs_tcon *tcon, > int > SMB2_query_acl(const unsigned int xid, struct cifs_tcon *tcon, > u64 persistent_fid, u64 volatile_fid, > - void **data, u32 *plen, u32 additional_info) > + void **data, u32 *plen, u32 extra_info) > { > + __u32 additional_info = OWNER_SECINFO | GROUP_SECINFO | DACL_SECINFO | > + extra_info; > *plen = 0; > > return query_info(xid, tcon, persistent_fid, volatile_fid, > diff --git a/fs/cifs/xattr.c b/fs/cifs/xattr.c > index 9318a2acf4ee..6b658a1172ef 100644 > --- a/fs/cifs/xattr.c > +++ b/fs/cifs/xattr.c > @@ -340,21 +340,19 @@ static int cifs_xattr_get(const struct xattr_handler *handler, > * fetch owner, DACL, and SACL if asked for full descriptor, > * fetch owner and DACL otherwise > */ > - u32 acllen, additional_info = 0; > + u32 acllen, extra_info; > struct cifs_ntsd *pacl; > > if (pTcon->ses->server->ops->get_acl == NULL) > goto out; /* rc already EOPNOTSUPP */ > > if (handler->flags == XATTR_CIFS_NTSD_FULL) { > - additional_info = OWNER_SECINFO | GROUP_SECINFO | > - DACL_SECINFO | SACL_SECINFO; > + extra_info = SACL_SECINFO; > } else { > - additional_info = OWNER_SECINFO | GROUP_SECINFO | > - DACL_SECINFO; > + extra_info = 0; > } > pacl = pTcon->ses->server->ops->get_acl(cifs_sb, > - inode, full_path, &acllen, additional_info); > + inode, full_path, &acllen, extra_info); > if (IS_ERR(pacl)) { > rc = PTR_ERR(pacl); > cifs_dbg(VFS, "%s: error %zd getting sec desc\n", > -- > 2.18.4 >
diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c index 353394d9ada8..6baa121952ce 100644 --- a/fs/cifs/cifsacl.c +++ b/fs/cifs/cifsacl.c @@ -1245,7 +1245,7 @@ cifs_acl_to_fattr(struct cifs_sb_info *cifs_sb, struct cifs_fattr *fattr, int rc = 0; struct tcon_link *tlink = cifs_sb_tlink(cifs_sb); struct smb_version_operations *ops; - const u32 unused = 0; + const u32 info = 0; cifs_dbg(NOISY, "converting ACL to mode for %s\n", path); @@ -1255,9 +1255,9 @@ cifs_acl_to_fattr(struct cifs_sb_info *cifs_sb, struct cifs_fattr *fattr, ops = tlink_tcon(tlink)->ses->server->ops; if (pfid && (ops->get_acl_by_fid)) - pntsd = ops->get_acl_by_fid(cifs_sb, pfid, &acllen, unused); + pntsd = ops->get_acl_by_fid(cifs_sb, pfid, &acllen, info); else if (ops->get_acl) - pntsd = ops->get_acl(cifs_sb, inode, path, &acllen, unused); + pntsd = ops->get_acl(cifs_sb, inode, path, &acllen, info); else { cifs_put_tlink(tlink); return -EOPNOTSUPP; @@ -1295,7 +1295,7 @@ id_mode_to_cifs_acl(struct inode *inode, const char *path, __u64 nmode, struct tcon_link *tlink = cifs_sb_tlink(cifs_sb); struct smb_version_operations *ops; bool mode_from_sid, id_from_sid; - const u32 unused = 0; + const u32 info = 0; if (IS_ERR(tlink)) return PTR_ERR(tlink); @@ -1311,7 +1311,7 @@ id_mode_to_cifs_acl(struct inode *inode, const char *path, __u64 nmode, return -EOPNOTSUPP; } - pntsd = ops->get_acl(cifs_sb, inode, path, &secdesclen, unused); + pntsd = ops->get_acl(cifs_sb, inode, path, &secdesclen, info); if (IS_ERR(pntsd)) { rc = PTR_ERR(pntsd); cifs_dbg(VFS, "%s: error %d getting sec desc\n", __func__, rc); diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c index d28a29728fb1..f5e198860c16 100644 --- a/fs/cifs/smb2ops.c +++ b/fs/cifs/smb2ops.c @@ -3315,9 +3315,9 @@ get_smb2_acl(struct cifs_sb_info *cifs_sb, struct cifs_ntsd *pntsd = NULL; struct cifsFileInfo *open_file = NULL; - if (inode) + if (inode && !(info & SACL_SECINFO)) open_file = find_readable_file(CIFS_I(inode), true); - if (!open_file) + if (!open_file || (info & SACL_SECINFO)) return get_smb2_acl_by_path(cifs_sb, path, pacllen, info); pntsd = get_smb2_acl_by_fid(cifs_sb, &open_file->fid, pacllen, info); diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index 0aeb63694306..b207e1eb6803 100644 --- a/fs/cifs/smb2pdu.c +++ b/fs/cifs/smb2pdu.c @@ -3472,8 +3472,10 @@ SMB311_posix_query_info(const unsigned int xid, struct cifs_tcon *tcon, int SMB2_query_acl(const unsigned int xid, struct cifs_tcon *tcon, u64 persistent_fid, u64 volatile_fid, - void **data, u32 *plen, u32 additional_info) + void **data, u32 *plen, u32 extra_info) { + __u32 additional_info = OWNER_SECINFO | GROUP_SECINFO | DACL_SECINFO | + extra_info; *plen = 0; return query_info(xid, tcon, persistent_fid, volatile_fid, diff --git a/fs/cifs/xattr.c b/fs/cifs/xattr.c index 9318a2acf4ee..6b658a1172ef 100644 --- a/fs/cifs/xattr.c +++ b/fs/cifs/xattr.c @@ -340,21 +340,19 @@ static int cifs_xattr_get(const struct xattr_handler *handler, * fetch owner, DACL, and SACL if asked for full descriptor, * fetch owner and DACL otherwise */ - u32 acllen, additional_info = 0; + u32 acllen, extra_info; struct cifs_ntsd *pacl; if (pTcon->ses->server->ops->get_acl == NULL) goto out; /* rc already EOPNOTSUPP */ if (handler->flags == XATTR_CIFS_NTSD_FULL) { - additional_info = OWNER_SECINFO | GROUP_SECINFO | - DACL_SECINFO | SACL_SECINFO; + extra_info = SACL_SECINFO; } else { - additional_info = OWNER_SECINFO | GROUP_SECINFO | - DACL_SECINFO; + extra_info = 0; } pacl = pTcon->ses->server->ops->get_acl(cifs_sb, - inode, full_path, &acllen, additional_info); + inode, full_path, &acllen, extra_info); if (IS_ERR(pacl)) { rc = PTR_ERR(pacl); cifs_dbg(VFS, "%s: error %zd getting sec desc\n",
Fix passing of the additional security info via version operations. Force new open when getting SACL and avoid reuse of files that were previously open without sufficient privileges to access SACLs. Signed-off-by: Boris Protopopov <pboris@amazon.com> --- After further testing, I found that the security info was not being passed correctly to opts->get_acl and opts->get_acl_by_fid(). Also, it turned out that files open for read were being used to fetch SACL without proper privileges. This patch fixes these issues, and is meant to be squashed (comments dropped) with the earlier patch. fs/cifs/cifsacl.c | 10 +++++----- fs/cifs/smb2ops.c | 4 ++-- fs/cifs/smb2pdu.c | 4 +++- fs/cifs/xattr.c | 10 ++++------ 4 files changed, 14 insertions(+), 14 deletions(-)