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

[Jae Hyun Yoo]

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 at 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 at linux.intel.com>

> 
> 
> Zev
>