* [patch] mfd: vexpress: use after free in vexpress_syscfg_regmap_init() @ 2014-06-11 6:07 Dan Carpenter 2014-06-11 9:22 ` Pawel Moll 0 siblings, 1 reply; 8+ messages in thread From: Dan Carpenter @ 2014-06-11 6:07 UTC (permalink / raw) To: Arnd Bergmann, Pawel Moll, Lee Jones Cc: Greg Kroah-Hartman, Samuel Ortiz, linux-kernel, kernel-janitors We should return NULL if regmap_init() fails instead of continuing. Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> diff --git a/drivers/misc/vexpress-syscfg.c b/drivers/misc/vexpress-syscfg.c index 73068e5..2c0ddb2 100644 --- a/drivers/misc/vexpress-syscfg.c +++ b/drivers/misc/vexpress-syscfg.c @@ -231,10 +231,12 @@ static struct regmap *vexpress_syscfg_regmap_init(struct device *dev, func->regmap = regmap_init(dev, NULL, func, &vexpress_syscfg_regmap_config); - if (IS_ERR(func->regmap)) + if (IS_ERR(func->regmap)) { kfree(func); - else - list_add(&func->list, &syscfg->funcs); + return NULL; + } + + list_add(&func->list, &syscfg->funcs); return func->regmap; } ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [patch] mfd: vexpress: use after free in vexpress_syscfg_regmap_init() 2014-06-11 6:07 [patch] mfd: vexpress: use after free in vexpress_syscfg_regmap_init() Dan Carpenter @ 2014-06-11 9:22 ` Pawel Moll 2014-06-11 10:07 ` Dan Carpenter 2014-06-11 10:17 ` [patch v2] mfd: vexpress: fix error handling vexpress_syscfg_regmap_init() Dan Carpenter 0 siblings, 2 replies; 8+ messages in thread From: Pawel Moll @ 2014-06-11 9:22 UTC (permalink / raw) To: Dan Carpenter Cc: Arnd Bergmann, Lee Jones, Greg Kroah-Hartman, Samuel Ortiz, linux-kernel, kernel-janitors On Wed, 2014-06-11 at 07:07 +0100, Dan Carpenter wrote: > We should return NULL if regmap_init() fails instead of continuing. > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > diff --git a/drivers/misc/vexpress-syscfg.c b/drivers/misc/vexpress-syscfg.c > index 73068e5..2c0ddb2 100644 > --- a/drivers/misc/vexpress-syscfg.c > +++ b/drivers/misc/vexpress-syscfg.c > @@ -231,10 +231,12 @@ static struct regmap *vexpress_syscfg_regmap_init(struct device *dev, > func->regmap = regmap_init(dev, NULL, func, > &vexpress_syscfg_regmap_config); > > - if (IS_ERR(func->regmap)) > + if (IS_ERR(func->regmap)) { > kfree(func); > - else > - list_add(&func->list, &syscfg->funcs); > + return NULL; > + } > + > + list_add(&func->list, &syscfg->funcs); > > return func->regmap; > } Not really, no. What made you think so? vexpress_config_bridge_ops.regmap_init should return an ERR_PTR in case of troubles, not a NULL. See devm_regmap_init_vexpress_config() in drivers/bus/vexpress-config.c: regmap = bridge->ops->regmap_init(dev, bridge->context); if (IS_ERR(regmap)) { Pawel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch] mfd: vexpress: use after free in vexpress_syscfg_regmap_init() 2014-06-11 9:22 ` Pawel Moll @ 2014-06-11 10:07 ` Dan Carpenter 2014-06-11 10:17 ` [patch v2] mfd: vexpress: fix error handling vexpress_syscfg_regmap_init() Dan Carpenter 1 sibling, 0 replies; 8+ messages in thread From: Dan Carpenter @ 2014-06-11 10:07 UTC (permalink / raw) To: Pawel Moll Cc: Arnd Bergmann, Lee Jones, Greg Kroah-Hartman, Samuel Ortiz, linux-kernel, kernel-janitors On Wed, Jun 11, 2014 at 10:22:22AM +0100, Pawel Moll wrote: > On Wed, 2014-06-11 at 07:07 +0100, Dan Carpenter wrote: > > We should return NULL if regmap_init() fails instead of continuing. > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > diff --git a/drivers/misc/vexpress-syscfg.c b/drivers/misc/vexpress-syscfg.c > > index 73068e5..2c0ddb2 100644 > > --- a/drivers/misc/vexpress-syscfg.c > > +++ b/drivers/misc/vexpress-syscfg.c > > @@ -231,10 +231,12 @@ static struct regmap *vexpress_syscfg_regmap_init(struct device *dev, > > func->regmap = regmap_init(dev, NULL, func, > > &vexpress_syscfg_regmap_config); > > > > - if (IS_ERR(func->regmap)) > > + if (IS_ERR(func->regmap)) { > > kfree(func); > > - else > > - list_add(&func->list, &syscfg->funcs); > > + return NULL; > > + } > > + > > + list_add(&func->list, &syscfg->funcs); > > > > return func->regmap; > > } > > Not really, no. What made you think so? > > vexpress_config_bridge_ops.regmap_init should return an ERR_PTR in case > of troubles, not a NULL. See devm_regmap_init_vexpress_config() in > drivers/bus/vexpress-config.c: > > regmap = bridge->ops->regmap_init(dev, bridge->context); > if (IS_ERR(regmap)) { Ah... That was sloppy of me. There is a similar bug earlier and I'll fix that as well. regards, dan carpenter ^ permalink raw reply [flat|nested] 8+ messages in thread
* [patch v2] mfd: vexpress: fix error handling vexpress_syscfg_regmap_init() 2014-06-11 9:22 ` Pawel Moll 2014-06-11 10:07 ` Dan Carpenter @ 2014-06-11 10:17 ` Dan Carpenter 2014-06-11 10:33 ` Pawel Moll 1 sibling, 1 reply; 8+ messages in thread From: Dan Carpenter @ 2014-06-11 10:17 UTC (permalink / raw) To: Arnd Bergmann, Pawel Moll Cc: Greg Kroah-Hartman, Lee Jones, Samuel Ortiz, linux-kernel, kernel-janitors This function should be returning an ERR_PTR() on failure instead of NULL. Also there is a use after free bug if regmap_init() fails because we free "func" and then dereference doing the return. Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> diff --git a/drivers/misc/vexpress-syscfg.c b/drivers/misc/vexpress-syscfg.c index 73068e5..3250fc1 100644 --- a/drivers/misc/vexpress-syscfg.c +++ b/drivers/misc/vexpress-syscfg.c @@ -199,7 +199,7 @@ static struct regmap *vexpress_syscfg_regmap_init(struct device *dev, func = kzalloc(sizeof(*func) + sizeof(*func->template) * num, GFP_KERNEL); if (!func) - return NULL; + return ERR_PTR(-ENOMEM); func->syscfg = syscfg; func->num_templates = num; @@ -231,10 +231,14 @@ static struct regmap *vexpress_syscfg_regmap_init(struct device *dev, func->regmap = regmap_init(dev, NULL, func, &vexpress_syscfg_regmap_config); - if (IS_ERR(func->regmap)) + if (IS_ERR(func->regmap)) { + void *err = func->regmap; + kfree(func); - else - list_add(&func->list, &syscfg->funcs); + return err; + } + + list_add(&func->list, &syscfg->funcs); return func->regmap; } ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [patch v2] mfd: vexpress: fix error handling vexpress_syscfg_regmap_init() 2014-06-11 10:17 ` [patch v2] mfd: vexpress: fix error handling vexpress_syscfg_regmap_init() Dan Carpenter @ 2014-06-11 10:33 ` Pawel Moll 2014-06-11 17:12 ` Greg Kroah-Hartman 2014-06-13 14:02 ` Arnd Bergmann 0 siblings, 2 replies; 8+ messages in thread From: Pawel Moll @ 2014-06-11 10:33 UTC (permalink / raw) To: Dan Carpenter, Arnd Bergmann, Olof Johansson Cc: Greg Kroah-Hartman, Lee Jones, Samuel Ortiz, linux-kernel, kernel-janitors On Wed, 2014-06-11 at 11:17 +0100, Dan Carpenter wrote: > This function should be returning an ERR_PTR() on failure instead of > NULL. Also there is a use after free bug if regmap_init() fails because > we free "func" and then dereference doing the return. > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > diff --git a/drivers/misc/vexpress-syscfg.c b/drivers/misc/vexpress-syscfg.c > index 73068e5..3250fc1 100644 > --- a/drivers/misc/vexpress-syscfg.c > +++ b/drivers/misc/vexpress-syscfg.c > @@ -199,7 +199,7 @@ static struct regmap *vexpress_syscfg_regmap_init(struct device *dev, > func = kzalloc(sizeof(*func) + sizeof(*func->template) * num, > GFP_KERNEL); > if (!func) > - return NULL; > + return ERR_PTR(-ENOMEM); > > func->syscfg = syscfg; > func->num_templates = num; > @@ -231,10 +231,14 @@ static struct regmap *vexpress_syscfg_regmap_init(struct device *dev, > func->regmap = regmap_init(dev, NULL, func, > &vexpress_syscfg_regmap_config); > > - if (IS_ERR(func->regmap)) > + if (IS_ERR(func->regmap)) { > + void *err = func->regmap; > + > kfree(func); > - else > - list_add(&func->list, &syscfg->funcs); > + return err; > + } > + > + list_add(&func->list, &syscfg->funcs); > > return func->regmap; > } Uh, right. Dereferencing a freed structure. My bad. Thanks for spotting this! Acked-by: Pawel Moll <pawel.moll@arm.com> (nit: the subject should be "misc: vexpress:" rather then "mfd:") Arnd, Olof, can you pick this one as an early fix or do you want me to queue it for rc1-based fixes branch? Paweł ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch v2] mfd: vexpress: fix error handling vexpress_syscfg_regmap_init() 2014-06-11 10:33 ` Pawel Moll @ 2014-06-11 17:12 ` Greg Kroah-Hartman 2014-06-11 17:11 ` Pawel Moll 2014-06-13 14:02 ` Arnd Bergmann 1 sibling, 1 reply; 8+ messages in thread From: Greg Kroah-Hartman @ 2014-06-11 17:12 UTC (permalink / raw) To: Pawel Moll Cc: Dan Carpenter, Arnd Bergmann, Olof Johansson, Lee Jones, Samuel Ortiz, linux-kernel, kernel-janitors On Wed, Jun 11, 2014 at 11:33:20AM +0100, Pawel Moll wrote: > On Wed, 2014-06-11 at 11:17 +0100, Dan Carpenter wrote: > > This function should be returning an ERR_PTR() on failure instead of > > NULL. Also there is a use after free bug if regmap_init() fails because > > we free "func" and then dereference doing the return. > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > diff --git a/drivers/misc/vexpress-syscfg.c b/drivers/misc/vexpress-syscfg.c > > index 73068e5..3250fc1 100644 > > --- a/drivers/misc/vexpress-syscfg.c > > +++ b/drivers/misc/vexpress-syscfg.c > > @@ -199,7 +199,7 @@ static struct regmap *vexpress_syscfg_regmap_init(struct device *dev, > > func = kzalloc(sizeof(*func) + sizeof(*func->template) * num, > > GFP_KERNEL); > > if (!func) > > - return NULL; > > + return ERR_PTR(-ENOMEM); > > > > func->syscfg = syscfg; > > func->num_templates = num; > > @@ -231,10 +231,14 @@ static struct regmap *vexpress_syscfg_regmap_init(struct device *dev, > > func->regmap = regmap_init(dev, NULL, func, > > &vexpress_syscfg_regmap_config); > > > > - if (IS_ERR(func->regmap)) > > + if (IS_ERR(func->regmap)) { > > + void *err = func->regmap; > > + > > kfree(func); > > - else > > - list_add(&func->list, &syscfg->funcs); > > + return err; > > + } > > + > > + list_add(&func->list, &syscfg->funcs); > > > > return func->regmap; > > } > > Uh, right. Dereferencing a freed structure. My bad. Thanks for spotting > this! > > Acked-by: Pawel Moll <pawel.moll@arm.com> > > (nit: the subject should be "misc: vexpress:" rather then "mfd:") > > Arnd, Olof, can you pick this one as an early fix or do you want me to > queue it for rc1-based fixes branch? > I can queue it up. thanks, greg k-h ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch v2] mfd: vexpress: fix error handling vexpress_syscfg_regmap_init() 2014-06-11 17:12 ` Greg Kroah-Hartman @ 2014-06-11 17:11 ` Pawel Moll 0 siblings, 0 replies; 8+ messages in thread From: Pawel Moll @ 2014-06-11 17:11 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Dan Carpenter, Arnd Bergmann, Olof Johansson, Lee Jones, Samuel Ortiz, linux-kernel, kernel-janitors On Wed, 2014-06-11 at 18:12 +0100, Greg Kroah-Hartman wrote: > I can queue it up. Cool, thanks! Pawel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch v2] mfd: vexpress: fix error handling vexpress_syscfg_regmap_init() 2014-06-11 10:33 ` Pawel Moll 2014-06-11 17:12 ` Greg Kroah-Hartman @ 2014-06-13 14:02 ` Arnd Bergmann 1 sibling, 0 replies; 8+ messages in thread From: Arnd Bergmann @ 2014-06-13 14:02 UTC (permalink / raw) To: Pawel Moll Cc: Dan Carpenter, Olof Johansson, Greg Kroah-Hartman, Lee Jones, Samuel Ortiz, linux-kernel, kernel-janitors On Wednesday 11 June 2014, Pawel Moll wrote: > On Wed, 2014-06-11 at 11:17 +0100, Dan Carpenter wrote: > > This function should be returning an ERR_PTR() on failure instead of > > NULL. Also there is a use after free bug if regmap_init() fails because > > we free "func" and then dereference doing the return. > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > diff --git a/drivers/misc/vexpress-syscfg.c b/drivers/misc/vexpress-syscfg.c > > index 73068e5..3250fc1 100644 > > --- a/drivers/misc/vexpress-syscfg.c > > +++ b/drivers/misc/vexpress-syscfg.c > > @@ -199,7 +199,7 @@ static struct regmap *vexpress_syscfg_regmap_init(struct device *dev, > > func = kzalloc(sizeof(*func) + sizeof(*func->template) * num, > > GFP_KERNEL); > > if (!func) > > - return NULL; > > + return ERR_PTR(-ENOMEM); > > > > func->syscfg = syscfg; > > func->num_templates = num; > > @@ -231,10 +231,14 @@ static struct regmap *vexpress_syscfg_regmap_init(struct device *dev, > > func->regmap = regmap_init(dev, NULL, func, > > &vexpress_syscfg_regmap_config); > > > > - if (IS_ERR(func->regmap)) > > + if (IS_ERR(func->regmap)) { > > + void *err = func->regmap; > > + > > kfree(func); > > - else > > - list_add(&func->list, &syscfg->funcs); > > + return err; > > + } > > + > > + list_add(&func->list, &syscfg->funcs); > > > > return func->regmap; > > } > > Uh, right. Dereferencing a freed structure. My bad. Thanks for spotting > this! > > Acked-by: Pawel Moll <pawel.moll@arm.com> > > (nit: the subject should be "misc: vexpress:" rather then "mfd:") > > Arnd, Olof, can you pick this one as an early fix or do you want me to > queue it for rc1-based fixes branch? I've applied it to the fixes branch now. Thanks! Arnd ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-06-13 14:03 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-06-11 6:07 [patch] mfd: vexpress: use after free in vexpress_syscfg_regmap_init() Dan Carpenter 2014-06-11 9:22 ` Pawel Moll 2014-06-11 10:07 ` Dan Carpenter 2014-06-11 10:17 ` [patch v2] mfd: vexpress: fix error handling vexpress_syscfg_regmap_init() Dan Carpenter 2014-06-11 10:33 ` Pawel Moll 2014-06-11 17:12 ` Greg Kroah-Hartman 2014-06-11 17:11 ` Pawel Moll 2014-06-13 14:02 ` Arnd Bergmann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).