netdev - Re: [PATCH v2] net: 9p: avoid freeing uninit memory in p9pdu

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2974507b-57fa-4c9b-a036-055dbf55f6a4-pchelkin@ispras.ru>
Date: Tue, 5 Dec 2023 15:15:43 +0300
From: Fedor Pchelkin <pchelkin@...ras.ru>
To: Dominique Martinet <asmadeus@...ewreck.org>
Cc: Latchesar Ionkov <lucho@...kov.net>, 
	Eric Van Hensbergen <ericvh@...nel.org>, Christian Schoenebeck <linux_oss@...debyte.com>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, v9fs@...ts.linux.dev, 
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Alexey Khoroshilov <khoroshilov@...ras.ru>, lvc-project@...uxtesting.org
Subject: Re: [PATCH v2] net: 9p: avoid freeing uninit memory in p9pdu_vreadf

On 23/12/05 06:31PM, Dominique Martinet wrote:
> Fedor Pchelkin wrote on Tue, Dec 05, 2023 at 12:19:50PM +0300:
> > If an error occurs while processing an array of strings in p9pdu_vreadf
> > then uninitialized members of *wnames array are freed.
> > 
> > Fix this by iterating over only lower indices of the array. Also handle
> > possible uninit *wnames usage if first p9pdu_readf() call inside 'T' case
> > fails.
> > 
> > Found by Linux Verification Center (linuxtesting.org).
> > 
> > Fixes: ace51c4dd2f9 ("9p: add new protocol support code")
> > Signed-off-by: Fedor Pchelkin <pchelkin@...ras.ru>
> > ---
> > v2: I've missed that *wnames can also be left uninitialized. Please
> > ignore the patch v1.
> 
> While I agree it's good to initialize it in general, how is that a
> problem here? Do we have users that'd ignore the return code and try to
> use *wnames?
> (The first initialization is required in case the first p9pdu_readf
> fails and *wnames had a non-null initial value, but the second is
> unrelated)
> 

My initial concern was just about the statement you wrote in parenthesis.
Case 'T' can be provided with non-null initial *wnames value, and if the
first p9pdu_readf() call there fails then *wnames is invalidly freed in
error handling path here:

case 'T':{
	[...]
	if (errcode) {
		if (*wnames) {
			int i;

			for (i = 0; i < *nwname; i++)
				kfree((*wnames)[i]);
		}
		kfree(*wnames);
		*wnames = NULL;
	}

So the first initialization is required to prevent the described error.

As for the second initialization (the one located after kfree(*wnames) in
error handling path - it was there all the time), I think it's better not
to touch it. I've just moved kfree and null-assignment under
'if (*wnames)' statement.

The concern you mentioned is about any user that'd ignore the return code
and try to use *wnames (so that the second initialization makes some
sense). I can't see if there is any such user but, as said before, it's
better not to touch that code.

> I don't mind the change even if there isn't but let's add a word in the
> commit message.
> 

OK, will do in v3.

> > As an answer to Dominique's comment: my organization marks this
> > statement in all commits.
> 
> Fair enough, I think you'd get more internet points with a 'Reported-by'
> but I see plenty of such messages in old commits and this isn't
> something I want to argue about -- ok.
> 
> -- 
> Dominique Martinet | Asmadeus