* [RFC PATCH 1/6] ksmbd: don't ignore the owner and group of a descriptor without a DACL
@ 2026-11-10 9:02 Jan Koester
2026-11-11 16:00 ` Ian Bridges
0 siblings, 1 reply; 3+ messages in thread
From: Jan Koester @ 2026-11-10 9:02 UTC (permalink / raw)
To: linkinjeon, smfrench
Cc: senozhatsky, tom, chenxiaosong, brauner, linux-cifs,
linux-fsdevel, samba-technical, linux-kernel
parse_sec_desc() returned before looking at the owner and group SIDs
when the descriptor had no DACL_PRESENT, so an SMB2 SET_INFO with only
OWNER_SECURITY_INFORMATION or GROUP_SECURITY_INFORMATION -- what Windows
sends to take ownership, and smbcacls -C/-G -- did nothing and still
reported success.
Map the owner and group in any case and parse the DACL only if there is
one. set_info_sec() then changes the mode and the POSIX ACLs only for a
descriptor with a DACL, and leaves the stored descriptor alone without
one: replacing it would drop its DACL, and the owner shown comes from
the inode anyway. This also covers the SD buffer create context, so its
type_check argument goes away.
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Jan Koester <jan.koester@tuxist.de>
---
fs/smb/server/smb2pdu.c | 4 ++--
fs/smb/server/smbacl.c | 39 ++++++++++++++++++++++++++-------------
fs/smb/server/smbacl.h | 2 +-
3 files changed, 29 insertions(+), 16 deletions(-)
diff --git a/fs/smb/server/smb2pdu.c b/fs/smb/server/smb2pdu.c
index 230bea45de..8e47d725c1 100644
--- a/fs/smb/server/smb2pdu.c
+++ b/fs/smb/server/smb2pdu.c
@@ -3661,7 +3661,7 @@ static int smb2_create_sd_buffer(struct ksmbd_work *work,
sizeof(struct create_sd_buf_req))
return -EINVAL;
return set_info_sec(work->conn, work->tcon, path, &sd_buf->ntsd,
- le32_to_cpu(sd_buf->ccontext.DataLength), true, false);
+ le32_to_cpu(sd_buf->ccontext.DataLength), false);
}
static void ksmbd_acls_fattr(struct smb_fattr *fattr,
@@ -8535,7 +8535,7 @@ static int smb2_set_info_sec(struct ksmbd_file *fp, int addition_info,
return -EACCES;
return set_info_sec(fp->conn, fp->tcon, &fp->filp->f_path, pntsd,
- buf_len, false, true);
+ buf_len, true);
}
/**
diff --git a/fs/smb/server/smbacl.c b/fs/smb/server/smbacl.c
index 126264cc07..d1cf63a9fe 100644
--- a/fs/smb/server/smbacl.c
+++ b/fs/smb/server/smbacl.c
@@ -1017,12 +1017,14 @@ int parse_sec_desc(struct mnt_idmap *idmap, struct smb_ntsd *pntsd,
le32_to_cpu(pntsd->sacloffset), dacloffset);
pntsd_type = le16_to_cpu(pntsd->type);
- if (!(pntsd_type & DACL_PRESENT)) {
+ /*
+ * Without a DACL only the owner and group change (Windows sends that
+ * to take ownership): don't return before them.
+ */
+ if (pntsd_type & DACL_PRESENT)
+ pntsd->type = cpu_to_le16(DACL_PRESENT);
+ else
ksmbd_debug(SMB, "DACL_PRESENT in DACL type is not set\n");
- return rc;
- }
-
- pntsd->type = cpu_to_le16(DACL_PRESENT);
if (pntsd->osidoffset) {
if (le32_to_cpu(pntsd->osidoffset) < sizeof(struct smb_ntsd))
@@ -1060,6 +1062,9 @@ int parse_sec_desc(struct mnt_idmap *idmap, struct smb_ntsd *pntsd,
}
}
+ if (!(pntsd_type & DACL_PRESENT))
+ return 0;
+
if ((pntsd_type & (DACL_AUTO_INHERITED | DACL_AUTO_INHERIT_REQ)) ==
(DACL_AUTO_INHERITED | DACL_AUTO_INHERIT_REQ))
pntsd->type |= cpu_to_le16(DACL_AUTO_INHERITED);
@@ -1768,18 +1773,21 @@ int smb_check_perm_dacl(struct ksmbd_conn *conn, const struct path *path,
int set_info_sec(struct ksmbd_conn *conn, struct ksmbd_tree_connect *tcon,
const struct path *path, struct smb_ntsd *pntsd, int ntsd_len,
- bool type_check, bool get_write)
+ bool get_write)
{
int rc;
struct smb_fattr fattr = {{0}};
struct inode *inode = d_inode(path->dentry);
struct mnt_idmap *idmap = mnt_idmap(path->mnt);
struct iattr newattrs;
+ bool dacl;
fattr.cf_uid = INVALID_UID;
fattr.cf_gid = INVALID_GID;
fattr.cf_mode = inode->i_mode;
+ /* parse_sec_desc() rewrites type */
+ dacl = le16_to_cpu(pntsd->type) & DACL_PRESENT;
rc = parse_sec_desc(idmap, pntsd, ntsd_len, &fattr);
if (rc)
goto out;
@@ -1793,12 +1801,14 @@ int set_info_sec(struct ksmbd_conn *conn, struct ksmbd_tree_connect *tcon,
newattrs.ia_valid |= ATTR_GID;
newattrs.ia_gid = fattr.cf_gid;
}
- newattrs.ia_valid |= ATTR_MODE;
- newattrs.ia_mode = (inode->i_mode & ~0777) | (fattr.cf_mode & 0777);
-
- ksmbd_vfs_remove_acl_xattrs(idmap, path);
+ /* only the owner and group without a DACL: mode and ACLs stay */
+ if (dacl) {
+ newattrs.ia_valid |= ATTR_MODE;
+ newattrs.ia_mode = (inode->i_mode & ~0777) | (fattr.cf_mode & 0777);
+ ksmbd_vfs_remove_acl_xattrs(idmap, path);
+ }
/* Update posix acls */
- if (IS_ENABLED(CONFIG_FS_POSIX_ACL) && fattr.cf_dacls) {
+ if (IS_ENABLED(CONFIG_FS_POSIX_ACL) && dacl && fattr.cf_dacls) {
rc = set_posix_acl(idmap, path->dentry,
ACL_TYPE_ACCESS, fattr.cf_acls);
if (rc < 0)
@@ -1821,8 +1831,11 @@ int set_info_sec(struct ksmbd_conn *conn, struct ksmbd_tree_connect *tcon,
if (rc)
goto out;
- /* Check it only calling from SD BUFFER context */
- if (type_check && !(le16_to_cpu(pntsd->type) & DACL_PRESENT))
+ /*
+ * The stored descriptor keeps its DACL when only the owner or group
+ * changed: the owner shown is the inode's anyway.
+ */
+ if (!dacl)
goto out;
if (test_share_config_flag(tcon->share_conf, KSMBD_SHARE_FLAG_ACL_XATTR)) {
diff --git a/fs/smb/server/smbacl.h b/fs/smb/server/smbacl.h
index b6a01f0d45..d4c9b679d8 100644
--- a/fs/smb/server/smbacl.h
+++ b/fs/smb/server/smbacl.h
@@ -99,7 +99,7 @@ int smb_check_perm_dacl(struct ksmbd_conn *conn, const struct path *path,
bool strict);
int set_info_sec(struct ksmbd_conn *conn, struct ksmbd_tree_connect *tcon,
const struct path *path, struct smb_ntsd *pntsd, int ntsd_len,
- bool type_check, bool get_write);
+ bool get_write);
void id_to_sid(unsigned int cid, uint sidtype, struct smb_sid *ssid);
void ksmbd_uid_to_sid(kuid_t uid, uint sidtype, struct smb_sid *ssid);
void ksmbd_gid_to_sid(kgid_t gid, uint sidtype, struct smb_sid *ssid);
--
2.56.0
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [RFC PATCH 1/6] ksmbd: don't ignore the owner and group of a descriptor without a DACL
2026-11-10 9:02 [RFC PATCH 1/6] ksmbd: don't ignore the owner and group of a descriptor without a DACL Jan Koester
@ 2026-11-11 16:00 ` Ian Bridges
0 siblings, 0 replies; 3+ messages in thread
From: Ian Bridges @ 2026-11-11 16:00 UTC (permalink / raw)
To: jan.koester
Cc: linkinjeon, senozhatsky, tom, chenxiaosong, brauner, linux-cifs,
linux-fsdevel, linux-kernel, Ian Bridges
Hi Jan,
On Tue, Nov 10, 2026 at 09:02:27AM +0000, Jan Koester wrote:
> + /* parse_sec_desc() rewrites type */
> + dacl = le16_to_cpu(pntsd->type) & DACL_PRESENT;
Shouldn't the parts to change come from the SecurityInformation of the
request rather than from DACL_PRESENT? As far as I can see, the Linux
cifs client's chown can send OWNER_SECURITY_INFORMATION alone with a
descriptor that still carries the DACL. It does so when no ACE names
the old owner. Would this patch then parse that DACL again, rebuild the
POSIX ACL from it and replace the stored descriptor, though the client
only asked to change the owner? And in the reverse case, would a
request for the DACL alone still change the owner when the descriptor
carries one?
Thanks,
Ian
^ permalink raw reply [flat|nested] 3+ messages in thread
* [RFC PATCH 1/6] ksmbd: don't ignore the owner and group of a descriptor without a DACL
2026-10-28 6:19 [RFC PATCH 0/6] ksmbd: Windows SIDs for user and group ids Jan Koester
@ 2026-10-28 6:19 ` Jan Koester
0 siblings, 0 replies; 3+ messages in thread
From: Jan Koester @ 2026-10-28 6:19 UTC (permalink / raw)
To: Namjae Jeon, Steve French
Cc: Sergey Senozhatsky, Tom Talpey, ChenXiaoSong, Christian Brauner,
linux-cifs, linux-fsdevel, samba-technical, linux-kernel
parse_sec_desc() returned before looking at the owner and group SIDs
when the descriptor had no DACL_PRESENT, so an SMB2 SET_INFO with only
OWNER_SECURITY_INFORMATION or GROUP_SECURITY_INFORMATION -- what Windows
sends to take ownership, and smbcacls -C/-G -- did nothing and still
reported success.
Map the owner and group in any case and parse the DACL only if there is
one. set_info_sec() then changes the mode and the POSIX ACLs only for a
descriptor with a DACL, and leaves the stored descriptor alone without
one: replacing it would drop its DACL, and the owner shown comes from
the inode anyway. This also covers the SD buffer create context, so its
type_check argument goes away.
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Jan Koester <jan.koester@tuxist.de>
---
fs/smb/server/smb2pdu.c | 4 ++--
fs/smb/server/smbacl.c | 39 ++++++++++++++++++++++++++-------------
fs/smb/server/smbacl.h | 2 +-
3 files changed, 29 insertions(+), 16 deletions(-)
diff --git a/fs/smb/server/smb2pdu.c b/fs/smb/server/smb2pdu.c
index 230bea45de..8e47d725c1 100644
--- a/fs/smb/server/smb2pdu.c
+++ b/fs/smb/server/smb2pdu.c
@@ -3661,7 +3661,7 @@ static int smb2_create_sd_buffer(struct ksmbd_work *work,
sizeof(struct create_sd_buf_req))
return -EINVAL;
return set_info_sec(work->conn, work->tcon, path, &sd_buf->ntsd,
- le32_to_cpu(sd_buf->ccontext.DataLength), true, false);
+ le32_to_cpu(sd_buf->ccontext.DataLength), false);
}
static void ksmbd_acls_fattr(struct smb_fattr *fattr,
@@ -8535,7 +8535,7 @@ static int smb2_set_info_sec(struct ksmbd_file *fp, int addition_info,
return -EACCES;
return set_info_sec(fp->conn, fp->tcon, &fp->filp->f_path, pntsd,
- buf_len, false, true);
+ buf_len, true);
}
/**
diff --git a/fs/smb/server/smbacl.c b/fs/smb/server/smbacl.c
index 126264cc07..d1cf63a9fe 100644
--- a/fs/smb/server/smbacl.c
+++ b/fs/smb/server/smbacl.c
@@ -1017,12 +1017,14 @@ int parse_sec_desc(struct mnt_idmap *idmap, struct smb_ntsd *pntsd,
le32_to_cpu(pntsd->sacloffset), dacloffset);
pntsd_type = le16_to_cpu(pntsd->type);
- if (!(pntsd_type & DACL_PRESENT)) {
+ /*
+ * Without a DACL only the owner and group change (Windows sends that
+ * to take ownership): don't return before them.
+ */
+ if (pntsd_type & DACL_PRESENT)
+ pntsd->type = cpu_to_le16(DACL_PRESENT);
+ else
ksmbd_debug(SMB, "DACL_PRESENT in DACL type is not set\n");
- return rc;
- }
-
- pntsd->type = cpu_to_le16(DACL_PRESENT);
if (pntsd->osidoffset) {
if (le32_to_cpu(pntsd->osidoffset) < sizeof(struct smb_ntsd))
@@ -1060,6 +1062,9 @@ int parse_sec_desc(struct mnt_idmap *idmap, struct smb_ntsd *pntsd,
}
}
+ if (!(pntsd_type & DACL_PRESENT))
+ return 0;
+
if ((pntsd_type & (DACL_AUTO_INHERITED | DACL_AUTO_INHERIT_REQ)) ==
(DACL_AUTO_INHERITED | DACL_AUTO_INHERIT_REQ))
pntsd->type |= cpu_to_le16(DACL_AUTO_INHERITED);
@@ -1768,18 +1773,21 @@ int smb_check_perm_dacl(struct ksmbd_conn *conn, const struct path *path,
int set_info_sec(struct ksmbd_conn *conn, struct ksmbd_tree_connect *tcon,
const struct path *path, struct smb_ntsd *pntsd, int ntsd_len,
- bool type_check, bool get_write)
+ bool get_write)
{
int rc;
struct smb_fattr fattr = {{0}};
struct inode *inode = d_inode(path->dentry);
struct mnt_idmap *idmap = mnt_idmap(path->mnt);
struct iattr newattrs;
+ bool dacl;
fattr.cf_uid = INVALID_UID;
fattr.cf_gid = INVALID_GID;
fattr.cf_mode = inode->i_mode;
+ /* parse_sec_desc() rewrites type */
+ dacl = le16_to_cpu(pntsd->type) & DACL_PRESENT;
rc = parse_sec_desc(idmap, pntsd, ntsd_len, &fattr);
if (rc)
goto out;
@@ -1793,12 +1801,14 @@ int set_info_sec(struct ksmbd_conn *conn, struct ksmbd_tree_connect *tcon,
newattrs.ia_valid |= ATTR_GID;
newattrs.ia_gid = fattr.cf_gid;
}
- newattrs.ia_valid |= ATTR_MODE;
- newattrs.ia_mode = (inode->i_mode & ~0777) | (fattr.cf_mode & 0777);
-
- ksmbd_vfs_remove_acl_xattrs(idmap, path);
+ /* only the owner and group without a DACL: mode and ACLs stay */
+ if (dacl) {
+ newattrs.ia_valid |= ATTR_MODE;
+ newattrs.ia_mode = (inode->i_mode & ~0777) | (fattr.cf_mode & 0777);
+ ksmbd_vfs_remove_acl_xattrs(idmap, path);
+ }
/* Update posix acls */
- if (IS_ENABLED(CONFIG_FS_POSIX_ACL) && fattr.cf_dacls) {
+ if (IS_ENABLED(CONFIG_FS_POSIX_ACL) && dacl && fattr.cf_dacls) {
rc = set_posix_acl(idmap, path->dentry,
ACL_TYPE_ACCESS, fattr.cf_acls);
if (rc < 0)
@@ -1821,8 +1831,11 @@ int set_info_sec(struct ksmbd_conn *conn, struct ksmbd_tree_connect *tcon,
if (rc)
goto out;
- /* Check it only calling from SD BUFFER context */
- if (type_check && !(le16_to_cpu(pntsd->type) & DACL_PRESENT))
+ /*
+ * The stored descriptor keeps its DACL when only the owner or group
+ * changed: the owner shown is the inode's anyway.
+ */
+ if (!dacl)
goto out;
if (test_share_config_flag(tcon->share_conf, KSMBD_SHARE_FLAG_ACL_XATTR)) {
diff --git a/fs/smb/server/smbacl.h b/fs/smb/server/smbacl.h
index b6a01f0d45..d4c9b679d8 100644
--- a/fs/smb/server/smbacl.h
+++ b/fs/smb/server/smbacl.h
@@ -99,7 +99,7 @@ int smb_check_perm_dacl(struct ksmbd_conn *conn, const struct path *path,
bool strict);
int set_info_sec(struct ksmbd_conn *conn, struct ksmbd_tree_connect *tcon,
const struct path *path, struct smb_ntsd *pntsd, int ntsd_len,
- bool type_check, bool get_write);
+ bool get_write);
void id_to_sid(unsigned int cid, uint sidtype, struct smb_sid *ssid);
void ksmbd_uid_to_sid(kuid_t uid, uint sidtype, struct smb_sid *ssid);
void ksmbd_gid_to_sid(kgid_t gid, uint sidtype, struct smb_sid *ssid);
--
2.56.0
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-11-11 16:00 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-11-10 9:02 [RFC PATCH 1/6] ksmbd: don't ignore the owner and group of a descriptor without a DACL Jan Koester
2026-11-11 16:00 ` Ian Bridges
-- strict thread matches above, loose matches on Subject: below --
2026-10-28 6:19 [RFC PATCH 0/6] ksmbd: Windows SIDs for user and group ids Jan Koester
2026-10-28 6:19 ` [RFC PATCH 1/6] ksmbd: don't ignore the owner and group of a descriptor without a DACL Jan Koester
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®