Hi everyone,
As you may know, console drivers in TF-A are required to provide a number of callbacks. One of them is getc() (to read a character from the console). Even though most platform ports provide a valid implementation of it, it does not seem to be called anywhere in the code base today, effectively qualifying it as dead code.
I did a bit of git history digging and from what I've seen, the very first public version of TF-A (v0.2!) already had a getc() callback in the Arm PL011 UART driver. So my guess is that all subsequent UART drivers added after that followed the same approach. When the multi-console framework was introduced, it naturally catered for this feature as well.
However, taking a step back, I wonder why we introduced getc() in the first place... Unlike other firmwares (like U-boot or EDK2), TF-A does not implement any kind of interactive user shell. And from a security point of view, getc() constitutes an attack vector into TF-A, which might allow an attacker to inject arbitrary data. So keeping this functionality without any valid use case sounds like a bad idea to me.
Now, even though getc() is not used in upstream TF-A code right now, I realize there might be downstream / internal test setups which need it. For example, for firmware recovery purposes (receiving a backup firmware over a serial interface) or automated tests setups (some script driving a test session using some communication protocol over a serial interface).
Is anyone depending on such use cases?
If not, then I suggest we consider removing getc() feature altogether. We could always bring it back when there is a real use case for it (it will survive through git history).
At the very least, I would like to disable getc() by default. Enabling it would require setting a build flag.
Any thoughts or concerns?
Best regards, Sandrine
Hi Sandrine,
On Tue, Sep 12 2023, Sandrine Bailleux via TF-A wrote:
Now, even though getc() is not used in upstream TF-A code right now, I realize there might be downstream / internal test setups which need it. For example, for firmware recovery purposes (receiving a backup firmware over a serial interface) or automated tests setups (some script driving a test session using some communication protocol over a serial interface).
Is anyone depending on such use cases?
We have a customer in this situation. Their SoC uses Xmodem as last resort for loading BL2. The serial console is PL011. Upstream driver getc implementation works nicely for this purpose.
If not, then I suggest we consider removing getc() feature altogether. We could always bring it back when there is a real use case for it (it will survive through git history).
At the very least, I would like to disable getc() by default. Enabling it would require setting a build flag.
This is a good compromise, IMHO.
Thanks, baruch
Hello Brian, Baruch,
Thank you both for sharing your use cases, that helps a lot. It's now clear to me that we need to keep getc() support in TF-A code base.
This also means we should add tests in TF-A OpenCI to exercise getc(), if possible. This would make sure we do not inadvertently introduce any regression that would break your factory test CLI / firmware recovery setups in the future. Obviously we would not be able to test each and every individual console driver, simply because we do not have all required platforms in TF-A OpenCI, but we could at least cover the ones we have.
It sounds like we are leaning towards putting getc() behind a disabled-by-default build flag. I'll wait for others' opinions on the mailing list about that. Else I'll propose a patch to this effect in the coming weeks.
Best regards, Sandrine
On 9/12/23 18:10, Baruch Siach wrote:
Hi Sandrine,
On Tue, Sep 12 2023, Sandrine Bailleux via TF-A wrote:
Now, even though getc() is not used in upstream TF-A code right now, I realize there might be downstream / internal test setups which need it. For example, for firmware recovery purposes (receiving a backup firmware over a serial interface) or automated tests setups (some script driving a test session using some communication protocol over a serial interface).
Is anyone depending on such use cases?
We have a customer in this situation. Their SoC uses Xmodem as last resort for loading BL2. The serial console is PL011. Upstream driver getc implementation works nicely for this purpose.
If not, then I suggest we consider removing getc() feature altogether. We could always bring it back when there is a real use case for it (it will survive through git history).
At the very least, I would like to disable getc() by default. Enabling it would require setting a build flag.
This is a good compromise, IMHO.
Thanks, baruch
Hi Sandrine,
Putting getc() under a flag that is disabled by default looks like a good idea. It is not used on ST platforms (at least inside ST).
Best regards, Yann
On 9/13/23 08:45, Sandrine Bailleux via TF-A wrote:
Hello Brian, Baruch,
Thank you both for sharing your use cases, that helps a lot. It's now clear to me that we need to keep getc() support in TF-A code base.
This also means we should add tests in TF-A OpenCI to exercise getc(), if possible. This would make sure we do not inadvertently introduce any regression that would break your factory test CLI / firmware recovery setups in the future. Obviously we would not be able to test each and every individual console driver, simply because we do not have all required platforms in TF-A OpenCI, but we could at least cover the ones we have.
It sounds like we are leaning towards putting getc() behind a disabled-by-default build flag. I'll wait for others' opinions on the mailing list about that. Else I'll propose a patch to this effect in the coming weeks.
Best regards, Sandrine
On 9/12/23 18:10, Baruch Siach wrote:
Hi Sandrine,
On Tue, Sep 12 2023, Sandrine Bailleux via TF-A wrote:
Now, even though getc() is not used in upstream TF-A code right now, I realize there might be downstream / internal test setups which need it. For example, for firmware recovery purposes (receiving a backup firmware over a serial interface) or automated tests setups (some script driving a test session using some communication protocol over a serial interface).
Is anyone depending on such use cases?
We have a customer in this situation. Their SoC uses Xmodem as last resort for loading BL2. The serial console is PL011. Upstream driver getc implementation works nicely for this purpose.
If not, then I suggest we consider removing getc() feature altogether. We could always bring it back when there is a real use case for it (it will survive through git history).
At the very least, I would like to disable getc() by default. Enabling it would require setting a build flag.
This is a good compromise, IMHO.
Thanks, baruch
+1 to disable by default under flag.
Sent from my iPhone
On Sep 13, 2023, at 2:46 AM, Yann Gautier via TF-A tf-a@lists.trustedfirmware.org wrote:
Hi Sandrine,
Putting getc() under a flag that is disabled by default looks like a good idea. It is not used on ST platforms (at least inside ST).
Best regards, Yann
On 9/13/23 08:45, Sandrine Bailleux via TF-A wrote: Hello Brian, Baruch, Thank you both for sharing your use cases, that helps a lot. It's now clear to me that we need to keep getc() support in TF-A code base. This also means we should add tests in TF-A OpenCI to exercise getc(), if possible. This would make sure we do not inadvertently introduce any regression that would break your factory test CLI / firmware recovery setups in the future. Obviously we would not be able to test each and every individual console driver, simply because we do not have all required platforms in TF-A OpenCI, but we could at least cover the ones we have. It sounds like we are leaning towards putting getc() behind a disabled-by-default build flag. I'll wait for others' opinions on the mailing list about that. Else I'll propose a patch to this effect in the coming weeks. Best regards, Sandrine
On 9/12/23 18:10, Baruch Siach wrote: Hi Sandrine,
On Tue, Sep 12 2023, Sandrine Bailleux via TF-A wrote:
Now, even though getc() is not used in upstream TF-A code right now, I realize there might be downstream / internal test setups which need it. For example, for firmware recovery purposes (receiving a backup firmware over a serial interface) or automated tests setups (some script driving a test session using some communication protocol over a serial interface).
Is anyone depending on such use cases?
We have a customer in this situation. Their SoC uses Xmodem as last resort for loading BL2. The serial console is PL011. Upstream driver getc implementation works nicely for this purpose.
If not, then I suggest we consider removing getc() feature altogether. We could always bring it back when there is a real use case for it (it will survive through git history).
At the very least, I would like to disable getc() by default. Enabling it would require setting a build flag.
This is a good compromise, IMHO.
Thanks, baruch
-- TF-A mailing list -- tf-a@lists.trustedfirmware.org To unsubscribe send an email to tf-a-leave@lists.trustedfirmware.org
Hello,
Following up on our previous discussion in this thread, I've posted a patch to disable getc() by default:
https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/23549
The aim is not only to disable getc() - as in, not being able to call it anymore - but also to remove all getc-related code from the firmware binaries so that there is no remnant getc code which could be used as a gadget as part of a bigger security attack.
Let me explain the changes introduced by this patch and the design choices I have made - which are all up for discussion!
Changes =======
1. By default, getc() is now compiled out at the multi-console framework level.
This means it is no longer possible to invoke any of the console drivers's getc() callbacks through the multi-console framework.
Note that TF-A libc does not provide getc() so console_getc() is the highest-level API we've got in this area.
2. By default, if a console driver attempts to register a getc() callback into the multi-console framework, TF-A now fails to build.
For example with the Arm PL011 console driver: drivers/arm/pl011/aarch64/pl011_console.S: Assembler messages: drivers/arm/pl011/aarch64/pl011_console.S:110: Error: getc() support is disabled. Do not register a getc() console callback.
3. By default, all console drivers which registered a getc() callback no longer do.
All of these changes can be reverted by building TF-A with ENABLE_CONSOLE_GETC=1.
Code Pruning ============
I believe that the combination of the aforementioned 3 changes eliminate all getc-related code from the firmware binaries. Individual console drivers should not need to put their console_xxx_getc() definitions behind an '#if ENABLE_CONSOLE_GETC' guard for them to be compiled out, so long as the -ffunction-sections / --gc-sections flags combo is used, which is standard in TF-A. I've verified that with a couple of platforms / drivers. With this flags combo, console_xxx_getc() function code is isolated in its own section, which gets garbage-collected at link time since there is no reference to it.
The above assumes that platform code itself does not explicitly call console_xxx_getc(). If that's the case, then console_xxx_getc() code would obviously be retained in the binary.
Alternative Designs ===================
As an alternative to 1., I considered leaving console_getc() enabled but change the implementation to unconditionally return EOF. This would have achieved the same outcome - not being able to call console drivers's getc() callbacks anymore. However, I am not sure this is the desired behaviour for all platforms. Besides, I imagine such a behavioural change could go unnoticed (because getc() returning EOF is not an error case) and thus trigger annoying bugs in developers' setups. So I gave up on this idea.
As an alternative to 2., I considered handling this error at runtime, at the console driver's registration time. In other words, adding a test in console_register() function to make sure that the console driver registers no getc() callback (i.e. a NULL pointer). However, this incurs a slight performance penalty which we can avoid. More importantly, this requires all callers of console_register() to check its return code, which is far from true today!
Both of these issues could have been addressed with a debug assertion (no performance penalty in release builds - which are free of debug assertions - and no need to check the return code of console_register()) but printing the assertion message requires a functional console, which is not available at this time since we are still in the process of setting it up!
Please let me know if you have any concerns with this change or can think of better ways to implement it.
Best regards, Sandrine
Hi Sandrine,
On Wed, Sep 27 2023, Sandrine Bailleux via TF-A wrote:
Following up on our previous discussion in this thread, I've posted a patch to disable getc() by default:
https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/23549
The aim is not only to disable getc() - as in, not being able to call it anymore - but also to remove all getc-related code from the firmware binaries so that there is no remnant getc code which could be used as a gadget as part of a bigger security attack.
Let me explain the changes introduced by this patch and the design choices I have made - which are all up for discussion!
Changes
- By default, getc() is now compiled out at the multi-console framework
level.
This means it is no longer possible to invoke any of the console drivers's getc() callbacks through the multi-console framework.
Note that TF-A libc does not provide getc() so console_getc() is the highest-level API we've got in this area.
- By default, if a console driver attempts to register a getc() callback into
the multi-console framework, TF-A now fails to build.
For example with the Arm PL011 console driver: drivers/arm/pl011/aarch64/pl011_console.S: Assembler messages: drivers/arm/pl011/aarch64/pl011_console.S:110: Error: getc() support is disabled. Do not register a getc() console callback.
- By default, all console drivers which registered a getc() callback no
longer do.
All of these changes can be reverted by building TF-A with ENABLE_CONSOLE_GETC=1.
These changes are fine by me as they draw good security/usability balance. For what it's worth:
Acked-by: Baruch Siach baruch@tkos.co.il
Thanks, baruch
tf-a@lists.trustedfirmware.org
-
Baruch Siach -
Raghupathy Krishnamurthy -
Sandrine Bailleux -
Yann Gautier