[PATCH] ipmi: kcs_bmc: handle devm_kasprintf() failure case

[PATCH] ipmi: kcs_bmc: handle devm_kasprintf() failure case

[Nicholas Mc Guire]

devm_kasprintf() may return NULL if internal allocation failed so this
assignment is not safe. Moved the error exit path and added the !NULL
which then allows the devres manager to take care of cleanup.

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
Fixes: cd2315d471f4 ("ipmi: kcs_bmc: don't change device name")
---

Problem located with experimental coccinelle script

Patch was compile tested with: aspeed_g5_defconfig (implies
CONFIG_IPMI_KCS_BMC=y)

Patch is against 4.20-rc3 (localversion-next is next-20181121)

 drivers/char/ipmi/kcs_bmc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
index e6124bd..86d29d2 100644
--- a/drivers/char/ipmi/kcs_bmc.c
+++ b/drivers/char/ipmi/kcs_bmc.c
@@ -440,12 +440,13 @@ struct kcs_bmc *kcs_bmc_alloc(struct device *dev, int sizeof_priv, u32 channel)
 	kcs_bmc->data_in = devm_kmalloc(dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
 	kcs_bmc->data_out = devm_kmalloc(dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
 	kcs_bmc->kbuffer = devm_kmalloc(dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
-	if (!kcs_bmc->data_in || !kcs_bmc->data_out || !kcs_bmc->kbuffer)
-		return NULL;
 
 	kcs_bmc->miscdev.minor = MISC_DYNAMIC_MINOR;
 	kcs_bmc->miscdev.name = devm_kasprintf(dev, GFP_KERNEL, "%s%u",
 					       DEVICE_NAME, channel);
+	if (!kcs_bmc->data_in || !kcs_bmc->data_out || !kcs_bmc->kbuffer ||
+	    !kcs_bmc->miscdev.name)
+		return NULL;
 	kcs_bmc->miscdev.fops = &kcs_bmc_fops;
 
 	return kcs_bmc;
-- 
2.1.4

Re: [PATCH] ipmi: kcs_bmc: handle devm_kasprintf() failure case

[Corey Minyard]

On 11/21/18 9:08 AM, Nicholas Mc Guire wrote:
> devm_kasprintf() may return NULL if internal allocation failed so this
> assignment is not safe. Moved the error exit path and added the !NULL
> which then allows the devres manager to take care of cleanup.


Added the original author.  This looks correct to me, I've included it, 
but I would
like Haiyue to comment, if possible.

Thanks,

-corey


> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> Fixes: cd2315d471f4 ("ipmi: kcs_bmc: don't change device name")
> ---
>
> Problem located with experimental coccinelle script
>
> Patch was compile tested with: aspeed_g5_defconfig (implies
> CONFIG_IPMI_KCS_BMC=y)
>
> Patch is against 4.20-rc3 (localversion-next is next-20181121)
>
>   drivers/char/ipmi/kcs_bmc.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
> index e6124bd..86d29d2 100644
> --- a/drivers/char/ipmi/kcs_bmc.c
> +++ b/drivers/char/ipmi/kcs_bmc.c
> @@ -440,12 +440,13 @@ struct kcs_bmc *kcs_bmc_alloc(struct device *dev, int sizeof_priv, u32 channel)
>   	kcs_bmc->data_in = devm_kmalloc(dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
>   	kcs_bmc->data_out = devm_kmalloc(dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
>   	kcs_bmc->kbuffer = devm_kmalloc(dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
> -	if (!kcs_bmc->data_in || !kcs_bmc->data_out || !kcs_bmc->kbuffer)
> -		return NULL;
>   
>   	kcs_bmc->miscdev.minor = MISC_DYNAMIC_MINOR;
>   	kcs_bmc->miscdev.name = devm_kasprintf(dev, GFP_KERNEL, "%s%u",
>   					       DEVICE_NAME, channel);
> +	if (!kcs_bmc->data_in || !kcs_bmc->data_out || !kcs_bmc->kbuffer ||
> +	    !kcs_bmc->miscdev.name)
> +		return NULL;
>   	kcs_bmc->miscdev.fops = &kcs_bmc_fops;
>   
>   	return kcs_bmc;

Re: [PATCH] ipmi: kcs_bmc: handle devm_kasprintf() failure case

[Wang, Haiyue]

[Resend for wrong reply HTML format mail]

Great check for making kcs_bmc module be more stable and handle things gracefully.

My tag if needed.
      Reviewed-by: Haiyue Wang<haiyue.wang@linux.intel.com>

在 2018-11-27 21:36, Corey Minyard 写道:
> On 11/21/18 9:08 AM, Nicholas Mc Guire wrote:
>> devm_kasprintf() may return NULL if internal allocation failed so this
>> assignment is not safe. Moved the error exit path and added the !NULL
>> which then allows the devres manager to take care of cleanup.
>
>
> Added the original author.  This looks correct to me, I've included 
> it, but I would
> like Haiyue to comment, if possible.
>
> Thanks,
>
> -corey
>
>
>> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
>> Fixes: cd2315d471f4 ("ipmi: kcs_bmc: don't change device name") 

Re: [Openipmi-developer] [PATCH] ipmi: kcs_bmc: handle devm_kasprintf() failure case

[Corey Minyard]

On 11/27/18 6:54 PM, Wang, Haiyue wrote:
> [Resend for wrong reply HTML format mail]
>
> Great check for making kcs_bmc module be more stable and handle things 
> gracefully.
>
> My tag if needed.
>      Reviewed-by: Haiyue Wang<haiyue.wang@linux.intel.com>
>
Thanks for the review, it's included.

-corey


> 在 2018-11-27 21:36, Corey Minyard 写道:
>> On 11/21/18 9:08 AM, Nicholas Mc Guire wrote:
>>> devm_kasprintf() may return NULL if internal allocation failed so this
>>> assignment is not safe. Moved the error exit path and added the !NULL
>>> which then allows the devres manager to take care of cleanup.
>>
>>
>> Added the original author.  This looks correct to me, I've included 
>> it, but I would
>> like Haiyue to comment, if possible.
>>
>> Thanks,
>>
>> -corey
>>
>>
>>> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
>>> Fixes: cd2315d471f4 ("ipmi: kcs_bmc: don't change device name") 
>
>
> _______________________________________________
> Openipmi-developer mailing list
> Openipmi-developer@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openipmi-developer