Re: [PATCH v2] net: 9p: avoid freeing uninit memory in p9pdu_vreadf

[Fedor Pchelkin]

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@xxxxxxxxx>
> > ---
> > 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