diff mbox series

[focal/linux,bionic/linux,hirsute/linux,hirsute/linux-azure] Revert "CIFS: Fix a potencially linear read overflow"

Message ID 20211129151458.22189-1-tim.gardner@canonical.com
State New
Headers show
Series [focal/linux,bionic/linux,hirsute/linux,hirsute/linux-azure] Revert "CIFS: Fix a potencially linear read overflow" | expand

Commit Message

Tim Gardner Nov. 29, 2021, 3:14 p.m. UTC 
BugLink: https://bugs.launchpad.net/bugs/1952094

This reverts commit 3d5631a27ec4767ac80dbf553f9ae501b18e07e3.

This stable patch causes a regression. There are no subsequent
upstream fixes.

Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
---

I'll ask the upstream reviewers about reverting or fixing this patch.
Its been included in many stable releases:

linux-4.4.y.txt:0955df2d9bf4857e3e2287e3028903e6cec06c30 CIFS: Fix a potencially linear read overflow
linux-4.9.y.txt:8878af780747f498551b7d360cae61b415798f18 CIFS: Fix a potencially linear read overflow
linux-4.14.y.txt:20967547ffc6039f17c63a1c24eb779ee166b245 CIFS: Fix a potencially linear read overflow
linux-4.19.y.txt:bea655491daf39f1934a71bf576bf3499092d3a4 CIFS: Fix a potencially linear read overflow
linux-5.4.y.txt:b444064a0e0ef64491b8739a9ae05a952b5f8974 CIFS: Fix a potencially linear read overflow
linux-5.10.y.txt:6c4857203ffa36918136756a889b12c5864bc4ad CIFS: Fix a potencially linear read overflow
linux-5.13.y.txt:9bffe470e9b537075345406512df01ca2188b725 CIFS: Fix a potencially linear read overflow
linux-5.14.y.txt:c41dd61c86482ab34f6f039b13296308018fd99b CIFS: Fix a potencially linear read overflow

rtg
---
 fs/cifs/cifs_unicode.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comments

Kamal Mostafa Nov. 29, 2021, 4:17 p.m. UTC | #1 
Ack this revert for all series, including Impish.

Acked-by: Kamal Mostafa <kamal@canonical.com>

 -Kamal


On Mon, Nov 29, 2021 at 08:14:58AM -0700, Tim Gardner wrote:
> BugLink: https://bugs.launchpad.net/bugs/1952094
> 
> This reverts commit 3d5631a27ec4767ac80dbf553f9ae501b18e07e3.
> 
> This stable patch causes a regression. There are no subsequent
> upstream fixes.
> 
> Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
> ---
> 
> I'll ask the upstream reviewers about reverting or fixing this patch.
> Its been included in many stable releases:
> 
> linux-4.4.y.txt:0955df2d9bf4857e3e2287e3028903e6cec06c30 CIFS: Fix a potencially linear read overflow
> linux-4.9.y.txt:8878af780747f498551b7d360cae61b415798f18 CIFS: Fix a potencially linear read overflow
> linux-4.14.y.txt:20967547ffc6039f17c63a1c24eb779ee166b245 CIFS: Fix a potencially linear read overflow
> linux-4.19.y.txt:bea655491daf39f1934a71bf576bf3499092d3a4 CIFS: Fix a potencially linear read overflow
> linux-5.4.y.txt:b444064a0e0ef64491b8739a9ae05a952b5f8974 CIFS: Fix a potencially linear read overflow
> linux-5.10.y.txt:6c4857203ffa36918136756a889b12c5864bc4ad CIFS: Fix a potencially linear read overflow
> linux-5.13.y.txt:9bffe470e9b537075345406512df01ca2188b725 CIFS: Fix a potencially linear read overflow
> linux-5.14.y.txt:c41dd61c86482ab34f6f039b13296308018fd99b CIFS: Fix a potencially linear read overflow
> 
> rtg
> ---
>  fs/cifs/cifs_unicode.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/cifs/cifs_unicode.c b/fs/cifs/cifs_unicode.c
> index 171ad8b42107e..9bd03a2310328 100644
> --- a/fs/cifs/cifs_unicode.c
> +++ b/fs/cifs/cifs_unicode.c
> @@ -358,9 +358,14 @@ cifs_strndup_from_utf16(const char *src, const int maxlen,
>  		if (!dst)
>  			return NULL;
>  		cifs_from_utf16(dst, (__le16 *) src, len, maxlen, codepage,
> -				NO_MAP_UNI_RSVD);
> +			       NO_MAP_UNI_RSVD);
>  	} else {
> -		dst = kstrndup(src, maxlen, GFP_KERNEL);
> +		len = strnlen(src, maxlen);
> +		len++;
> +		dst = kmalloc(len, GFP_KERNEL);
> +		if (!dst)
> +			return NULL;
> +		strlcpy(dst, src, len);
>  	}
>  
>  	return dst;
> -- 
> 2.34.1
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
Thadeu Lima de Souza Cascardo Nov. 29, 2021, 5:38 p.m. UTC | #2 
On Mon, Nov 29, 2021 at 08:14:58AM -0700, Tim Gardner wrote:
> BugLink: https://bugs.launchpad.net/bugs/1952094
> 
> This reverts commit 3d5631a27ec4767ac80dbf553f9ae501b18e07e3.

Which is upstream commit f980d055a0f858d73d9467bb0b570721bbfcdfb8.

> 
> This stable patch causes a regression. There are no subsequent
> upstream fixes.
> 

I still fail to see how that introduces a regression.

Let's take a src string that is larger or equal to maxlen.

Before the patch:
len = strnlen(src, maxlen); -> len = maxlen;
len++; len = maxlen + 1;
dst = kmalloc(len, GPF_KERNEL); /* allocates a maxlen+1 sized dst */
if (!dst)
	return NULL;
strlcpy(dst, src, len); // where I replace len/size with maxlen+1 from now on
	size_t ret = strlen(src); // ret >= maxlen
	if (maxlen + 1) {
		size_t len = (ret >= maxlen + 1) ? maxlen + 1 - 1 : ret;
		/* here, either ret == maxlen, which makes len = ret = maxlen;
		   or ret > maxlen, which makes len = maxlen + 1 - 1 = maxlen */
		memcpy(dest, src, maxlen);
		dest[maxlen] = '\0';
	}

After the patch:
dst = kstrndup(src, maxlen, GFP_KERNEL);
	...
	len = strnlen(s, max); -> len = max;
	buf = kmalloc_track_caller(len+1, gfp); /* again, allocates maxlen+1 */
	/* from here on, replaces len with maxlen */
	if (buf) {
		memcpy(buf, s, maxlen);
		dest[maxlen] = '\0';
	}
	return buf; /* here, returns NULL as before the patch, in case allocation fails */


By looking at the bug, it looks like the failure happens when nls_load fails to
load nls_utf8 module. So, perhaps, the user missed the installation of
linux-modules-extra when installing from the archive, and then installed the
package when instructed to install the test kernel?

Cascardo.

> Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
> ---
> 
> I'll ask the upstream reviewers about reverting or fixing this patch.
> Its been included in many stable releases:
> 
> linux-4.4.y.txt:0955df2d9bf4857e3e2287e3028903e6cec06c30 CIFS: Fix a potencially linear read overflow
> linux-4.9.y.txt:8878af780747f498551b7d360cae61b415798f18 CIFS: Fix a potencially linear read overflow
> linux-4.14.y.txt:20967547ffc6039f17c63a1c24eb779ee166b245 CIFS: Fix a potencially linear read overflow
> linux-4.19.y.txt:bea655491daf39f1934a71bf576bf3499092d3a4 CIFS: Fix a potencially linear read overflow
> linux-5.4.y.txt:b444064a0e0ef64491b8739a9ae05a952b5f8974 CIFS: Fix a potencially linear read overflow
> linux-5.10.y.txt:6c4857203ffa36918136756a889b12c5864bc4ad CIFS: Fix a potencially linear read overflow
> linux-5.13.y.txt:9bffe470e9b537075345406512df01ca2188b725 CIFS: Fix a potencially linear read overflow
> linux-5.14.y.txt:c41dd61c86482ab34f6f039b13296308018fd99b CIFS: Fix a potencially linear read overflow
> 
> rtg
> ---
>  fs/cifs/cifs_unicode.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/cifs/cifs_unicode.c b/fs/cifs/cifs_unicode.c
> index 171ad8b42107e..9bd03a2310328 100644
> --- a/fs/cifs/cifs_unicode.c
> +++ b/fs/cifs/cifs_unicode.c
> @@ -358,9 +358,14 @@ cifs_strndup_from_utf16(const char *src, const int maxlen,
>  		if (!dst)
>  			return NULL;
>  		cifs_from_utf16(dst, (__le16 *) src, len, maxlen, codepage,
> -				NO_MAP_UNI_RSVD);
> +			       NO_MAP_UNI_RSVD);
>  	} else {
> -		dst = kstrndup(src, maxlen, GFP_KERNEL);
> +		len = strnlen(src, maxlen);
> +		len++;
> +		dst = kmalloc(len, GFP_KERNEL);
> +		if (!dst)
> +			return NULL;
> +		strlcpy(dst, src, len);
>  	}
>  
>  	return dst;
> -- 
> 2.34.1
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
Tim Gardner Nov. 29, 2021, 5:58 p.m. UTC | #3 
On 11/29/21 10:38 AM, Thadeu Lima de Souza Cascardo wrote:
> On Mon, Nov 29, 2021 at 08:14:58AM -0700, Tim Gardner wrote:
>> BugLink: https://bugs.launchpad.net/bugs/1952094
>>
>> This reverts commit 3d5631a27ec4767ac80dbf553f9ae501b18e07e3.
> 
> Which is upstream commit f980d055a0f858d73d9467bb0b570721bbfcdfb8.
> 
>>
>> This stable patch causes a regression. There are no subsequent
>> upstream fixes.
>>
> 
> I still fail to see how that introduces a regression.
> 
> Let's take a src string that is larger or equal to maxlen.
> 
> Before the patch:
> len = strnlen(src, maxlen); -> len = maxlen;
> len++; len = maxlen + 1;
> dst = kmalloc(len, GPF_KERNEL); /* allocates a maxlen+1 sized dst */
> if (!dst)
> 	return NULL;
> strlcpy(dst, src, len); // where I replace len/size with maxlen+1 from now on
> 	size_t ret = strlen(src); // ret >= maxlen
> 	if (maxlen + 1) {
> 		size_t len = (ret >= maxlen + 1) ? maxlen + 1 - 1 : ret;
> 		/* here, either ret == maxlen, which makes len = ret = maxlen;
> 		   or ret > maxlen, which makes len = maxlen + 1 - 1 = maxlen */
> 		memcpy(dest, src, maxlen);
> 		dest[maxlen] = '\0';
> 	}
> 
> After the patch:
> dst = kstrndup(src, maxlen, GFP_KERNEL);
> 	...
> 	len = strnlen(s, max); -> len = max;
> 	buf = kmalloc_track_caller(len+1, gfp); /* again, allocates maxlen+1 */
> 	/* from here on, replaces len with maxlen */
> 	if (buf) {
> 		memcpy(buf, s, maxlen);
> 		dest[maxlen] = '\0';
> 	}
> 	return buf; /* here, returns NULL as before the patch, in case allocation fails */
> 
> 
> By looking at the bug, it looks like the failure happens when nls_load fails to
> load nls_utf8 module. So, perhaps, the user missed the installation of
> linux-modules-extra when installing from the archive, and then installed the
> package when instructed to install the test kernel?
> 
> Cascardo.
> 

Cascardo makes a fair point. Lets wait until we get a response from the 
reporter before we get all aggro on this patch. I've also had a response 
from one of the upstream patch reviewers. He is skeptical that this 
patch could have anything to do with a mount failure.

If necessary I'll resubmit a v2 with the full list of affected kernels.

rtg
diff mbox series

Patch

diff --git a/fs/cifs/cifs_unicode.c b/fs/cifs/cifs_unicode.c
index 171ad8b42107e..9bd03a2310328 100644
--- a/fs/cifs/cifs_unicode.c
+++ b/fs/cifs/cifs_unicode.c
@@ -358,9 +358,14 @@  cifs_strndup_from_utf16(const char *src, const int maxlen,
 		if (!dst)
 			return NULL;
 		cifs_from_utf16(dst, (__le16 *) src, len, maxlen, codepage,
-				NO_MAP_UNI_RSVD);
+			       NO_MAP_UNI_RSVD);
 	} else {
-		dst = kstrndup(src, maxlen, GFP_KERNEL);
+		len = strnlen(src, maxlen);
+		len++;
+		dst = kmalloc(len, GFP_KERNEL);
+		if (!dst)
+			return NULL;
+		strlcpy(dst, src, len);
 	}
 
 	return dst;