| Message ID | f19be994-d355-48a6-ab45-d0f7e5955daf@stanley.mountain (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [next] firmware: thead,th1520-aon: Fix use after free in th1520_aon_init() | expand |
| Context | Check | Description |
|---|---|---|
| bjorn/pre-ci_am | fail | Failed to apply series |
> Record the error code before freeing "aon_chan" to avoid a > use after free. Would it become helpful to mention which selection of source code analysis tools detected such a questionable implementation detail? … > +++ b/drivers/firmware/thead,th1520-aon.c > @@ -203,6 +203,7 @@ struct th1520_aon_chan *th1520_aon_init(struct device *dev) > { > struct th1520_aon_chan *aon_chan; > struct mbox_client *cl; > + int ret; > > aon_chan = kzalloc(sizeof(*aon_chan), GFP_KERNEL); > if (!aon_chan) > @@ -217,8 +218,9 @@ struct th1520_aon_chan *th1520_aon_init(struct device *dev) > aon_chan->ch = mbox_request_channel_byname(cl, "aon"); > if (IS_ERR(aon_chan->ch)) { > dev_err(dev, "Failed to request aon mbox chan\n"); > + ret = PTR_ERR(aon_chan->ch); > kfree(aon_chan); > - return ERR_CAST(aon_chan->ch); > + return ERR_PTR(ret); > } > > mutex_init(&aon_chan->transaction_lock); May the additional variable (for an information) be defined only for the affected if branch? Would a smaller scope be more appropriate here? Regards, Markus
On Sun, Mar 16, 2025 at 01:05:08PM +0100, Markus Elfring wrote: > > +++ b/drivers/firmware/thead,th1520-aon.c > > @@ -203,6 +203,7 @@ struct th1520_aon_chan *th1520_aon_init(struct device *dev) > > { > > struct th1520_aon_chan *aon_chan; > > struct mbox_client *cl; > > + int ret; > > > > aon_chan = kzalloc(sizeof(*aon_chan), GFP_KERNEL); > > if (!aon_chan) > > @@ -217,8 +218,9 @@ struct th1520_aon_chan *th1520_aon_init(struct device *dev) > > aon_chan->ch = mbox_request_channel_byname(cl, "aon"); > > if (IS_ERR(aon_chan->ch)) { > > dev_err(dev, "Failed to request aon mbox chan\n"); > > + ret = PTR_ERR(aon_chan->ch); > > kfree(aon_chan); > > - return ERR_CAST(aon_chan->ch); > > + return ERR_PTR(ret); > > } > > > > mutex_init(&aon_chan->transaction_lock); > > May the additional variable (for an information) be defined only for > the affected if branch? > Would a smaller scope be more appropriate here? There are some variables which should always be at function scope and "int ret" is one of those. regards, dan carpenter
On Sat, 15 Mar 2025 at 11:04, Dan Carpenter <dan.carpenter@linaro.org> wrote: > > Record the error code before freeing "aon_chan" to avoid a > use after free. > > Fixes: e4b3cbd840e5 ("firmware: thead: Add AON firmware protocol driver") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> Applied for next, thanks! Kind regards Uffe > --- > drivers/firmware/thead,th1520-aon.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/firmware/thead,th1520-aon.c b/drivers/firmware/thead,th1520-aon.c > index 4416e9bbf854..38f812ac9920 100644 > --- a/drivers/firmware/thead,th1520-aon.c > +++ b/drivers/firmware/thead,th1520-aon.c > @@ -203,6 +203,7 @@ struct th1520_aon_chan *th1520_aon_init(struct device *dev) > { > struct th1520_aon_chan *aon_chan; > struct mbox_client *cl; > + int ret; > > aon_chan = kzalloc(sizeof(*aon_chan), GFP_KERNEL); > if (!aon_chan) > @@ -217,8 +218,9 @@ struct th1520_aon_chan *th1520_aon_init(struct device *dev) > aon_chan->ch = mbox_request_channel_byname(cl, "aon"); > if (IS_ERR(aon_chan->ch)) { > dev_err(dev, "Failed to request aon mbox chan\n"); > + ret = PTR_ERR(aon_chan->ch); > kfree(aon_chan); > - return ERR_CAST(aon_chan->ch); > + return ERR_PTR(ret); > } > > mutex_init(&aon_chan->transaction_lock); > -- > 2.47.2 >
diff --git a/drivers/firmware/thead,th1520-aon.c b/drivers/firmware/thead,th1520-aon.c index 4416e9bbf854..38f812ac9920 100644 --- a/drivers/firmware/thead,th1520-aon.c +++ b/drivers/firmware/thead,th1520-aon.c @@ -203,6 +203,7 @@ struct th1520_aon_chan *th1520_aon_init(struct device *dev) { struct th1520_aon_chan *aon_chan; struct mbox_client *cl; + int ret; aon_chan = kzalloc(sizeof(*aon_chan), GFP_KERNEL); if (!aon_chan) @@ -217,8 +218,9 @@ struct th1520_aon_chan *th1520_aon_init(struct device *dev) aon_chan->ch = mbox_request_channel_byname(cl, "aon"); if (IS_ERR(aon_chan->ch)) { dev_err(dev, "Failed to request aon mbox chan\n"); + ret = PTR_ERR(aon_chan->ch); kfree(aon_chan); - return ERR_CAST(aon_chan->ch); + return ERR_PTR(ret); } mutex_init(&aon_chan->transaction_lock);
Record the error code before freeing "aon_chan" to avoid a use after free. Fixes: e4b3cbd840e5 ("firmware: thead: Add AON firmware protocol driver") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- drivers/firmware/thead,th1520-aon.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)