Re: [PATCH 006 of 8] knfsd: nfsd4: fslocations data structures

[Andrew Morton <akpm@osdl.org>]

On Fri, 29 Sep 2006 13:09:13 +1000
NeilBrown <neilb@suse.de> wrote:

> 
> From: Manoj Naik <manoj@almaden.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.
> 
> ...
>  
> +#ifdef CONFIG_NFSD_V4
> +
> +static int
> +fsloc_parse(char **mesg, char *buf, struct nfsd4_fs_locations *fsloc)
> +{
> +	int len;
> +	int migrated, i, err;
> +
> +	len = qword_get(mesg, buf, PAGE_SIZE);
> +	if (len != 5 || memcmp(buf, "fsloc", 5))
> +		return 0;
> +
> +	/* listsize */
> +	err = get_int(mesg, &fsloc->locations_count);
> +	if (err)
> +		return err;
> +	if (fsloc->locations_count < 0)

this is unsigned.

> +		return -EINVAL;
> +	if (fsloc->locations_count == 0)
> +		return 0;
> +
> +	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..


> +	if (!fsloc->locations)
> +		return -ENOMEM;
> +	for (i=0; i < fsloc->locations_count; i++) {
> +		/* colon separated host list */
> +		err = -EINVAL;
> +		len = qword_get(mesg, buf, PAGE_SIZE);
> +		if (len <= 0)
> +			goto out_free_all;
> +		err = -ENOMEM;
> +		fsloc->locations[i].hosts = kstrdup(buf, GFP_KERNEL);
> +		if (!fsloc->locations[i].hosts)
> +			goto out_free_all;
> +		err = -EINVAL;
> +		/* slash separated path component list */
> +		len = qword_get(mesg, buf, PAGE_SIZE);
> +		if (len <= 0)
> +			goto out_free_all;
> +		err = -ENOMEM;
> +		fsloc->locations[i].path = kstrdup(buf, GFP_KERNEL);
> +		if (!fsloc->locations[i].path)
> +			goto out_free_all;
> +	}
> +	/* migrated */
> +	err = get_int(mesg, &migrated);
> +	if (err)
> +		goto out_free_all;
> +	err = -EINVAL;
> +	if (migrated < 0 || migrated > 1)
> +		goto out_free_all;
> +	fsloc->migrated = migrated;
> +	return 0;
> +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?

> +	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..