Messages in this thread | | | Date | Thu, 28 Sep 2006 23:45:40 -0700 | From | Andrew Morton <> | Subject | Re: [PATCH 006 of 8] knfsd: nfsd4: fslocations data structures |
| |
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..
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |