[PATCH] RedfishPkg/RedfishConfigHandler: fix FreePool issue

[PATCH] RedfishPkg/RedfishConfigHandler: fix FreePool issue

[Igor Kulchytskyy]

gRedfishDiscoveredToken buffer is allocated as one piece
during protocol installed process, but deleted by parts
during driver unload process.

Cc: Abner Chang <abner.chang@amd.com>
Cc: Nickle Wang <nicklew@nvidia.com>
Cc: Igor Kulchytskyy <igork@ami.com>
Signed-off-by: Igor Kulchytskyy <igork@ami.com>
---
 RedfishPkg/RedfishConfigHandler/RedfishConfigHandlerDriver.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/RedfishPkg/RedfishConfigHandler/RedfishConfigHandlerDriver.c b/RedfishPkg/RedfishConfigHandler/RedfishConfigHandlerDriver.c
index 96ac70f418..64b7fb7841 100644
--- a/RedfishPkg/RedfishConfigHandler/RedfishConfigHandlerDriver.c
+++ b/RedfishPkg/RedfishConfigHandler/RedfishConfigHandlerDriver.c
@@ -481,6 +481,7 @@ RedfishDiscoverProtocolInstalled (
 ErrorReturn:
   if (gRedfishDiscoveredToken != NULL) {
     FreePool (gRedfishDiscoveredToken);
+    gRedfishDiscoveredToken = NULL;
   }
 }

@@ -511,10 +512,10 @@ RedfishConfigHandlerDriverUnload (
         gBS->CloseEvent (ThisRedfishDiscoveredToken->Event);
       }

-      FreePool (ThisRedfishDiscoveredToken);
       ThisRedfishDiscoveredToken++;
     }

+    FreePool (gRedfishDiscoveredToken);
     gRedfishDiscoveredToken = NULL;
   }

--
2.37.1.windows.1
-The information contained in this message may be confidential and proprietary to American Megatrends (AMI). This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited. Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.

Re: [PATCH] RedfishPkg/RedfishConfigHandler: fix FreePool issue

[Chang, Abner]

[AMD Official Use Only - General]

Hi Igor,
Thanks for catching this issue, I have a comment below inline.

> -----Original Message-----
> From: Igor Kulchytskyy <igork@ami.com>
> Sent: Saturday, February 11, 2023 5:43 AM
> To: devel@edk2.groups.io
> Cc: Chang, Abner <Abner.Chang@amd.com>; Nickle Wang
> <nicklew@nvidia.com>
> Subject: [PATCH] RedfishPkg/RedfishConfigHandler: fix FreePool issue
> 
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
> 
> 
> gRedfishDiscoveredToken buffer is allocated as one piece during protocol
> installed process, but deleted by parts during driver unload process.
> 
> Cc: Abner Chang <abner.chang@amd.com>
> Cc: Nickle Wang <nicklew@nvidia.com>
> Cc: Igor Kulchytskyy <igork@ami.com>
> Signed-off-by: Igor Kulchytskyy <igork@ami.com>
> ---
>  RedfishPkg/RedfishConfigHandler/RedfishConfigHandlerDriver.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/RedfishPkg/RedfishConfigHandler/RedfishConfigHandlerDriver.c
> b/RedfishPkg/RedfishConfigHandler/RedfishConfigHandlerDriver.c
> index 96ac70f418..64b7fb7841 100644
> --- a/RedfishPkg/RedfishConfigHandler/RedfishConfigHandlerDriver.c
> +++ b/RedfishPkg/RedfishConfigHandler/RedfishConfigHandlerDriver.c
> @@ -481,6 +481,7 @@ RedfishDiscoverProtocolInstalled (
>  ErrorReturn:
>    if (gRedfishDiscoveredToken != NULL) {
>      FreePool (gRedfishDiscoveredToken);
> +    gRedfishDiscoveredToken = NULL;
>    }
>  }
> 
> @@ -511,10 +512,10 @@ RedfishConfigHandlerDriverUnload (
>          gBS->CloseEvent (ThisRedfishDiscoveredToken->Event);
>        }
> 
> -      FreePool (ThisRedfishDiscoveredToken);
>        ThisRedfishDiscoveredToken++;
>      }
> 
> +    FreePool (gRedfishDiscoveredToken);
I found here is a potential issue of gRedfishDiscoveredToken:
Due to RedfishDiscoverProtocolInstalled() may be called more than once in the case of multiple NIC installed on the system, means gRedfishDiscoveredToken will be overwritten by AllocateZeroPool() and result in memory leakage when unload the RedfishConfigHandler driver.
Could you please help to create an linked list (e.g. mRedfishDiscoveredTokenList) to record the newly allocated memory for gRedfishDiscoveredToken (I think we can rename it to just RedfishDiscoveredToken)?
So we can go through the link list to free RedfishDiscoveredToken allocated for each NIC when unload the driver.
Does this make sense?
Thanks
Abner

>      gRedfishDiscoveredToken = NULL;
>    }
> 
> --
> 2.37.1.windows.1
> -The information contained in this message may be confidential and
> proprietary to American Megatrends (AMI). This communication is intended
> to be read only by the individual or entity to whom it is addressed or by their
> designee. If the reader of this message is not the intended recipient, you are
> on notice that any distribution of this message, in any form, is strictly
> prohibited. Please promptly notify the sender by reply e-mail or by
> telephone at 770-246-8600, and then delete or destroy all copies of the
> transmission.

Re: [PATCH] RedfishPkg/RedfishConfigHandler: fix FreePool issue

[Igor Kulchytskyy]

Hi Abner,
Yes, I will work on this to create the linked list.
Thank you,
Igor

-----Original Message-----
From: Chang, Abner <Abner.Chang@amd.com>
Sent: Tuesday, February 14, 2023 1:55 AM
To: Igor Kulchytskyy <igork@ami.com>; devel@edk2.groups.io
Cc: Nickle Wang <nicklew@nvidia.com>
Subject: [EXTERNAL] RE: [PATCH] RedfishPkg/RedfishConfigHandler: fix FreePool issue


**CAUTION: The e-mail below is from an external source. Please exercise caution before opening attachments, clicking links, or following guidance.**

[AMD Official Use Only - General]

Hi Igor,
Thanks for catching this issue, I have a comment below inline.

> -----Original Message-----
> From: Igor Kulchytskyy <igork@ami.com>
> Sent: Saturday, February 11, 2023 5:43 AM
> To: devel@edk2.groups.io
> Cc: Chang, Abner <Abner.Chang@amd.com>; Nickle Wang
> <nicklew@nvidia.com>
> Subject: [PATCH] RedfishPkg/RedfishConfigHandler: fix FreePool issue
>
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
>
>
> gRedfishDiscoveredToken buffer is allocated as one piece during protocol
> installed process, but deleted by parts during driver unload process.
>
> Cc: Abner Chang <abner.chang@amd.com>
> Cc: Nickle Wang <nicklew@nvidia.com>
> Cc: Igor Kulchytskyy <igork@ami.com>
> Signed-off-by: Igor Kulchytskyy <igork@ami.com>
> ---
>  RedfishPkg/RedfishConfigHandler/RedfishConfigHandlerDriver.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/RedfishPkg/RedfishConfigHandler/RedfishConfigHandlerDriver.c
> b/RedfishPkg/RedfishConfigHandler/RedfishConfigHandlerDriver.c
> index 96ac70f418..64b7fb7841 100644
> --- a/RedfishPkg/RedfishConfigHandler/RedfishConfigHandlerDriver.c
> +++ b/RedfishPkg/RedfishConfigHandler/RedfishConfigHandlerDriver.c
> @@ -481,6 +481,7 @@ RedfishDiscoverProtocolInstalled (
>  ErrorReturn:
>    if (gRedfishDiscoveredToken != NULL) {
>      FreePool (gRedfishDiscoveredToken);
> +    gRedfishDiscoveredToken = NULL;
>    }
>  }
>
> @@ -511,10 +512,10 @@ RedfishConfigHandlerDriverUnload (
>          gBS->CloseEvent (ThisRedfishDiscoveredToken->Event);
>        }
>
> -      FreePool (ThisRedfishDiscoveredToken);
>        ThisRedfishDiscoveredToken++;
>      }
>
> +    FreePool (gRedfishDiscoveredToken);
I found here is a potential issue of gRedfishDiscoveredToken:
Due to RedfishDiscoverProtocolInstalled() may be called more than once in the case of multiple NIC installed on the system, means gRedfishDiscoveredToken will be overwritten by AllocateZeroPool() and result in memory leakage when unload the RedfishConfigHandler driver.
Could you please help to create an linked list (e.g. mRedfishDiscoveredTokenList) to record the newly allocated memory for gRedfishDiscoveredToken (I think we can rename it to just RedfishDiscoveredToken)?
So we can go through the link list to free RedfishDiscoveredToken allocated for each NIC when unload the driver.
Does this make sense?
Thanks
Abner

>      gRedfishDiscoveredToken = NULL;
>    }
>
> --
> 2.37.1.windows.1
> -The information contained in this message may be confidential and
> proprietary to American Megatrends (AMI). This communication is intended
> to be read only by the individual or entity to whom it is addressed or by their
> designee. If the reader of this message is not the intended recipient, you are
> on notice that any distribution of this message, in any form, is strictly
> prohibited. Please promptly notify the sender by reply e-mail or by
> telephone at 770-246-8600, and then delete or destroy all copies of the
> transmission.
-The information contained in this message may be confidential and proprietary to American Megatrends (AMI). This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited. Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.