smb2_set_ea() checks that an EA name is followed by a NUL, but not that it contains none. The xattr layer takes the name as a C string, so the name ends at the first NUL. An EA named "DOSATTRIB\0X" with EaNameLength 11 passes smb2_is_private_ea(), which compares all EaNameLength bytes, and is then stored as user.DOSATTRIB, the xattr in which ksmbd keeps the DOS attributes and creation time of a file. A client with only FILE_WRITE_EA on a file can so replace them, which commit 4ea46ea602fc ("ksmbd: protect private extended attributes") meant to prevent. Any other name with a NUL is stored under the part before it. An EA name is EaNameLength bytes followed by a single terminating NUL (MS-FSCC 2.4.15), so a NUL inside it is malformed. Refuse such an entry as one without the terminating NUL already is, with STATUS_EA_LIST_INCONSISTENT, for both SET_INFO and the EA buffer of a CREATE. An AI assistant was used to draft the patch and as an independent reviewer. Fixes: 4ea46ea602fc ("ksmbd: protect private extended attributes") Cc: stable@vger.kernel.org Signed-off-by: Michail Andreas Michalopoulos --- Namjae, this gets around the private-EA check from 4ea46ea602fc, so I'm sending it to you first rather than to the list. Please post or apply it as you prefer. Tested on a KASAN/lockdep UML kernel based on ksmbd-for-next, with a share on tmpfs with user xattrs: setting, reading and deleting a user EA works as before; an EA named "ab\0cd" was stored as user.ab before this patch and is now refused with STATUS_EA_LIST_INCONSISTENT; DOSATTRIB is still refused with STATUS_ACCESS_DENIED. fs/smb/server/smb2pdu.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/fs/smb/server/smb2pdu.c b/fs/smb/server/smb2pdu.c index d17fe9f9ffc6d..066d30b4ec281 100644 --- a/fs/smb/server/smb2pdu.c +++ b/fs/smb/server/smb2pdu.c @@ -3441,7 +3441,14 @@ static int smb2_set_ea(struct smb2_ea_info *eabuf, unsigned int buf_len, if (!eabuf->EaNameLength) goto next; - if (eabuf->name[eabuf->EaNameLength]) { + /* + * The name is EaNameLength bytes and a terminating NUL, with no + * NUL inside it: the xattr name ends at the first NUL, so one + * inside would also get past smb2_is_private_ea(). + */ + if (eabuf->name[eabuf->EaNameLength] || + strnlen(eabuf->name, eabuf->EaNameLength) != + eabuf->EaNameLength) { rc = -EBADMSG; break; } base-commit: f4692d0e3b5e16d32d4ec7b1e0e58d7dbe74aaf4 -- 2.43.0