Skip to content

Commit ebd9779

Browse files
JustinStittsmfrench
authored andcommitted
smb: client: replace deprecated strncpy with strscpy
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: KSPP#90 Cc: [email protected] Signed-off-by: Justin Stitt <[email protected]> Reviewed-by: Kees Cook <[email protected]> Signed-off-by: Steve French <[email protected]>
1 parent 39cd87c commit ebd9779

File tree

3 files changed

+4
-6
lines changed

3 files changed

+4
-6
lines changed

fs/smb/client/cifssmb.c

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5854,10 +5854,8 @@ CIFSSMBSetEA(const unsigned int xid, struct cifs_tcon *tcon,
58545854
parm_data->list.EA_flags = 0;
58555855
/* we checked above that name len is less than 255 */
58565856
parm_data->list.name_len = (__u8)name_len;
5857-
/* EA names are always ASCII */
5858-
if (ea_name)
5859-
strncpy(parm_data->list.name, ea_name, name_len);
5860-
parm_data->list.name[name_len] = '\0';
5857+
/* EA names are always ASCII and NUL-terminated */
5858+
strscpy(parm_data->list.name, ea_name ?: "", name_len + 1);
58615859
parm_data->list.value_len = cpu_to_le16(ea_value_len);
58625860
/* caller ensures that ea_value_len is less than 64K but
58635861
we need to ensure that it fits within the smb */

fs/smb/client/smb2ops.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3913,7 +3913,7 @@ smb21_set_oplock_level(struct cifsInodeInfo *cinode, __u32 oplock,
39133913
strcat(message, "W");
39143914
}
39153915
if (!new_oplock)
3916-
strncpy(message, "None", sizeof(message));
3916+
strscpy(message, "None");
39173917

39183918
cinode->oplock = new_oplock;
39193919
cifs_dbg(FYI, "%s Lease granted on inode %p\n", message,

fs/smb/client/smb2transport.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -659,7 +659,7 @@ smb2_sign_rqst(struct smb_rqst *rqst, struct TCP_Server_Info *server)
659659
}
660660
spin_unlock(&server->srv_lock);
661661
if (!is_binding && !server->session_estab) {
662-
strncpy(shdr->Signature, "BSRSPYL", 8);
662+
strscpy(shdr->Signature, "BSRSPYL");
663663
return 0;
664664
}
665665

0 commit comments

Comments
 (0)