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

[Tim Gardner]

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
-- 
-----------
Tim Gardner
Canonical, Inc