Skip to contentSkip to conversation

cifs: don't send uninitialized memory to sock_{send,recv}msg()

PATCH In review Stefan Metzmacher 2 participants 4 messages Sep 14, 2022

Subsystems (3)
Stable branch SMB client (CIFS) Kernel SMB3 server (ksmbd)
lore
Share & tools
lore

Stefan Metzmacher

author
#0 lore raw

cover cifs: don't send uninitialized memory to sock_{send,recv}msg()

Passing just half initialized struct msghdr variables down to sock_{send,recv}msg() means we're waiting for a disater to happen...

I added the removal of passing the destination address to tcp as a separate patch in order to explain it separately.

Stefan Metzmacher (2):
cifs: don't send down the destination address to sendmsg for a
SOCK_STREAM
cifs: always initialize struct msghdr smb_msg completely

 fs/cifs/connect.c   | 11 +++--------
 fs/cifs/transport.c |  6 +-----
 2 files changed, 4 insertions(+), 13 deletions(-)
Signature · 1 line
2.34.1
author
#1 lore raw

PATCH 1/2 cifs: don't send down the destination address to sendmsg for a SOCK_STREAM

This is ignored anyway by the tcp layer.

Signed-off-by Stefan Metzmacher <[email protected]>
Cc [email protected]
Reviewed-by Paulo Alcantara (SUSE) <[email protected]>
---
 fs/cifs/transport.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
fs/cifs/transport.c +2 -2
index c2fe035e573b..a43c87c1d343 100644
@@ -194,8 +194,8 @@ smb_send_kvec(struct TCP_Server_Info *server, struct msghdr *smb_msg,
 
 	*sent = 0;
 
-	smb_msg->msg_name = (struct sockaddr *) &server->dstaddr;
-	smb_msg->msg_namelen = sizeof(struct sockaddr);
+	smb_msg->msg_name = NULL;
+	smb_msg->msg_namelen = 0;
 	smb_msg->msg_control = NULL;
 	smb_msg->msg_controllen = 0;
 	if (server->noblocksnd)
Signature · 1 line
2.34.1
author
#2 lore raw

PATCH 2/2 cifs: always initialize struct msghdr smb_msg completely

In reply to Stefan Metzmacher

So far we were just lucky because the uninitialized members of struct msghdr are not used by default on a SOCK_STREAM tcp socket.

But as new things like msg_ubuf and sg_from_iter where added recently, we should play on the safe side and avoid potention problems in future.

Signed-off-by Stefan Metzmacher <[email protected]>
Cc [email protected]
Reviewed-by Paulo Alcantara (SUSE) <[email protected]>
---
 fs/cifs/connect.c   | 11 +++--------
 fs/cifs/transport.c |  6 +-----
 2 files changed, 4 insertions(+), 13 deletions(-)
fs/cifs/connect.c +3 -8
index a0a06b6f252b..0225f4c8adf0 100644
@@ -702,9 +702,6 @@ cifs_readv_from_socket(struct TCP_Server_Info *server, struct msghdr *smb_msg)
 	int length = 0;
 	int total_read;
 
-	smb_msg->msg_control = NULL;
-	smb_msg->msg_controllen = 0;
-
 	for (total_read = 0; msg_data_left(smb_msg); total_read += length) {
 		try_to_freeze();
 

@@ -760,7 +757,7 @@ int
 cifs_read_from_socket(struct TCP_Server_Info *server, char *buf,
 		      unsigned int to_read)
 {
-	struct msghdr smb_msg;
+	struct msghdr smb_msg = {};
 	struct kvec iov = {.iov_base = buf, .iov_len = to_read};
 	iov_iter_kvec(&smb_msg.msg_iter, READ, &iov, 1, to_read);
 

@@ -770,15 +767,13 @@ cifs_read_from_socket(struct TCP_Server_Info *server, char *buf,
 ssize_t
 cifs_discard_from_socket(struct TCP_Server_Info *server, size_t to_read)
 {
-	struct msghdr smb_msg;
+	struct msghdr smb_msg = {};
 
 	/*
 	 *  iov_iter_discard already sets smb_msg.type and count and iov_offset
 	 *  and cifs_readv_from_socket sets msg_control and msg_controllen
 	 *  so little to initialize in struct msghdr
 	 */
-	smb_msg.msg_name = NULL;
-	smb_msg.msg_namelen = 0;
 	iov_iter_discard(&smb_msg.msg_iter, READ, to_read);
 
 	return cifs_readv_from_socket(server, &smb_msg);

@@ -788,7 +783,7 @@ int
 cifs_read_page_from_socket(struct TCP_Server_Info *server, struct page *page,
 	unsigned int page_offset, unsigned int to_read)
 {
-	struct msghdr smb_msg;
+	struct msghdr smb_msg = {};
 	struct bio_vec bv = {
 		.bv_page = page, .bv_len = to_read, .bv_offset = page_offset};
 	iov_iter_bvec(&smb_msg.msg_iter, READ, &bv, 1, to_read);
fs/cifs/transport.c +1 -5
index a43c87c1d343..9a2753e21170 100644
@@ -194,10 +194,6 @@ smb_send_kvec(struct TCP_Server_Info *server, struct msghdr *smb_msg,
 
 	*sent = 0;
 
-	smb_msg->msg_name = NULL;
-	smb_msg->msg_namelen = 0;
-	smb_msg->msg_control = NULL;
-	smb_msg->msg_controllen = 0;
 	if (server->noblocksnd)
 		smb_msg->msg_flags = MSG_DONTWAIT + MSG_NOSIGNAL;
 	else

@@ -309,7 +305,7 @@ __smb_send_rqst(struct TCP_Server_Info *server, int num_rqst,
 	sigset_t mask, oldmask;
 	size_t total_len = 0, sent, size;
 	struct socket *ssocket = server->ssocket;
-	struct msghdr smb_msg;
+	struct msghdr smb_msg = {};
 	__be32 rfc1002_marker;
 
 	if (cifs_rdma_enabled(server)) {
Signature · 1 line
2.34.1
reviewer
#3 lore raw
In reply to Stefan Metzmacher

Reviewed by me for both patches

Quoted 19 lines · Stefan Metzmacher

Passing just half initialized struct msghdr variables down to sock_{send,recv}msg() means we're waiting for a disater to happen...

I added the removal of passing the destination address to tcp as a separate patch in order to explain it separately.

Stefan Metzmacher (2):
cifs: don't send down the destination address to sendmsg for a
SOCK_STREAM
cifs: always initialize struct msghdr smb_msg completely

 fs/cifs/connect.c   | 11 +++--------
 fs/cifs/transport.c |  6 +-----
 2 files changed, 4 insertions(+), 13 deletions(-)

--
2.34.1