[patch] CIFS: set *resp_buf_type to NO_BUFFER on error

[patch] CIFS: set *resp_buf_type to NO_BUFFER on error

[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 <dan.carpenter@oracle.com>

diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 526f0533cb4e..8fa5e058fb15 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -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;

[patch] CIFS: set *resp_buf_type to NO_BUFFER on error

[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 <dan.carpenter@oracle.com>

diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 526f0533cb4e..8fa5e058fb15 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -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;

Re: [patch] CIFS: set *resp_buf_type to NO_BUFFER on error

[Aurélien Aptel]

Dan Carpenter <dan.carpenter@oracle.com> writes:
> 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 <dan.carpenter@oracle.com>
>
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index 526f0533cb4e..8fa5e058fb15 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -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;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo@vger.kernel.org
> 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 <aaptel@suse.com>

-- 
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)

Re: [patch] CIFS: set *resp_buf_type to NO_BUFFER on error

[Aurélien Aptel]

Dan Carpenter <dan.carpenter@oracle.com> writes:
> 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 <dan.carpenter@oracle.com>
>
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index 526f0533cb4e..8fa5e058fb15 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -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;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo@vger.kernel.org
> 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 <aaptel@suse.com>

-- 
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)

Re: [patch] CIFS: set *resp_buf_type to NO_BUFFER on error

[Pavel Shilovsky]

2017-02-07 5:18 GMT-08:00 Dan Carpenter <dan.carpenter@oracle.com>:
> 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 <dan.carpenter@oracle.com>
>
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index 526f0533cb4e..8fa5e058fb15 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -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;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Good catch, thanks!

Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>

-- 
Best regards,
Pavel Shilovsky

Re: [patch] CIFS: set *resp_buf_type to NO_BUFFER on error

[Pavel Shilovsky]

2017-02-07 5:18 GMT-08:00 Dan Carpenter <dan.carpenter@oracle.com>:
> 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 <dan.carpenter@oracle.com>
>
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index 526f0533cb4e..8fa5e058fb15 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -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;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Good catch, thanks!

Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>

-- 
Best regards,
Pavel Shilovsky

Re: [patch] CIFS: set *resp_buf_type to NO_BUFFER on error

[Steve French]

[-- Attachment #1: Type: text/plain, Size: 1727 bytes --]

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?

On Tue, Feb 7, 2017 at 7:00 PM, Pavel Shilovsky <piastryyy@gmail.com> wrote:
> 2017-02-07 5:18 GMT-08:00 Dan Carpenter <dan.carpenter@oracle.com>:
>> 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 <dan.carpenter@oracle.com>
>>
>> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
>> index 526f0533cb4e..8fa5e058fb15 100644
>> --- a/fs/cifs/transport.c
>> +++ b/fs/cifs/transport.c
>> @@ -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;
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> Good catch, thanks!
>
> Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
>
> --
> Best regards,
> Pavel Shilovsky
>



-- 
Thanks,

Steve

[-- Attachment #2: 0001-CIFS-set-resp_buf_type-to-NO_BUFFER-on-error.patch --]
[-- Type: text/x-patch, Size: 1195 bytes --]

From c09c13668f624ede336489ef8412c2471c5c3afc Mon Sep 17 00:00:00 2001
From: Steve French <smfrench@gmail.com>
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 <dan.carpenter@oracle.com>
Signed-off-by: Steve French <smfrench@gmail.com>
---
 fs/cifs/transport.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 8f6f25918229..3fb0e433b8e2 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -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;
 
-- 
2.14.1

Re: [patch] CIFS: set *resp_buf_type to NO_BUFFER on error

[Dan Carpenter]

On Sun, Apr 22, 2018 at 10:30:00AM -0500, Steve French wrote:
> 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

Re: [patch] CIFS: set *resp_buf_type to NO_BUFFER on error

[Steve French]

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

On Mon, Apr 23, 2018 at 6:54 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Sun, Apr 22, 2018 at 10:30:00AM -0500, Steve French wrote:
>> 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
>



-- 
Thanks,

Steve