CIFS: set *resp_buf_type to NO_BUFFER on error

PATCH In review Dan Carpenter 4 participants 6 messages Feb 7, 2017 – Apr 23, 2018

Subsystems (3)

Kernel janitors SMB client (CIFS) Kernel SMB3 server (ksmbd)

lore

Share & tools

lore

Dan Carpenter

author

2017-02-07 13:18 UTC #0 lore raw

PATCH 1/1 CIFS: set *resp_buf_type to NO_BUFFER on error

We recently shuffled this code around and introduced a new error path before *resp_buf_type gets initialized. It creates uninitialized variable bugs in the callers.

fs/cifs/smb2pdu.c:579 SMB2_negotiate()
error: uninitialized symbol 'resp_buftype'.

Fixes 738f9de5cdb9 ("CIFS: Send RFC1001 length in a separate iov")

Signed-off-by Dan Carpenter <[email protected]>

fs/cifs/transport.c +2 -0

index 526f0533cb4e..8fa5e058fb15 100644
@@ -807,6 +807,8 @@ SendReceive2(const unsigned int xid, struct cifs_ses *ses,
 	struct kvec *new_iov;
 	int rc;
 
+	*resp_buf_type = CIFS_NO_BUFFER; /* no response buf yet */
+
 	new_iov = kmalloc(sizeof(struct kvec) * (n_vec + 1), GFP_KERNEL);
 	if (!new_iov)
 		return -ENOMEM;

Aurélien Aptel

Reviewed-by on v1 1/1

2017-02-07 15:33 UTC #1 lore raw

Quoted 24 lines · Dan Carpenter

We recently shuffled this code around and introduced a new error path before *resp_buf_type gets initialized. It creates uninitialized variable bugs in the callers.
fs/cifs/smb2pdu.c:579 SMB2_negotiate()
error: uninitialized symbol 'resp_buftype'.
Fixes 738f9de5cdb9 ("CIFS: Send RFC1001 length in a separate iov")
Signed-off-by Dan Carpenter <[email protected]>
fs/cifs/transport.c +2 -1
index 526f0533cb4e..8fa5e058fb15 100644
@@ -807,6 +807,8 @@ SendReceive2(const unsigned int xid, struct cifs_ses *ses,
 	struct kvec *new_iov;
 	int rc;
 
+	*resp_buf_type = CIFS_NO_BUFFER; /* no response buf yet */
+
 	new_iov = kmalloc(sizeof(struct kvec) * (n_vec + 1), GFP_KERNEL);
 	if (!new_iov)
 		return -ENOMEM;
--

Quoted 4 lines

To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

LGTM. To be a bit more explicit:

resp_buf_type is an output parameter of the SendReceive2 function and in case the kmalloc failed the function could return to the caller with this parameter left uninitialized.

Reviewed-by Aurelien Aptel <[email protected]>

Signature · 4 lines

Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)

Pavel Shilovsky

Reviewed-by on v1 1/1

2017-02-08 01:00 UTC #2 lore raw

In reply to Dan Carpenter

2017-02-07 5:18 GMT-08:00 Dan Carpenter <[email protected]>:

Quoted 24 lines

We recently shuffled this code around and introduced a new error path before *resp_buf_type gets initialized. It creates uninitialized variable bugs in the callers.
fs/cifs/smb2pdu.c:579 SMB2_negotiate()
error: uninitialized symbol 'resp_buftype'.
Fixes 738f9de5cdb9 ("CIFS: Send RFC1001 length in a separate iov")
Signed-off-by Dan Carpenter <[email protected]>
fs/cifs/transport.c +2 -1
index 526f0533cb4e..8fa5e058fb15 100644
@@ -807,6 +807,8 @@ SendReceive2(const unsigned int xid, struct cifs_ses *ses,
        struct kvec *new_iov;
        int rc;
 
+       *resp_buf_type = CIFS_NO_BUFFER; /* no response buf yet */
+
        new_iov = kmalloc(sizeof(struct kvec) * (n_vec + 1), GFP_KERNEL);
        if (!new_iov)
                return -ENOMEM;
--

Quoted 3 lines

To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Good catch, thanks!

Reviewed-by Pavel Shilovsky <[email protected]>

Signature · 2 lines

Best regards,
Pavel Shilovsky

Steve French

2018-04-22 15:30 UTC #3 lore raw

In reply to Dan Carpenter

Looks like missed merging this one. The code paths have changed around this - but on error they seem to ignore the resp_buf_type field, but looks like it would be cleaner to initialize it, so created an updated patch to do roughly the same thing and merged into cifs-2.6.git for-next

Dan,
Any objections?

Quoted 37 lines · Pavel Shilovsky

2017-02-07 5:18 GMT-08:00 Dan Carpenter <[email protected]>:
Quoted 24 lines
We recently shuffled this code around and introduced a new error path before *resp_buf_type gets initialized. It creates uninitialized variable bugs in the callers.
fs/cifs/smb2pdu.c:579 SMB2_negotiate()
error: uninitialized symbol 'resp_buftype'.
Fixes 738f9de5cdb9 ("CIFS: Send RFC1001 length in a separate iov")
Signed-off-by Dan Carpenter <[email protected]>
fs/cifs/transport.c +2 -1
index 526f0533cb4e..8fa5e058fb15 100644
@@ -807,6 +807,8 @@ SendReceive2(const unsigned int xid, struct cifs_ses *ses,
        struct kvec *new_iov;
        int rc;
 
+       *resp_buf_type = CIFS_NO_BUFFER; /* no response buf yet */
+
        new_iov = kmalloc(sizeof(struct kvec) * (n_vec + 1), GFP_KERNEL);
        if (!new_iov)
                return -ENOMEM;
--
Quoted 3 lines
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Good catch, thanks!
Reviewed-by Pavel Shilovsky <[email protected]>
--
Best regards,
Pavel Shilovsky

Signature · 3 lines

Thanks,

Steve

From c09c13668f624ede336489ef8412c2471c5c3afc Mon Sep 17 00:00:00 2001
From: Steve French <[email protected]>
Date: Sun, 22 Apr 2018 10:24:19 -0500
Subject: [PATCH] CIFS: set *resp_buf_type to NO_BUFFER on error

Dan Carpenter had pointed this out a while ago, but the code around this had changed so wasn't causing any problems since that field was not used in this error path.

Still, it is cleaner to always initialize this field, so changing the error path to set it.

CC Dan Carpenter <[email protected]>

Signed-off-by Steve French <[email protected]>

---
 fs/cifs/transport.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

fs/cifs/transport.c +4 -1

index 8f6f25918229..3fb0e433b8e2 100644
@@ -834,8 +834,11 @@ SendReceive2(const unsigned int xid, struct cifs_ses *ses,
 	if (n_vec + 1 > CIFS_MAX_IOV_SIZE) {
 		new_iov = kmalloc(sizeof(struct kvec) * (n_vec + 1),
 				  GFP_KERNEL);
-		if (!new_iov)
+		if (!new_iov) {
+			/* otherwise cifs_send_recv below sets resp_buf_type */
+			*resp_buf_type = CIFS_NO_BUFFER;
 			return -ENOMEM;
+		}
 	} else
 		new_iov = s_iov;

Signature · 1 line

2.14.1

Dan Carpenter

author

2018-04-23 11:54 UTC #4 lore raw

In reply to Dan Carpenter

Quoted 10 lines · Steve French

Looks like missed merging this one. The code paths have changed around this - but on error they seem to ignore the resp_buf_type field, but looks like it would be cleaner to initialize it, so created an updated patch to do roughly the same thing and merged into cifs-2.6.git for-next
Dan,
Any objections?

I don't have any objections. It sounds like you already wrote the patch or were you asking me to do that? Either way is fine.

regards,
dan carpenter

Steve French

2018-04-23 15:17 UTC #5 lore raw

In reply to Dan Carpenter

Yes - wrote a lightly updated patch and put it in linux-next (cifs-2.6.git for-next branch).

Quoted 18 lines · Dan Carpenter

Quoted 10 lines · Steve French
Looks like missed merging this one. The code paths have changed around this - but on error they seem to ignore the resp_buf_type field, but looks like it would be cleaner to initialize it, so created an updated patch to do roughly the same thing and merged into cifs-2.6.git for-next
Dan,
Any objections?
I don't have any objections. It sounds like you already wrote the patch or were you asking me to do that? Either way is fine.
regards,
dan carpenter

Signature · 3 lines

Thanks,

Steve