| Message ID | 20200926212734.23836-2-zev@bewilderbeest.net |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | PECI patchset tweaks | expand |
Hello Zev, On 9/26/2020 2:27 PM, Zev Weiss wrote: > peci_get_xfer_msg() returns NULL on failure, not an ERR_PTR. Also > avoid calling kfree() on an ERR_PTR. > > Signed-off-by: Zev Weiss <zev@bewilderbeest.net> > --- > drivers/peci/peci-dev.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/peci/peci-dev.c b/drivers/peci/peci-dev.c > index e0fe09467a80..84e90af81ccc 100644 > --- a/drivers/peci/peci-dev.c > +++ b/drivers/peci/peci-dev.c > @@ -122,8 +122,8 @@ static long peci_dev_ioctl(struct file *file, uint iocmd, ulong arg) > } > > xmsg = peci_get_xfer_msg(uxmsg.tx_len, uxmsg.rx_len); > - if (IS_ERR(xmsg)) { > - ret = PTR_ERR(xmsg); > + if (!xmsg) { > + ret = -ENOMEM; Yes, it's a right fix. Thanks! > break; > } > > @@ -162,7 +162,8 @@ static long peci_dev_ioctl(struct file *file, uint iocmd, ulong arg) > } > > peci_put_xfer_msg(xmsg); > - kfree(msg); > + if (!IS_ERR(msg)) > + kfree(msg); Not needed. kfree itself has null pointer checking inside. > > return (long)ret; > } >
On Mon, Sep 28, 2020 at 02:03:12PM CDT, Jae Hyun Yoo wrote: >Hello Zev, > >On 9/26/2020 2:27 PM, Zev Weiss wrote: >>peci_get_xfer_msg() returns NULL on failure, not an ERR_PTR. Also >>avoid calling kfree() on an ERR_PTR. >> >>Signed-off-by: Zev Weiss <zev@bewilderbeest.net> >>--- >> drivers/peci/peci-dev.c | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >>diff --git a/drivers/peci/peci-dev.c b/drivers/peci/peci-dev.c >>index e0fe09467a80..84e90af81ccc 100644 >>--- a/drivers/peci/peci-dev.c >>+++ b/drivers/peci/peci-dev.c >>@@ -122,8 +122,8 @@ static long peci_dev_ioctl(struct file *file, uint iocmd, ulong arg) >> } >> xmsg = peci_get_xfer_msg(uxmsg.tx_len, uxmsg.rx_len); >>- if (IS_ERR(xmsg)) { >>- ret = PTR_ERR(xmsg); >>+ if (!xmsg) { >>+ ret = -ENOMEM; > >Yes, it's a right fix. Thanks! > >> break; >> } >>@@ -162,7 +162,8 @@ static long peci_dev_ioctl(struct file *file, uint iocmd, ulong arg) >> } >> peci_put_xfer_msg(xmsg); >>- kfree(msg); >>+ if (!IS_ERR(msg)) >>+ kfree(msg); > >Not needed. kfree itself has null pointer checking inside. > Certainly, but the condition in question here isn't whether it's NULL, but whether it's an ERR_PTR (which as far as I can tell kfree() does not check for). As is, there's an error path that leads to passing a non-NULL but also non-kfree-safe ERR_PTR (the memdup_user() return value). Zev
On 9/28/2020 12:09 PM, Zev Weiss wrote: > On Mon, Sep 28, 2020 at 02:03:12PM CDT, Jae Hyun Yoo wrote: >> Hello Zev, >> >> On 9/26/2020 2:27 PM, Zev Weiss wrote: >>> peci_get_xfer_msg() returns NULL on failure, not an ERR_PTR. Also >>> avoid calling kfree() on an ERR_PTR. >>> >>> Signed-off-by: Zev Weiss <zev@bewilderbeest.net> >>> --- >>> drivers/peci/peci-dev.c | 7 ++++--- >>> 1 file changed, 4 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/peci/peci-dev.c b/drivers/peci/peci-dev.c >>> index e0fe09467a80..84e90af81ccc 100644 >>> --- a/drivers/peci/peci-dev.c >>> +++ b/drivers/peci/peci-dev.c >>> @@ -122,8 +122,8 @@ static long peci_dev_ioctl(struct file *file, >>> uint iocmd, ulong arg) >>> } >>> xmsg = peci_get_xfer_msg(uxmsg.tx_len, uxmsg.rx_len); >>> - if (IS_ERR(xmsg)) { >>> - ret = PTR_ERR(xmsg); >>> + if (!xmsg) { >>> + ret = -ENOMEM; >> >> Yes, it's a right fix. Thanks! >> >>> break; >>> } >>> @@ -162,7 +162,8 @@ static long peci_dev_ioctl(struct file *file, >>> uint iocmd, ulong arg) >>> } >>> peci_put_xfer_msg(xmsg); >>> - kfree(msg); >>> + if (!IS_ERR(msg)) >>> + kfree(msg); >> >> Not needed. kfree itself has null pointer checking inside. >> > > Certainly, but the condition in question here isn't whether it's NULL, > but whether it's an ERR_PTR (which as far as I can tell kfree() does not > check for). As is, there's an error path that leads to passing a > non-NULL but also non-kfree-safe ERR_PTR (the memdup_user() return value). Yeah, checked that memdup_user can also return ERR_PTR(-ENOMEM) or ERR_PTR(-EFAULT) in error cases so null checking isn't enough for calling free(). I'll add this change into PECI patch set. Thanks! Reviewed-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> > > > Zev >
On Sat, 26 Sep 2020 at 21:27, Zev Weiss <zev@bewilderbeest.net> wrote: > > peci_get_xfer_msg() returns NULL on failure, not an ERR_PTR. Also > avoid calling kfree() on an ERR_PTR. > > Signed-off-by: Zev Weiss <zev@bewilderbeest.net> Fixes: 90ddc4e972b5 ("peci: Add support for PECI bus driver core") Reviewed-by: Joel Stanley <joel@jms.id.au> Applied to dev-5.8. Cheers, Joel > --- > drivers/peci/peci-dev.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/peci/peci-dev.c b/drivers/peci/peci-dev.c > index e0fe09467a80..84e90af81ccc 100644 > --- a/drivers/peci/peci-dev.c > +++ b/drivers/peci/peci-dev.c > @@ -122,8 +122,8 @@ static long peci_dev_ioctl(struct file *file, uint iocmd, ulong arg) > } > > xmsg = peci_get_xfer_msg(uxmsg.tx_len, uxmsg.rx_len); > - if (IS_ERR(xmsg)) { > - ret = PTR_ERR(xmsg); > + if (!xmsg) { > + ret = -ENOMEM; > break; > } > > @@ -162,7 +162,8 @@ static long peci_dev_ioctl(struct file *file, uint iocmd, ulong arg) > } > > peci_put_xfer_msg(xmsg); > - kfree(msg); > + if (!IS_ERR(msg)) > + kfree(msg); > > return (long)ret; > } > -- > 2.28.0 >
diff --git a/drivers/peci/peci-dev.c b/drivers/peci/peci-dev.c index e0fe09467a80..84e90af81ccc 100644 --- a/drivers/peci/peci-dev.c +++ b/drivers/peci/peci-dev.c @@ -122,8 +122,8 @@ static long peci_dev_ioctl(struct file *file, uint iocmd, ulong arg) } xmsg = peci_get_xfer_msg(uxmsg.tx_len, uxmsg.rx_len); - if (IS_ERR(xmsg)) { - ret = PTR_ERR(xmsg); + if (!xmsg) { + ret = -ENOMEM; break; } @@ -162,7 +162,8 @@ static long peci_dev_ioctl(struct file *file, uint iocmd, ulong arg) } peci_put_xfer_msg(xmsg); - kfree(msg); + if (!IS_ERR(msg)) + kfree(msg); return (long)ret; }
peci_get_xfer_msg() returns NULL on failure, not an ERR_PTR. Also avoid calling kfree() on an ERR_PTR. Signed-off-by: Zev Weiss <zev@bewilderbeest.net> --- drivers/peci/peci-dev.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)