Skip to content

Conversation

@LeviYeoReum
Copy link
Contributor

@LeviYeoReum LeviYeoReum commented Jun 6, 2025

Description

This patchset adds:
- Fix memory leak on Rx/Tx buffer in ArmFfaPeiLib.
- Add ArmFfaSecLib for PeilessSec.

Patch 1 fixes the memory leak on RxTx buffer when ArmFfaPeiLib used.
Patch 2 makes PcdFixedAtBuildPcd as PcdsFixedAtBuildPCD.
Patch 3 adds ArmFfaSecLib used in PeilessSec
Patch 4 cleanup duplication code on ArmFfaLib.
Patch 5 change type of buffer address in ArmFFaRxTxBufferInfo structure.

Signed-off-by: Yeoreum Yun [email protected]

@LeviYeoReum LeviYeoReum force-pushed the levi/arm_ffa_sec_lib branch 3 times, most recently from 3b1a672 to 8141648 Compare June 7, 2025 15:15
@LeviYeoReum
Copy link
Contributor Author

@kuqin12, @samimujawar
Could you review this please?

Thanks!

@LeviYeoReum LeviYeoReum force-pushed the levi/arm_ffa_sec_lib branch from 8141648 to aa2db3f Compare June 20, 2025 09:06
@LeviYeoReum LeviYeoReum requested a review from kuqin12 June 20, 2025 18:08
@LeviYeoReum
Copy link
Contributor Author

@kuqin12 @samimujawar
Gentle ping in case of forgotten.

@kuqin12
Copy link
Contributor

kuqin12 commented Jun 26, 2025

@LeviYeoReum I was thinking about this change in general. Would it make sense to let SEC phase to handle the memory movement? So that DXE phase does not need to know about this? I think leaving the movement to DXE phase and perform operations based on a flag in data is like logic leakage.

Or is there any reason that it cannot be done in SEC phase? In our use case, by the time SEC hands off to DXE, the permanent memory is already available.

@LeviYeoReum
Copy link
Contributor Author

Hi Kun,

Thanks for reviewing this patch :)

@LeviYeoReum I was thinking about this change in general. Would it make sense to let SEC phase to handle the memory movement? So that DXE phase does not need to know about this? I think leaving the movement to DXE phase and perform operations based on a flag in data is like logic leakage.

Or is there any reason that it cannot be done in SEC phase? In our use case, by the time SEC hands off to DXE, the permanent memory is already available.

Actually for PeilessSec as you said, this is true. it don't need to do memory movement.
However, what I thought is when SEC + PEI phase is enabled and SEC allocates the Rx/Tx buffer.
In this sitaution, SEC couldn't use the permanent memory but it could be used by PEI second phase.
But, Suppose no PEIM uses the ArmFfaLib.
Then the Rx/Tx buffer allocated by SEC still allocated in temporary memory,
so If the DXE doesn't handle the memory movement, It wouldn't use the Rx/Tx buffer properly.

Thanks.

@kuqin12
Copy link
Contributor

kuqin12 commented Jun 27, 2025

Hi Kun,

Thanks for reviewing this patch :)

@LeviYeoReum I was thinking about this change in general. Would it make sense to let SEC phase to handle the memory movement? So that DXE phase does not need to know about this? I think leaving the movement to DXE phase and perform operations based on a flag in data is like logic leakage.
Or is there any reason that it cannot be done in SEC phase? In our use case, by the time SEC hands off to DXE, the permanent memory is already available.

Actually for PeilessSec as you said, this is true. it don't need to do memory movement. However, what I thought is when SEC + PEI phase is enabled and SEC allocates the Rx/Tx buffer. In this sitaution, SEC couldn't use the permanent memory but it could be used by PEI second phase. But, Suppose no PEIM uses the ArmFfaLib. Then the Rx/Tx buffer allocated by SEC still allocated in temporary memory, so If the DXE doesn't handle the memory movement, It wouldn't use the Rx/Tx buffer properly.

Thanks.

I see. Please correct me if I misunderstood, we mainly have 2 scenarios:

Boot Process Notes
SEC -> DXE No memory movement
SEC -> PEI -> DXE Need movement in PEI

The concern is mainly around when no PEIM uses FFA, correct? Wondering if you have considered using a separate PEIM to handle the movement? We are currently imposing all FFA consuming PEIMs to register a callback, is that necessary?

A less-than-optimal, but simpler solution is to remap the buffer regardless in DXE phase. This is trading the complicated logic of carrying on the information with a little overhead of remapping the buffer, unless you can think of any reason why DXE has to use the same Rx/Tx buffer as PEI.

@LeviYeoReum
Copy link
Contributor Author

LeviYeoReum commented Jun 27, 2025

Hi Kun,

I see. Please correct me if I misunderstood, we mainly have 2 scenarios:

Boot Process Notes
SEC -> DXE No memory movement

This case is PeilessSec. So Yes.

SEC -> PEI -> DXE Need movement in PEI

Here is tow case.
- In PEI phase no one use ArmFfaPeiLib so, movement occurs in PEI so Dxe do it.
- PEI do memory movement.

The concern is mainly around when no PEIM uses FFA, correct?

Yes.

Wondering if you have considered using a separate PEIM to handle the movement? We are currently imposing all FFA >consuming PEIMs to register a callback, is that necessary?

Yes. if PEIM use ArmFfaPeiLib. it's done by ArmFfaLib.
But in case of doing no PEIM using ArmFfaPeiLib, makes a PEIM seems too much.

A less-than-optimal, but simpler solution is to remap the buffer regardless in DXE phase. This is trading the complicated logic of carrying on the information with a little overhead of remapping the buffer, unless you can think of any reason why DXE has to use the same Rx/Tx buffer as PEI.

Hmm.. This seems not good for me.
If Rx/Tx buffer is allocated in ealry PEI phase (before permenant memory use), It's allocated as "Reserved Memory".
But If SPMC requires minimum alignment as 64KB. It's too big to take it as overhead (That's why I think it's a "LEAK").
As you said, There's no reason we must use "the same Rx/Tx Buffer". But we care the "LEAK".
Also, access the Temporary memory after PEIM can use permanent memory seems the UAF.

Thanks ;)

@kuqin12
Copy link
Contributor

kuqin12 commented Jun 27, 2025

Wondering if you have considered using a separate PEIM to handle the movement? We are currently imposing all FFA >consuming PEIMs to register a callback, is that necessary?

Yes. if PEIM use ArmFfaPeiLib. it's done by ArmFfaLib. But in case of doing no PEIM using ArmFfaPeiLib, makes a PEIM seems too much.

Is a PEIM really considered too much? I think it might be better than sprinkle the callbacks around all PEIMs that do use FFA?

A less-than-optimal, but simpler solution is to remap the buffer regardless in DXE phase. This is trading the complicated logic of carrying on the information with a little overhead of remapping the buffer, unless you can think of any reason why DXE has to use the same Rx/Tx buffer as PEI.

Hmm.. This seems not good for me. If Rx/Tx buffer is allocated in ealry PEI phase (before permenant memory use), It's allocated as "Reserved Memory". But If SPMC requires minimum alignment as 64KB. It's too big to take it as overhead (That's why I think it's a "LEAK"). As you said, There's no reason we must use "the same Rx/Tx Buffer". But we care the "LEAK". Also, access the Temporary memory after PEIM can use permanent memory seems the UAF.

Sorry, maybe I missed your point of "LEAK". But even if we stick to the same buffer throughout the boot phase, OS/hypervisor will remap their Rx/Tx buffer anyway. Would that not be a leakage?

@LeviYeoReum
Copy link
Contributor Author

LeviYeoReum commented Jun 28, 2025

Hi Kun,

Is a PEIM really considered too much? I think it might be better than sprinkle the callbacks around all PEIMs that do use FFA?

Hmm, the "Remap" is done only once. I don't think the callback is sprinkled by all PEIM?
Secondly, though Rx/Tx buffer is allocated by Library but remaping is done by PEIM seems much srpinkle for me since
the FF-A user for PEI, it always add one more line in there .dsc file to use ArmFfaPeiLib..

Hmm.. This seems not good for me. If Rx/Tx buffer is allocated in ealry PEI phase (before permenant memory use), It's allocated as "Reserved Memory". But If SPMC requires minimum alignment as 64KB. It's too big to take it as overhead (That's why I think it's a "LEAK"). As you said, There's no reason we must use "the same Rx/Tx Buffer". But we care the "LEAK". Also, access the Temporary memory after PEIM can use permanent memory seems the UAF.

Sorry, maybe I missed your point of "LEAK". But even if we stick to the same buffer throughout the boot phase, OS/hypervisor will remap their Rx/Tx buffer anyway. Would that not be a leakage?

No. When you see the before this patch, we skip the "FreeMemory()" in case of PEIM allocate Rx/Tx buffer
(in PeiServiceMemoryDiscoveredNotifyCallback()).
This means if Rx/Tx Buffer is allocated by PEIM, the former Rx/Tx buffer is still allocated as a "Reserved Memory"
after PEI intialized the permenat memory (the allocated at temporary memory wiill be mirgrated in peremnant memory to by calculating offest) -- for detail, Please see my commit message:
a5d19c5

So, this unused Rx/Tx buffer memory area still reserved in OS/Hypervisor, It's a leak.

Thanks :)

@kuqin12
Copy link
Contributor

kuqin12 commented Jun 28, 2025

Hmm, the "Remap" is done only once. I don't think the callback is sprinkled by all PEIM? Secondly, though Rx/Tx buffer is allocated by Library but remaping is done by PEIM seems much srpinkle for me....

We are imposing all FFA aware PEI modules to carry the check of permanent memory availability. Additionally, the callbacks are registered for each of the PEIMs that will use FFA lib in early PEI phase, correct? I think this will increase the footprint on flash for each involved module. And early PEIMs normally do not have compression.

No. When you see the before this patch, we skip the "FreeMemory()" in case of PEIM allocate Rx/Tx buffer (in PeiServiceMemoryDiscoveredNotifyCallback()). This means if Rx/Tx Buffer is allocated by PEIM, the former Rx/Tx buffer is still allocated as a "Reserved Memory" after PEI intialized the permenat memory (the allocated at temporary memory wiill be allocated in peremnant memory to by calculating offest).

So, this unused Rx/Tx buffer memory area still reserved in OS/Hypervisor, It's a leak.

Are you confirming that, if OS/Hypervisor will not use the same Rx/Tx buffer region, they will "leak" the memory region because it is marked as reserved in UEFI, even after this change?

@LeviYeoReum
Copy link
Contributor Author

LeviYeoReum commented Jun 28, 2025

Hi Kun,

We are imposing all FFA aware PEI modules to carry the check of permanent memory availability. Additionally, the callbacks are registered for each of the PEIMs that will use FFA lib in early PEI phase, correct? I think this will increase the footprint on flash for each involved module. And early PEIMs normally do not have compression.

No, callback is registered by the first PEIM who allocates the Rx/Tx buffer if Rx/Tx buffer are allocated in Temporary memory.
So, It doesn't impose entry PEIM module to take of it.
I afraid of whether I understand your concern correct or not, But for me separate handling for Remap (migrate) in separate PEIM module makes user mistake easily who want to use ArmFfaPeiLib.
May I ask what is benefit to have separate PEIM to handle Remap only for Rx/Tx buffer taking over my worry?
The increase size for this patch for code area seems ignorable I think compared to have separate PEIM module for this (Including basic library for ths PEIM and etc..?).

Are you confirming that, if OS/Hypervisor will not use the same Rx/Tx buffer region, they will "leak" the memory region because it is marked as reserved in UEFI, even after this change?

Before this patch, Yes.

  1. Allocate Rx/Tx buffer in temporary memory.
  2. while Rx/Tx buffer is mirgrated after permanent my use, the Rx/Tx buffer allocated in temporary memory is migrated too.
  3. at the Callback for PEIM memory, It allocates New "Rx/Tx" buffer, So (2) buffer is leaked and still remain as "Reserved"
    (NOTE: It saves this information in the "HOB", so Dxe reserves this memory as "Reserved Memory")

After this Patch, No.
Since:
1. Rx/Tx buffer is remapped with migrated address, in this progress, there is no allocation.
2. At the ExitBootService, this area is freed via FreeMemory() this means the area is remarked as Conventional memory.
So no memory leak for this.

Thanks!

@kuqin12
Copy link
Contributor

kuqin12 commented Jun 28, 2025

Before this patch, Yes.

  1. Allocate Rx/Tx buffer in temporary memory.
  2. while Rx/Tx buffer is mirgrated after permanent my use, the Rx/Tx buffer allocated in temporary memory is migrated too.
  3. at the Callback for PEIM memory, It allocates New "Rx/Tx" buffer, So (2) buffer is leaked and still remain as "Reserved"
    (NOTE: It saves this information in the "HOB", so Dxe reserves this memory as "Reserved Memory")

After this Patch, No. Since: 1. Rx/Tx buffer is remapped with migrated address, in this progress, there is no allocation. 2. At the ExitBootService, this area is freed via FreeMemory() this means the area is remarked as Conventional memory. So no memory leak for this.

Great. The migration part makes sense.

We have been all over the places during the conversation. My overall point was that I hope the PEI being the only one that needs to handle the logic. A separate PEIM will cover the edge case if no PEIM uses FFA. The single module probably will save the footprint more scalablely. But you can decide how to proceed on that point.

@LeviYeoReum
Copy link
Contributor Author

Before this patch, Yes.

  1. Allocate Rx/Tx buffer in temporary memory.
  2. while Rx/Tx buffer is mirgrated after permanent my use, the Rx/Tx buffer allocated in temporary memory is migrated too.
  3. at the Callback for PEIM memory, It allocates New "Rx/Tx" buffer, So (2) buffer is leaked and still remain as "Reserved"
    (NOTE: It saves this information in the "HOB", so Dxe reserves this memory as "Reserved Memory")

After this Patch, No. Since: 1. Rx/Tx buffer is remapped with migrated address, in this progress, there is no allocation. 2. At the ExitBootService, this area is freed via FreeMemory() this means the area is remarked as Conventional memory. So no memory leak for this.

Great. The migration part makes sense.

Thanks ;)

We have been all over the places during the conversation. My overall point was that I hope the PEI being the only one that needs to handle the logic. A separate PEIM will cover the edge case if no PEIM uses FFA. The single module probably will save the footprint more scalablely. But you can decide how to proceed on that point

I understood what is your point. But, IMHO, I'm not sure the add PEIM really saves the footprint taking the duty to add the PEIM on every platform which want to use ArmFfaPeiLib and a risk -- not add the the PEIM for handling this.

Also, I'm not sure about the feature "edge case" like this problem -- handling something over the Phases.
if this happen in near future, I think It would be better to add at that time since for handling single problem to add one more PEIM seems to increase footprint for me.

If you agree, I want to remain as it is right now.

Thanks.

@LeviYeoReum LeviYeoReum force-pushed the levi/arm_ffa_sec_lib branch from aa2db3f to 79ba890 Compare June 30, 2025 10:08
@tianocore-assign-reviewers
Copy link

WARNING: Cannot add some reviewers: A user specified as a reviewer for this PR is not a collaborator of the repository. Please add them as a collaborator to the repository so they can be requested in the future.

Non-collaborators requested:

Attn Admins:


Admin Instructions:

  • Add the non-collaborators as collaborators to the appropriate team(s) listed in teams
  • If they are no longer needed as reviewers, remove them from Maintainers.txt

@LeviYeoReum LeviYeoReum force-pushed the levi/arm_ffa_sec_lib branch 6 times, most recently from 09d7d31 to 8757143 Compare June 30, 2025 17:51
@LeviYeoReum LeviYeoReum requested a review from kuqin12 June 30, 2025 17:51
@LeviYeoReum LeviYeoReum force-pushed the levi/arm_ffa_sec_lib branch from 8757143 to 12e3b09 Compare June 30, 2025 19:22
@LeviYeoReum LeviYeoReum requested a review from kuqin12 June 30, 2025 19:23
@LeviYeoReum LeviYeoReum force-pushed the levi/arm_ffa_sec_lib branch from 12e3b09 to 041ba8c Compare July 2, 2025 13:08
@LeviYeoReum
Copy link
Contributor Author

Hi @samerhaj and @lgao4,

If there is no problem, Would you merge this patch series please?

Thanks.

@lgao4
Copy link
Contributor

lgao4 commented Jul 4, 2025

Hi @samerhaj and @lgao4,

If there is no problem, Would you merge this patch series please?

Thanks.

If no more comments, I will merge it next week.

@LeviYeoReum LeviYeoReum force-pushed the levi/arm_ffa_sec_lib branch 3 times, most recently from dd0c9f1 to 43388aa Compare July 6, 2025 06:47
@LeviYeoReum
Copy link
Contributor Author

Hi @samimujawar and @lgao4

Could you merge this patch series please?

Copy link
Contributor

@samimujawar samimujawar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like one minor comment change was lost. Otherwise this PR looks good to me.

The commit e15fe06
("MdeModulePkg/Library: make ArmFfaPeiLib available early PEIM stage")
uses ArmFfaPeiLib in the early PEIM stage.

However, the Rx/Tx buffer allocated in the early PEIM stage uses
temporary memory. This results in a memory leak when the temporary
memory's heap is relocated to permanent memory.

For example, if the Rx/Tx buffer memory is allocated at 0x20006000
in temporary memory, and if offset between temporary memory and
permanent is 0x40000000, then:

 - Once permanent memory installed the temporary memory at 0x20006000
   is migrated to 0x60006000.
 - However, ArmFfaPeiLib allocates new Rx/Tx buffer without freeing
   the migrated Rx/Tx buffers, i.e. the buffers at 0x60006000.

This results in a memory leak as the migrated Rx/Tx buffer area is
lost.
To address this memory leak, use the MemoryAllocationHob's name, so
that the migrated memory area will be reused as Rx/Tx buffer.

This patch also includes rename ArmFfaRxTxStmm.c to
ArmFfaStandaloneMmRxTxMap.c to keep the file name convention in
ArmFfaLib with ArmFfa{Phase}{...}.c

Fixes: e15fe06 ("MdeModulePkg/Library: ...")
Signed-off-by: Yeoreum Yun <[email protected]>
Continuous-integration-options: PatchCheck.ignore-multi-package
The PcdFfaTxRxPageCount can never be changed dynamically
and is configured at build time to specify the size of
the Rx/Tx buffers.

Therefore, make PcdFfaTxRxPageCount a PcdsFixedAtBuild PCD.

Signed-off-by: Yeoreum Yun <[email protected]>
To use Arm-FFA intereface in PeilessSec, implments
ArmFfaSecLib used by PeilessSec.
For example, communicate with TPM service using CRB over ARM-FFA
(via Tpm2DeviceLibFfa), PeilessSec need to use Arm-FFA interface.

Signed-off-by: Yeoreum Yun <[email protected]>
Some of code for handling Rx/Tx buffer is duplicate.
This patch commonize some of duplication routine used in
Rx/Tx buffer related functions.

Signed-off-by: Yeoreum Yun <[email protected]>
…erInfo

Change type of buffer address type in ArmFfaRxTxBufferInfo
so that reduce the type casting.

Signed-off-by: Yeoreum Yun <[email protected]>
@LeviYeoReum LeviYeoReum force-pushed the levi/arm_ffa_sec_lib branch from 43388aa to 73bdc5b Compare July 14, 2025 09:54
@LeviYeoReum
Copy link
Contributor Author

Looks like one minor comment change was lost. Otherwise this PR looks good to me.

Thanks. I've updated it !

@samimujawar samimujawar added the push Auto push patch series in PR if all checks pass label Jul 14, 2025
@mergify mergify bot merged commit f85c718 into tianocore:master Jul 14, 2025
130 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

push Auto push patch series in PR if all checks pass

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants