On Mon, Aug 1, 2011 at 4:19 PM, Steve French <smfrench@xxxxxxxxx> wrote:
> Mostly looks ok - but needs a few changes (see below).
>
> On Mon, Aug 1, 2011 at 3:50 PM, <shirishpargaonkar@xxxxxxxxx> wrote:
>> From: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
>>
>> Now build security descriptor to change either owner or group at the
>> server. Initially security descriptor was built to change only
>> ACL, that functionality has been extended.
>>
>> When either an Owner or Group of a file object at the server is changed,
>> rest of security descriptor remains same (DACL etc.).
>>
>> To set security descriptor, it is essential to open that file
>> with WRITE_DAC as well as WRITE_OWNER (Take Ownership) permission bits.
>> Function set_cifs_acl_by_fid() has been removed since we can't be
>> sure how a file was opened for writing, a valid request can fail
>> if the file was not opened with two above mentioned permissions.
>
> The set_cifs_acl_by_fid is still needed (e.g. don't break oplock on setacl
> due to reopening the file), but only for the setacl case can we
> reuse the handle (and why would find_readable_file have
> WRITE_DAC permission?), the setowner case requires
> the WRITE_DAC and WRITE_OWNER flags, so the set_cifs_acl
> function will have to distinguish the two cases.
>
I will change function set_cifs_acl to find a writable handle and
do basic testing wrt changing DACL. But even with that change,
we can't say even if a writable handle was found, the (writable)
file was opened with WRITE_DAC and/or WRITE_OWNER
access flags
> First we need to fix the case in existing code (if possible) where
> we have a file open and call set_cifs_acl - that looks like
> it is broken. Then put your patch on top and will need to
> make sure we don't request too much permission on the
> cifs open (only WRITE_DAC, unless changing owner when
> we also need WRITE_OWNER). We don't want to fail
> on the server on the open due to asking for too much.
>
>> It is the server that decides whether a set security descriptor with
>> either owner or group change succeeds or not.
>>
>>
>> Signed-off-by: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
>> ---
>> fs/cifs/cifsacl.c | 148 ++++++++++++++++++++++++++++-----------------------
>> fs/cifs/cifsproto.h | 7 ++-
>> fs/cifs/cifssmb.c | 4 +-
>> fs/cifs/inode.c | 35 ++++++++----
>> fs/cifs/xattr.c | 2 +-
>> 5 files changed, 111 insertions(+), 85 deletions(-)
>>
>> diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c
>> index b7e723a..88a801a 100644
>> --- a/fs/cifs/cifsacl.c
>> +++ b/fs/cifs/cifsacl.c
>> @@ -904,7 +904,7 @@ static void parse_dacl(struct cifs_acl *pdacl, char *end_of_acl,
>> acl_size = sizeof(struct cifs_acl);
>>
>> num_aces = le32_to_cpu(pdacl->num_aces);
>> - if (num_aces > 0) {
>> + if (num_aces > 0) {
>> umode_t user_mask = S_IRWXU;
>> umode_t group_mask = S_IRWXG;
>> umode_t other_mask = S_IRWXU | S_IRWXG | S_IRWXO;
>> @@ -1066,52 +1066,82 @@ static int parse_sec_desc(struct cifs_sb_info *cifs_sb,
>> else
>> cFYI(1, "no ACL"); /* BB grant all or default perms? */
>>
>> -/* cifscred->uid = owner_sid_ptr->rid;
>> - cifscred->gid = group_sid_ptr->rid;
>> - memcpy((void *)(&(cifscred->osid)), (void *)owner_sid_ptr,
>> - sizeof(struct cifs_sid));
>> - memcpy((void *)(&(cifscred->gsid)), (void *)group_sid_ptr,
>> - sizeof(struct cifs_sid)); */
>> -
>> return rc;
>> }
>>
>> -
>> /* Convert permission bits from mode to equivalent CIFS ACL */
>> static int build_sec_desc(struct cifs_ntsd *pntsd, struct cifs_ntsd *pnntsd,
>> - struct inode *inode, __u64 nmode)
>> + __u32 secdesclen, __u64 nmode, uid_t uid, gid_t gid, int *aclflag)
>> {
>> int rc = 0;
>> __u32 dacloffset;
>> __u32 ndacloffset;
>> __u32 sidsoffset;
>> struct cifs_sid *owner_sid_ptr, *group_sid_ptr;
>> + struct cifs_sid *nowner_sid_ptr, *ngroup_sid_ptr;
>> struct cifs_acl *dacl_ptr = NULL; /* no need for SACL ptr */
>> struct cifs_acl *ndacl_ptr = NULL; /* no need for SACL ptr */
>>
>> - if ((inode == NULL) || (pntsd == NULL) || (pnntsd == NULL))
>> - return -EIO;
>> -
>> - owner_sid_ptr = (struct cifs_sid *)((char *)pntsd +
>> + if (nmode != NO_CHANGE_64) { /* chmod */
>> + owner_sid_ptr = (struct cifs_sid *)((char *)pntsd +
>> le32_to_cpu(pntsd->osidoffset));
>> - group_sid_ptr = (struct cifs_sid *)((char *)pntsd +
>> + group_sid_ptr = (struct cifs_sid *)((char *)pntsd +
>> le32_to_cpu(pntsd->gsidoffset));
>> -
>> - dacloffset = le32_to_cpu(pntsd->dacloffset);
>> - dacl_ptr = (struct cifs_acl *)((char *)pntsd + dacloffset);
>> -
>> - ndacloffset = sizeof(struct cifs_ntsd);
>> - ndacl_ptr = (struct cifs_acl *)((char *)pnntsd + ndacloffset);
>> - ndacl_ptr->revision = dacl_ptr->revision;
>> - ndacl_ptr->size = 0;
>> - ndacl_ptr->num_aces = 0;
>> -
>> - rc = set_chmod_dacl(ndacl_ptr, owner_sid_ptr, group_sid_ptr, nmode);
>> -
>> - sidsoffset = ndacloffset + le16_to_cpu(ndacl_ptr->size);
>> -
>> - /* copy security descriptor control portion and owner and group sid */
>> - copy_sec_desc(pntsd, pnntsd, sidsoffset);
>> + dacloffset = le32_to_cpu(pntsd->dacloffset);
>> + dacl_ptr = (struct cifs_acl *)((char *)pntsd + dacloffset);
>> + ndacloffset = sizeof(struct cifs_ntsd);
>> + ndacl_ptr = (struct cifs_acl *)((char *)pnntsd + ndacloffset);
>> + ndacl_ptr->revision = dacl_ptr->revision;
>> + ndacl_ptr->size = 0;
>> + ndacl_ptr->num_aces = 0;
>> +
>> + rc = set_chmod_dacl(ndacl_ptr, owner_sid_ptr, group_sid_ptr,
>> + nmode);
>> + sidsoffset = ndacloffset + le16_to_cpu(ndacl_ptr->size);
>> + /* copy sec desc control portion & owner and group sids */
>> + copy_sec_desc(pntsd, pnntsd, sidsoffset);
>> + *aclflag = CIFS_ACL_DACL;
>> + } else {
>> + memcpy(pnntsd, pntsd, secdesclen);
>> + if (uid != NO_CHANGE_32) { /* chown */
>> + owner_sid_ptr = (struct cifs_sid *)((char *)pnntsd +
>> + le32_to_cpu(pnntsd->osidoffset));
>> + nowner_sid_ptr = kmalloc(sizeof(struct cifs_sid),
>> + GFP_KERNEL);
>> + if (!nowner_sid_ptr)
>> + return -ENOMEM;
>> + rc = id_to_sid(uid, SIDOWNER, nowner_sid_ptr);
>> + if (rc) {
>> + cFYI(1, "%s: Mapping error %d for owner id %d",
>> + __func__, rc, uid);
>> + kfree(nowner_sid_ptr);
>> + return rc;
>> + }
>> + memcpy(owner_sid_ptr, nowner_sid_ptr,
>> + sizeof(struct cifs_sid));
>> + kfree(nowner_sid_ptr);
>> + *aclflag = CIFS_ACL_OWNER;
>> + }
>> + if (gid != NO_CHANGE_32) { /* chgrp */
>> + group_sid_ptr = (struct cifs_sid *)((char *)pnntsd +
>> + le32_to_cpu(pnntsd->gsidoffset));
>> + ngroup_sid_ptr = kmalloc(sizeof(struct cifs_sid),
>> + GFP_KERNEL);
>> + if (!ngroup_sid_ptr)
>> + return -ENOMEM;
>> + rc = id_to_sid(gid, SIDGROUP, ngroup_sid_ptr);
>> + if (rc) {
>> + cFYI(1, "%s: Mapping error %d for group id %d",
>> + __func__, rc, gid);
>> + kfree(ngroup_sid_ptr);
>> + return rc;
>> + }
>> + memcpy(group_sid_ptr, ngroup_sid_ptr,
>> + sizeof(struct cifs_sid));
>> + kfree(ngroup_sid_ptr);
>> + *aclflag = CIFS_ACL_GROUP;
>> + }
>> + }
>>
>> return rc;
>> }
>> @@ -1189,26 +1219,8 @@ struct cifs_ntsd *get_cifs_acl(struct cifs_sb_info *cifs_sb,
>> return pntsd;
>> }
>>
>> -static int set_cifs_acl_by_fid(struct cifs_sb_info *cifs_sb, __u16 fid,
>> - struct cifs_ntsd *pnntsd, u32 acllen)
>> -{
>> - int xid, rc;
>> - struct tcon_link *tlink = cifs_sb_tlink(cifs_sb);
>> -
>> - if (IS_ERR(tlink))
>> - return PTR_ERR(tlink);
>> -
>> - xid = GetXid();
>> - rc = CIFSSMBSetCIFSACL(xid, tlink_tcon(tlink), fid, pnntsd, acllen);
>> - FreeXid(xid);
>> - cifs_put_tlink(tlink);
>> -
>> - cFYI(DBG2, "SetCIFSACL rc = %d", rc);
>> - return rc;
>> -}
>> -
>> static int set_cifs_acl_by_path(struct cifs_sb_info *cifs_sb, const char *path,
>> - struct cifs_ntsd *pnntsd, u32 acllen)
>> + struct cifs_ntsd *pnntsd, u32 acllen, int aclflag)
>> {
>> int oplock = 0;
>> int xid, rc;
>> @@ -1222,7 +1234,7 @@ static int set_cifs_acl_by_path(struct cifs_sb_info *cifs_sb, const char *path,
>> tcon = tlink_tcon(tlink);
>> xid = GetXid();
>>
>> - rc = CIFSSMBOpen(xid, tcon, path, FILE_OPEN, WRITE_DAC, 0,
>> + rc = CIFSSMBOpen(xid, tcon, path, FILE_OPEN, WRITE_DAC | WRITE_OWNER, 0,
>> &fid, &oplock, NULL, cifs_sb->local_nls,
>> cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR);
>> if (rc) {
>> @@ -1230,7 +1242,7 @@ static int set_cifs_acl_by_path(struct cifs_sb_info *cifs_sb, const char *path,
>> goto out;
>> }
>>
>> - rc = CIFSSMBSetCIFSACL(xid, tcon, fid, pnntsd, acllen);
>> + rc = CIFSSMBSetCIFSACL(xid, tcon, fid, pnntsd, acllen, aclflag);
>> cFYI(DBG2, "SetCIFSACL rc = %d", rc);
>>
>> CIFSSMBClose(xid, tcon, fid);
>> @@ -1242,21 +1254,18 @@ out:
>>
>> /* Set an ACL on the server */
>> int set_cifs_acl(struct cifs_ntsd *pnntsd, __u32 acllen,
>> - struct inode *inode, const char *path)
>> + struct inode *inode, const char *path, int aclflag)
>> {
>> struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
>> - struct cifsFileInfo *open_file;
>> - int rc;
>> -
>> - cFYI(DBG2, "set ACL for %s from mode 0x%x", path, inode->i_mode);
>> -
>> - open_file = find_readable_file(CIFS_I(inode), true);
>> - if (!open_file)
>> - return set_cifs_acl_by_path(cifs_sb, path, pnntsd, acllen);
>>
>> - rc = set_cifs_acl_by_fid(cifs_sb, open_file->netfid, pnntsd, acllen);
>> - cifsFileInfo_put(open_file);
>> - return rc;
>> + cFYI(DBG2, "%s: set ACL for %s", __func__, path);
>> + /*
>> + * Always open file by path. We do not know how a file was
>> + * opened if we search and find a writable find handle.
>> + * WRITRE_DAC and WRITE_OWNER permissions are essential for
>> + * changing ownership of a file at the server.
>> + */
>> + return set_cifs_acl_by_path(cifs_sb, path, pnntsd, acllen, aclflag);
>> }
>>
>> /* Translate the CIFS ACL (simlar to NTFS ACL) for a file into mode bits */
>> @@ -1290,9 +1299,12 @@ cifs_acl_to_fattr(struct cifs_sb_info *cifs_sb, struct cifs_fattr *fattr,
>> }
>>
>> /* Convert mode bits to an ACL so we can update the ACL on the server */
>> -int mode_to_cifs_acl(struct inode *inode, const char *path, __u64 nmode)
>> +int
>> +id_mode_to_cifs_acl(struct inode *inode, const char *path, __u64 nmode,
>> + uid_t uid, gid_t gid)
>> {
>> int rc = 0;
>> + int aclflag = CIFS_ACL_DACL; /* default flag to set */
>> __u32 secdesclen = 0;
>> struct cifs_ntsd *pntsd = NULL; /* acl obtained from server */
>> struct cifs_ntsd *pnntsd = NULL; /* modified acl to be sent to server */
>> @@ -1322,13 +1334,15 @@ int mode_to_cifs_acl(struct inode *inode, const char *path, __u64 nmode)
>> return -ENOMEM;
>> }
>>
>> - rc = build_sec_desc(pntsd, pnntsd, inode, nmode);
>> + rc = build_sec_desc(pntsd, pnntsd, secdesclen, nmode, uid, gid,
>> + &aclflag);
>>
>> cFYI(DBG2, "build_sec_desc rc: %d", rc);
>>
>> if (!rc) {
>> /* Set the security descriptor */
>> - rc = set_cifs_acl(pnntsd, secdesclen, inode, path);
>> + rc = set_cifs_acl(pnntsd, secdesclen, inode,
>> + path, aclflag);
>> cFYI(DBG2, "set_cifs_acl rc: %d", rc);
>> }
>>
>> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
>> index 953f844..3c0e026 100644
>> --- a/fs/cifs/cifsproto.h
>> +++ b/fs/cifs/cifsproto.h
>> @@ -145,11 +145,12 @@ extern int cifs_get_inode_info_unix(struct inode **pinode,
>> extern int cifs_acl_to_fattr(struct cifs_sb_info *cifs_sb,
>> struct cifs_fattr *fattr, struct inode *inode,
>> const char *path, const __u16 *pfid);
>> -extern int mode_to_cifs_acl(struct inode *inode, const char *path, __u64);
>> +extern int id_mode_to_cifs_acl(struct inode *inode, const char *path, __u64,
>> + uid_t, gid_t);
>> extern struct cifs_ntsd *get_cifs_acl(struct cifs_sb_info *, struct inode *,
>> const char *, u32 *);
>> extern int set_cifs_acl(struct cifs_ntsd *, __u32, struct inode *,
>> - const char *);
>> + const char *, int);
>>
>> extern void cifs_setup_cifs_sb(struct smb_vol *pvolume_info,
>> struct cifs_sb_info *cifs_sb);
>> @@ -419,7 +420,7 @@ extern int CIFSSMBSetEA(const int xid, struct cifs_tcon *tcon,
>> extern int CIFSSMBGetCIFSACL(const int xid, struct cifs_tcon *tcon,
>> __u16 fid, struct cifs_ntsd **acl_inf, __u32 *buflen);
>> extern int CIFSSMBSetCIFSACL(const int, struct cifs_tcon *, __u16,
>> - struct cifs_ntsd *, __u32);
>> + struct cifs_ntsd *, __u32, int);
>> extern int CIFSSMBGetPosixACL(const int xid, struct cifs_tcon *tcon,
>> const unsigned char *searchName,
>> char *acl_inf, const int buflen, const int acl_type,
>> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
>> index 1a9fe7f..f393bf1 100644
>> --- a/fs/cifs/cifssmb.c
>> +++ b/fs/cifs/cifssmb.c
>> @@ -3467,7 +3467,7 @@ qsec_out:
>>
>> int
>> CIFSSMBSetCIFSACL(const int xid, struct cifs_tcon *tcon, __u16 fid,
>> - struct cifs_ntsd *pntsd, __u32 acllen)
>> + struct cifs_ntsd *pntsd, __u32 acllen, int aclflag)
>> {
>> __u16 byte_count, param_count, data_count, param_offset, data_offset;
>> int rc = 0;
>> @@ -3504,7 +3504,7 @@ setCifsAclRetry:
>>
>> pSMB->Fid = fid; /* file handle always le */
>> pSMB->Reserved2 = 0;
>> - pSMB->AclFlags = cpu_to_le32(CIFS_ACL_DACL);
>> + pSMB->AclFlags = cpu_to_le32(aclflag);
>>
>> if (pntsd && acllen) {
>> memcpy((char *) &pSMBr->hdr.Protocol + data_offset,
>> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
>> index 9b018c8..7251f88 100644
>> --- a/fs/cifs/inode.c
>> +++ b/fs/cifs/inode.c
>> @@ -2106,6 +2106,8 @@ static int
>> cifs_setattr_nounix(struct dentry *direntry, struct iattr *attrs)
>> {
>> int xid;
>> + uid_t uid = NO_CHANGE_32;
>> + gid_t gid = NO_CHANGE_32;
>> struct inode *inode = direntry->d_inode;
>> struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
>> struct cifsInodeInfo *cifsInode = CIFS_I(inode);
>> @@ -2156,13 +2158,25 @@ cifs_setattr_nounix(struct dentry *direntry, struct iattr *attrs)
>> goto cifs_setattr_exit;
>> }
>>
>> - /*
>> - * Without unix extensions we can't send ownership changes to the
>> - * server, so silently ignore them. This is consistent with how
>> - * local DOS/Windows filesystems behave (VFAT, NTFS, etc). With
>> - * CIFSACL support + proper Windows to Unix idmapping, we may be
>> - * able to support this in the future.
>> - */
>> + if (attrs->ia_valid & ATTR_UID)
>> + uid = attrs->ia_uid;
>> +
>> + if (attrs->ia_valid & ATTR_GID)
>> + gid = attrs->ia_gid;
>> +
>> +#ifdef CONFIG_CIFS_ACL
>> + if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_CIFS_ACL) {
>> + if (uid != NO_CHANGE_32 || gid != NO_CHANGE_32) {
>> + rc = id_mode_to_cifs_acl(inode, full_path, NO_CHANGE_64,
>> + uid, gid);
>> + if (rc) {
>> + cFYI(1, "%s: Setting id failed with error: %d",
>> + __func__, rc);
>> + goto cifs_setattr_exit;
>> + }
>> + }
>> + } else
>> +#endif /* CONFIG_CIFS_ACL */
>> if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SET_UID))
>> attrs->ia_valid &= ~(ATTR_UID | ATTR_GID);
>>
>> @@ -2171,15 +2185,12 @@ cifs_setattr_nounix(struct dentry *direntry, struct iattr *attrs)
>> attrs->ia_valid &= ~ATTR_MODE;
>>
>> if (attrs->ia_valid & ATTR_MODE) {
>> - cFYI(1, "Mode changed to 0%o", attrs->ia_mode);
>> mode = attrs->ia_mode;
>> - }
>> -
>> - if (attrs->ia_valid & ATTR_MODE) {
>> rc = 0;
>> #ifdef CONFIG_CIFS_ACL
>> if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_CIFS_ACL) {
>> - rc = mode_to_cifs_acl(inode, full_path, mode);
>> + rc = id_mode_to_cifs_acl(inode, full_path, mode,
>> + NO_CHANGE_32, NO_CHANGE_32);
>> if (rc) {
>> cFYI(1, "%s: Setting ACL failed with error: %d",
>> __func__, rc);
>> diff --git a/fs/cifs/xattr.c b/fs/cifs/xattr.c
>> index 2a22fb2..fde15b1 100644
>> --- a/fs/cifs/xattr.c
>> +++ b/fs/cifs/xattr.c
>> @@ -178,7 +178,7 @@ int cifs_setxattr(struct dentry *direntry, const char *ea_name,
>> #ifdef CONFIG_CIFS_ACL
>> memcpy(pacl, ea_value, value_size);
>> rc = set_cifs_acl(pacl, value_size,
>> - direntry->d_inode, full_path);
>> + direntry->d_inode, full_path, CIFS_ACL_DACL);
>> if (rc == 0) /* force revalidate of the inode */
>> CIFS_I(direntry->d_inode)->time = 0;
>> kfree(pacl);
>> --
>> 1.6.0.2
>>
>>
>
>
>
> --
> Thanks,
>
> Steve
>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
