Skip to content

Commit

Permalink
smb: client: replace deprecated strncpy with strscpy
Browse files Browse the repository at this point in the history
strncpy() is deprecated for use on NUL-terminated destination strings
[1] and as such we should prefer more robust and less ambiguous string
interfaces.

In cifssmb.c:
Using strncpy with a length argument equal to strlen(src) is generally
dangerous because it can cause string buffers to not be NUL-terminated.
In this case, however, there was extra effort made to ensure the buffer
was NUL-terminated via a manual NUL-byte assignment. In an effort to rid
the kernel of strncpy() use, let's swap over to using strscpy() which
guarantees NUL-termination on the destination buffer.

To handle the case where ea_name is NULL, let's use the ?: operator to
substitute in an empty string, thereby allowing strscpy to still
NUL-terminate the destintation string.

Interesting note: this flex array buffer may go on to also have some
value encoded after the NUL-termination:
|	if (ea_value_len)
|		memcpy(parm_data->list.name + name_len + 1,
|			ea_value, ea_value_len);

Now for smb2ops.c and smb2transport.c:
Both of these cases are simple, strncpy() is used to copy string
literals which have a length less than the destination buffer's size. We
can simply swap in the new 2-argument version of strscpy() introduced in
Commit e6584c3 ("string: Allow 2-argument strscpy()").

Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
Link: https://github.com/KSPP/linux/issues/90
Cc: linux-hardening@vger.kernel.org
Signed-off-by: Justin Stitt <justinstitt@google.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
  • Loading branch information
Justin Stitt authored and Steve French committed Mar 31, 2024
1 parent 39cd87c commit ebd9779
Show file tree
Hide file tree
Showing 3 changed files with 4 additions and 6 deletions.
6 changes: 2 additions & 4 deletions fs/smb/client/cifssmb.c
Original file line number Diff line number Diff line change
Expand Up @@ -5854,10 +5854,8 @@ CIFSSMBSetEA(const unsigned int xid, struct cifs_tcon *tcon,
parm_data->list.EA_flags = 0;
/* we checked above that name len is less than 255 */
parm_data->list.name_len = (__u8)name_len;
/* EA names are always ASCII */
if (ea_name)
strncpy(parm_data->list.name, ea_name, name_len);
parm_data->list.name[name_len] = '\0';
/* EA names are always ASCII and NUL-terminated */
strscpy(parm_data->list.name, ea_name ?: "", name_len + 1);
parm_data->list.value_len = cpu_to_le16(ea_value_len);
/* caller ensures that ea_value_len is less than 64K but
we need to ensure that it fits within the smb */
Expand Down
2 changes: 1 addition & 1 deletion fs/smb/client/smb2ops.c
Original file line number Diff line number Diff line change
Expand Up @@ -3913,7 +3913,7 @@ smb21_set_oplock_level(struct cifsInodeInfo *cinode, __u32 oplock,
strcat(message, "W");
}
if (!new_oplock)
strncpy(message, "None", sizeof(message));
strscpy(message, "None");

cinode->oplock = new_oplock;
cifs_dbg(FYI, "%s Lease granted on inode %p\n", message,
Expand Down
2 changes: 1 addition & 1 deletion fs/smb/client/smb2transport.c
Original file line number Diff line number Diff line change
Expand Up @@ -659,7 +659,7 @@ smb2_sign_rqst(struct smb_rqst *rqst, struct TCP_Server_Info *server)
}
spin_unlock(&server->srv_lock);
if (!is_binding && !server->session_estab) {
strncpy(shdr->Signature, "BSRSPYL", 8);
strscpy(shdr->Signature, "BSRSPYL");
return 0;
}

Expand Down

0 comments on commit ebd9779

Please sign in to comment.