Re: [PATCH 1/2] peci: fix error-handling in peci_dev_ioctl()

[Zev Weiss <zev@bewilderbeest.net>]

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