linux-kernel - Re: [NFS] [PATCH 006 of 8] knfsd: nfsd4: fslocations data structures

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: <20061002182327.GB8084@fieldses.org>
Date:	Mon, 2 Oct 2006 14:23:27 -0400
From:	"J. Bruce Fields" <bfields@...ldses.org>
To:	Andrew Morton <akpm@...l.org>
Cc:	NeilBrown <neilb@...e.de>, nfs@...ts.sourceforge.net,
	linux-kernel@...r.kernel.org
Subject: Re: [NFS] [PATCH 006 of 8] knfsd: nfsd4: fslocations data structures

On Thu, Sep 28, 2006 at 11:45:40PM -0700, Andrew Morton wrote:
> On Fri, 29 Sep 2006 13:09:13 +1000
> NeilBrown <neilb@...e.de> wrote:
> 
> > 
> > From: Manoj Naik <manoj@...aden.ibm.com>
> > 
> > Define FS locations structures, some functions to manipulate them, and add
> > code to parse FS locations in downcall and add to the exports structure.
...
> > +	if (fsloc->locations_count < 0)
> 
> this is unsigned.

Yes, thanks.

> > +	fsloc->locations = kzalloc(fsloc->locations_count
> > +			* sizeof(struct nfsd4_fs_location), GFP_KERNEL);
> 
> This is subject to multiplication overflow.  If it's a privileged operation
> and isn't dependent on stuff coming in over the wire then ok..

This is data provided locally by root.  But there's no reason to allow
this to be arbitrarily large, and I'd still prefer to have some
sanity-checking, so I'll replace the bogus "<0" check above by a
check for something "too large".

> > +out_free_all:
> > +	nfsd4_fslocs_free(fsloc);
> 
> This call to nfsd4_fslocs_free() can end up kfreeing members of
> fsloc->locations[] which haven't been initialised here.  Are we sure the
> caller set them all to zero?

Yes.  There will only ever be one caller, and it has to initialize
these to zero.  I agree that this could be more obvious....

> > +	return err;
> > +}
> > +
> > +#else /* CONFIG_NFSD_V4 */
> > +static int fsloc_parse(char **, char *, struct svc_export *) { return 0; }
> 
> static inline
> 
> This has a different prototype from the other version of fsloc_parse()
> 
> This ain't C++ - function arguments need identifiers as well as types.
> 
> Someone needs to read Documentation/SubmitChecklist..

That would be me, sorry, yes.

Bryce Harrington is also setting up automatic compile-testing for us
with the obvious config options turned on and off, so hopefully that
will help save me from myself....

Patches addressing the above follow.

--b.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/