ibmvnic: fix krealloc() memory leak

[PATCH] ibmvnic: fix krealloc() memory leak

Posted by Alexander A. Klimov 1 week, 6 days ago

Don't just overwrite the original pointer passed to krealloc()
with its return value without checking latter:

    MEM = krealloc(MEM, SZ, GFP);

If krealloc() returns NULL, that erases the pointer
to the still allocated memory, hence leaks this memory.
Instead, use a temporary variable, check it's not NULL
and only then assign it to the original pointer:

    TMP = krealloc(MEM, SZ, GFP);
    if (!TMP) return;
    MEM = TMP;

Fixes: 4e6759be28e4 ("ibmvnic: Feature implementation of Vital Product Data (VPD) for the ibmvnic driver")
Signed-off-by: Alexander A. Klimov <grandmaster@al2klimov.de>
---
 drivers/net/ethernet/ibm/ibmvnic.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index 5a510eed335e..25d1d844ad19 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -1761,8 +1761,9 @@ static int ibmvnic_get_vpd(struct ibmvnic_adapter *adapter)
 	union ibmvnic_crq crq;
 	int len = 0;
 	int rc;
+	unsigned char *buff = adapter->vpd->buff;
 
-	if (adapter->vpd->buff)
+	if (buff)
 		len = adapter->vpd->len;
 
 	mutex_lock(&adapter->fw_lock);
@@ -1788,17 +1789,17 @@ static int ibmvnic_get_vpd(struct ibmvnic_adapter *adapter)
 	if (!adapter->vpd->len)
 		return -ENODATA;
 
-	if (!adapter->vpd->buff)
-		adapter->vpd->buff = kzalloc(adapter->vpd->len, GFP_KERNEL);
+	if (!buff)
+		buff = kzalloc(adapter->vpd->len, GFP_KERNEL);
 	else if (adapter->vpd->len != len)
-		adapter->vpd->buff =
-			krealloc(adapter->vpd->buff,
-				 adapter->vpd->len, GFP_KERNEL);
+		buff = krealloc(buff,
+				adapter->vpd->len, GFP_KERNEL);
 
-	if (!adapter->vpd->buff) {
+	if (!buff) {
 		dev_err(dev, "Could allocate VPD buffer\n");
 		return -ENOMEM;
 	}
+	adapter->vpd->buff = buff;
 
 	adapter->vpd->dma_addr =
 		dma_map_single(dev, adapter->vpd->buff, adapter->vpd->len,
-- 
2.54.0

Re: [PATCH] ibmvnic: fix krealloc() memory leak

Posted by Nicolai Buchwitz 1 week, 5 days ago

Hi Alex

You patch is missing the prefix with the target tree. Please have
a look at [1] for more details on the workflow.

On 26.5.2026 20:41, Alexander A. Klimov wrote:
> Don't just overwrite the original pointer passed to krealloc()
> with its return value without checking latter:
> 
>     MEM = krealloc(MEM, SZ, GFP);
> 
> If krealloc() returns NULL, that erases the pointer
> to the still allocated memory, hence leaks this memory.
> Instead, use a temporary variable, check it's not NULL
> and only then assign it to the original pointer:
> 
>     TMP = krealloc(MEM, SZ, GFP);
>     if (!TMP) return;
>     MEM = TMP;
> 
> Fixes: 4e6759be28e4 ("ibmvnic: Feature implementation of Vital Product 
> Data (VPD) for the ibmvnic driver")
> Signed-off-by: Alexander A. Klimov <grandmaster@al2klimov.de>
> ---
>  drivers/net/ethernet/ibm/ibmvnic.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c 
> b/drivers/net/ethernet/ibm/ibmvnic.c
> index 5a510eed335e..25d1d844ad19 100644
> --- a/drivers/net/ethernet/ibm/ibmvnic.c
> +++ b/drivers/net/ethernet/ibm/ibmvnic.c
> @@ -1761,8 +1761,9 @@ static int ibmvnic_get_vpd(struct ibmvnic_adapter 
> *adapter)
>  	union ibmvnic_crq crq;
>  	int len = 0;
>  	int rc;
> +	unsigned char *buff = adapter->vpd->buff;

Should be reverse x-mas tree (longest to shortest).

> 
> -	if (adapter->vpd->buff)
> +	if (buff)
>  		len = adapter->vpd->len;
> 
>  	mutex_lock(&adapter->fw_lock);
> @@ -1788,17 +1789,17 @@ static int ibmvnic_get_vpd(struct 
> ibmvnic_adapter *adapter)
>  	if (!adapter->vpd->len)
>  		return -ENODATA;
> 
> -	if (!adapter->vpd->buff)
> -		adapter->vpd->buff = kzalloc(adapter->vpd->len, GFP_KERNEL);
> +	if (!buff)
> +		buff = kzalloc(adapter->vpd->len, GFP_KERNEL);
>  	else if (adapter->vpd->len != len)
> -		adapter->vpd->buff =
> -			krealloc(adapter->vpd->buff,
> -				 adapter->vpd->len, GFP_KERNEL);
> +		buff = krealloc(buff,
> +				adapter->vpd->len, GFP_KERNEL);

Dead branch? The only caller, init_resources(), kzalloc()s a fresh vpd
right before, and resets run release_vpd_data() first, so vpd->buff is
always NULL here and kzalloc() above always wins. The leak can't 
trigger,
which makes the Fixes tag misleading.

> 
> -	if (!adapter->vpd->buff) {
> +	if (!buff) {
>  		dev_err(dev, "Could allocate VPD buffer\n");
>  		return -ENOMEM;
>  	}
> +	adapter->vpd->buff = buff;

If you keep it anyway: on failure the old buffer stays in vpd->buff 
while
vpd->len is already the new size, a mismatch the original avoided by
NULLing. kfree() it (krealloc() does not free on failure) and NULL 
before
-ENOMEM.

> 
>  	adapter->vpd->dma_addr =
>  		dma_map_single(dev, adapter->vpd->buff, adapter->vpd->len,

[1] https://docs.kernel.org/process/maintainer-netdev.html

Thanks,
Nicolai

Re: [PATCH] ibmvnic: fix krealloc() memory leak

Posted by Alexander A. Klimov 1 week, 5 days ago


On 5/26/26 22:50, Nicolai Buchwitz wrote:
> Hi Alex
> 
> You patch is missing the prefix with the target tree. Please have
> a look at [1] for more details on the workflow.

Damn. :-)

So far I've always got away with whatever I copied from the majority of
git log --oneline --follow FILE

> 
> On 26.5.2026 20:41, Alexander A. Klimov wrote:
>> Don't just overwrite the original pointer passed to krealloc()
>> with its return value without checking latter:
>>
>>     MEM = krealloc(MEM, SZ, GFP);
>>
>> If krealloc() returns NULL, that erases the pointer
>> to the still allocated memory, hence leaks this memory.
>> Instead, use a temporary variable, check it's not NULL
>> and only then assign it to the original pointer:
>>
>>     TMP = krealloc(MEM, SZ, GFP);
>>     if (!TMP) return;
>>     MEM = TMP;
>>
>> Fixes: 4e6759be28e4 ("ibmvnic: Feature implementation of Vital Product Data (VPD) for the ibmvnic driver")
>> Signed-off-by: Alexander A. Klimov <grandmaster@al2klimov.de>
>> ---
>>  drivers/net/ethernet/ibm/ibmvnic.c | 15 ++++++++-------
>>  1 file changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
>> index 5a510eed335e..25d1d844ad19 100644
>> --- a/drivers/net/ethernet/ibm/ibmvnic.c
>> +++ b/drivers/net/ethernet/ibm/ibmvnic.c
>> @@ -1761,8 +1761,9 @@ static int ibmvnic_get_vpd(struct ibmvnic_adapter *adapter)
>>      union ibmvnic_crq crq;
>>      int len = 0;
>>      int rc;
>> +    unsigned char *buff = adapter->vpd->buff;
> 
> Should be reverse x-mas tree (longest to shortest).
> 
>>
>> -    if (adapter->vpd->buff)
>> +    if (buff)
>>          len = adapter->vpd->len;
>>
>>      mutex_lock(&adapter->fw_lock);
>> @@ -1788,17 +1789,17 @@ static int ibmvnic_get_vpd(struct ibmvnic_adapter *adapter)
>>      if (!adapter->vpd->len)
>>          return -ENODATA;
>>
>> -    if (!adapter->vpd->buff)
>> -        adapter->vpd->buff = kzalloc(adapter->vpd->len, GFP_KERNEL);
>> +    if (!buff)
>> +        buff = kzalloc(adapter->vpd->len, GFP_KERNEL);
>>      else if (adapter->vpd->len != len)
>> -        adapter->vpd->buff =
>> -            krealloc(adapter->vpd->buff,
>> -                 adapter->vpd->len, GFP_KERNEL);
>> +        buff = krealloc(buff,
>> +                adapter->vpd->len, GFP_KERNEL);
> 
> Dead branch? The only caller, init_resources(), kzalloc()s a fresh vpd
> right before, and resets run release_vpd_data() first, so vpd->buff is
> always NULL here and kzalloc() above always wins. The leak can't trigger,

Cool! No leak = no problem = nothing to do here