Skip to content

Restore the stack canary #1273

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
bigspider wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Restore the stack canary #1273

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c831c4d
Move APP_STACK_CANARY_MAGIC constant to os_io.h
bigspider Oct 13, 2025
874a4cd
Initialize canary on os_io_init, and check it on receive/sends.
bigspider Oct 13, 2025
a4c52f6
Use os_sched_exit instead of os_reset when the stack canary is corrupted
bigspider Oct 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions io/include/os_io.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,12 @@ typedef struct {
#define OS_IO_BUFFER_SIZE OS_IO_SEPH_BUFFER_SIZE
#endif // !CUSTOM_IO_APDU_BUFFER_SIZE

#ifdef HAVE_BOLOS_APP_STACK_CANARY
#define APP_STACK_CANARY_MAGIC 0xDEAD0031
// apps will exit with this value if canary corruption is detected
#define APP_STACK_CANARY_CORRUPTED_EXIT_VALUE 42
#endif // HAVE_BOLOS_APP_STACK_CANARY

/* Exported macros------------------------------------------------------------*/

/* Exported variables --------------------------------------------------------*/
Expand Down
22 changes: 22 additions & 0 deletions io/src/os_io.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,10 @@ uint8_t G_io_init_syscall;

/* Private variables ---------------------------------------------------------*/

#ifdef HAVE_BOLOS_APP_STACK_CANARY
extern unsigned int app_stack_canary;
#endif // HAVE_BOLOS_APP_STACK_CANARY

/* Private functions ---------------------------------------------------------*/
#ifndef USE_OS_IO_STACK
static int process_itc_event(uint8_t *buffer_in, size_t buffer_in_length)
Expand Down Expand Up @@ -136,6 +140,10 @@ static int process_itc_event(uint8_t *buffer_in, size_t buffer_in_length)
#ifndef USE_OS_IO_STACK
int os_io_init(os_io_init_t *init)
{
#ifdef HAVE_BOLOS_APP_STACK_CANARY
app_stack_canary = APP_STACK_CANARY_MAGIC;
#endif // HAVE_BOLOS_APP_STACK_CANARY

if (!init) {
return -1;
}
Expand Down Expand Up @@ -241,6 +249,13 @@ int os_io_rx_evt(unsigned char *buffer,
int status = 0;
uint16_t length = 0;

#ifdef HAVE_BOLOS_APP_STACK_CANARY
// if the canary is corrupted, reset the device
if (app_stack_canary != APP_STACK_CANARY_MAGIC) {
os_sched_exit(APP_STACK_CANARY_CORRUPTED_EXIT_VALUE);
Copy link
Copy Markdown
Contributor

@iartemov-ledger iartemov-ledger Oct 14, 2025

Choose a reason for hiding this comment

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

In the perfect world it should be automatically tested against an app with and without legacy IO stack.
But we do not seem to have an infrastructure for this.
Might be https://github.com/LedgerHQ/speculos/tree/master/tests/c, but also it does not seem to fit.

}
#endif

if (!G_io_seph_buffer_size) {
status = os_io_seph_se_rx_event(G_io_seph_buffer,
sizeof(G_io_seph_buffer),
Expand Down Expand Up @@ -352,6 +367,13 @@ int os_io_tx_cmd(uint8_t type,
unsigned short length,
unsigned int *timeout_ms)
{
#ifdef HAVE_BOLOS_APP_STACK_CANARY
// if the canary is corrupted, reset the device
if (app_stack_canary != APP_STACK_CANARY_MAGIC) {
os_sched_exit(APP_STACK_CANARY_CORRUPTED_EXIT_VALUE);
Comment thread
iartemov-ledger marked this conversation as resolved.
}
#endif

int status = 0;
switch (type) {
#ifdef HAVE_IO_USB
Expand Down
3 changes: 0 additions & 3 deletions io_legacy/src/os_io_legacy.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,6 @@
/* Private defines------------------------------------------------------------*/

/* Private macros-------------------------------------------------------------*/
#ifdef HAVE_BOLOS_APP_STACK_CANARY
#define APP_STACK_CANARY_MAGIC 0xDEAD0031
#endif // HAVE_BOLOS_APP_STACK_CANARY
Comment on lines -51 to -53
Copy link
Copy Markdown
Contributor

@fbeutin-ledger fbeutin-ledger Oct 14, 2025

Choose a reason for hiding this comment

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

We'll need to check if this causes issues with some applications still using legacy IO and the canary

Copy link
Copy Markdown
Contributor Author

@bigspider bigspider Oct 14, 2025

Choose a reason for hiding this comment

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

Nothing should change for them. These are moved in os_io.h, which is imported by os_io_legacy.
The only difference is that this define is now visible in anything that imports os_io.h, so it could only conflict if they have a different define with the same name.


Comment thread
iartemov-ledger marked this conversation as resolved.
/* Private functions prototypes ----------------------------------------------*/
#ifdef HAVE_NFC_READER
Expand Down
22 changes: 22 additions & 0 deletions src/syscalls.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,10 @@
#include "os_endorsement.h"
#include <string.h>

#ifdef HAVE_BOLOS_APP_STACK_CANARY
extern unsigned int app_stack_canary;
#endif // HAVE_BOLOS_APP_STACK_CANARY

unsigned int SVC_Call(unsigned int syscall_id, void *parameters);
unsigned int SVC_cx_call(unsigned int syscall_id, unsigned int *parameters);

Expand Down Expand Up @@ -1865,6 +1869,10 @@ int os_io_seph_se_rx_event(unsigned char *buffer,

__attribute((weak)) int os_io_init(os_io_init_t *init)
{
#ifdef HAVE_BOLOS_APP_STACK_CANARY
app_stack_canary = APP_STACK_CANARY_MAGIC;
#endif // HAVE_BOLOS_APP_STACK_CANARY

Copy link
Copy Markdown
Contributor

@jarevalo-ledger jarevalo-ledger Oct 14, 2025

Choose a reason for hiding this comment

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

I would prefer to not modify the syscall APIs.
Why not init in an early app start entrypoint?

Copy link
Copy Markdown
Contributor Author

@bigspider bigspider Oct 14, 2025

Choose a reason for hiding this comment

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

the syscalls APIs are the only place used for io for most apps (that is, after io revamp and defining USE_OS_IO_STACK). So even if you find a different place for initialization, you still need to modify syscalls.c to check for canary corruption on os_io_rx_evt/os_io_tx_cmd.

(I chose os_io_init because I checked that both the C and the Rust SDK app preamble calls it before any real application code.)

Copy link
Copy Markdown
Contributor

@jarevalo-ledger jarevalo-ledger Oct 16, 2025

Choose a reason for hiding this comment

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

Syscalls are not called directly by apps, there is SDK functions that can be used. Let's discuss to find a better solution.

unsigned int parameters[1];
parameters[0] = (unsigned int) init;
return (int) SVC_Call(SYSCALL_os_io_init_ID, parameters);
Expand All @@ -1889,6 +1897,13 @@ __attribute((weak)) int os_io_tx_cmd(unsigned char type,
unsigned short length,
unsigned int *timeout_ms)
{
#ifdef HAVE_BOLOS_APP_STACK_CANARY
// if the canary is corrupted, reset the device
if (app_stack_canary != APP_STACK_CANARY_MAGIC) {
os_sched_exit(APP_STACK_CANARY_CORRUPTED_EXIT_VALUE);
}
#endif
Comment on lines +1900 to +1905
Copy link
Copy Markdown
Contributor

@fbeutin-ledger fbeutin-ledger Oct 14, 2025

Choose a reason for hiding this comment

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

Maybe we can factorize this snippet

Copy link
Copy Markdown
Contributor Author

@bigspider bigspider Oct 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I was worried about introducing an additional function because if we know the stack is already corrupted, we should use it as little as possible.
Perhaps we can use a macro.


unsigned int parameters[4];
parameters[0] = (unsigned int) type;
parameters[1] = (unsigned int) buffer;
Expand All @@ -1902,6 +1917,13 @@ __attribute((weak)) int os_io_rx_evt(unsigned char *buffer,
unsigned int *timeout_ms,
bool check_se_event)
{
#ifdef HAVE_BOLOS_APP_STACK_CANARY
// if the canary is corrupted, reset the device
if (app_stack_canary != APP_STACK_CANARY_MAGIC) {
os_sched_exit(APP_STACK_CANARY_CORRUPTED_EXIT_VALUE);
}
#endif

unsigned int parameters[4];
parameters[0] = (unsigned int) buffer;
parameters[1] = (unsigned int) buffer_max_length;
Expand Down
Loading