linux-kernel - Re: [PATCH 2/2] Add SMB 2 support for getting and setting SACLs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 18 Dec 2020 13:40:01 -0500
From:   Boris Protopopov <boris.v.protopopov@...il.com>
To:     Shyam Prasad N <nspmangalore@...il.com>
Cc:     Boris Protopopov <pboris@...zon.com>,
        Steven French <sfrench@...ba.org>,
        CIFS <linux-cifs@...r.kernel.org>,
        samba-technical <samba-technical@...ts.samba.org>,
        LKML <linux-kernel@...r.kernel.org>, samjonas@...zon.com
Subject: Re: [PATCH 2/2] Add SMB 2 support for getting and setting SACLs

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