Skip to content
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.

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

Furi: Detect use-after-free #3995

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

Willy-JL
Copy link
Contributor

@Willy-JL Willy-JL commented Nov 9, 2024

What's new

  • free() now marks memory with 0xDD
  • BusFault handler checks for 0xDDDDDDDD and 64K after it to determine "Possible use-after-free"
  • if such memory address is not dereferenced no crash will happen, but this change is still useful while debugging, as seeing 0xDD in your data is a clear giveaway of use-after-free
  • added back BusFault handler extra logging from old TLSF experiment
  • fixed atomicity in furi_semaphore_release():
    • if thread A is waiting for acquire() and thread B calls release()
    • then halfway through release() code, A would wake up and continue, before B finishes release()
    • this could cause use-after-free if A free()s semaphore after acquire(), because after this B would finish release() which dereferences instance->event_loop_link
    • for example, RpcCli had this use-after-free:
      • rpc_cli_command_start_session() is waiting for furi_semaphore_acquire()
      • rpc_cli_session_terminated_callback() will furi_semaphore_release()
      • this wakes up rpc_cli_command_start_session() which then does furi_semaphore_free()
      • later, furi_semaphore_release() inside of rpc_cli_session_terminated_callback() resumes, and dereferences the semaphore that rpc_cli_command_start_session() has already free'd
    • there might be more similar issues with event loop items being used after yielding, which would break atomicity and lead to possible use-after-free, but i have not looked for them or found other crashes like this yet

Verification

  • test anything and everything for hidden use-after-free that were unknown until now

Checklist (For Reviewer)

  • PR has description of feature/bug or link to Confluence/Jira task
  • Description contains actions to verify feature/bugfix
  • I've built this code, uploaded it to the device and verified feature/bugfix

if thread A waiting for acquire(), thread B calls release()
halfway through release() code, A would
wake up and continue, before B finishes release()
this could cause use-after-free if A free()s semaphore after acquire
@CookiePLMonster
Copy link
Contributor

  • BusFault handler checks for 0xDDDDDDDD and 64K after it to determine "Possible use-after-free"

Just sanity checking - is nothing mapped there in the Cortex M4 memory map? Peripheral or something?

@Willy-JL
Copy link
Contributor Author

  • BusFault handler checks for 0xDDDDDDDD and 64K after it to determine "Possible use-after-free"

Just sanity checking - is nothing mapped there in the Cortex M4 memory map? Peripheral or something?

doesnt seem like it from what i can see, in firmware code i see

/*!< Boundary memory map */
#define FLASH_BASE             (0x08000000UL)/*!< FLASH(up to 1 MB) base address */
#define SRAM_BASE              (0x20000000UL)/*!< SRAM(up to 256 KB) base address */
#define PERIPH_BASE            (0x40000000UL)/*!< Peripheral base address */

/*!< Memory, OTP and Option bytes */

/* Base addresses */
#define SYSTEM_MEMORY_BASE     (0x1FFF0000UL)  /*!< System Memory : 28Kb (0x1FFF0000 - 0x1FFF6FFF) */
#define OTP_AREA_BASE          (0x1FFF7000UL)  /*!< OTP area : 1kB (0x1FFF7000 - 0x1FFF73FF)       */
#define OPTION_BYTE_BASE       (0x1FFF8000UL)  /*!< Option Bytes : 4kB (0x1FFF8000 - 0x1FFF8FFF)   */
#define ENGI_BYTE_BASE         (0x1FFF7400UL)  /*!< Engi Bytes : 3kB (0x1FFF7400 - 0x1FFF7FFF)     */

however in the ARM documentation, it seems like 0xDDDDDDDD would fall into the "external device" region. not sure what that means
https://developer.arm.com/documentation/ddi0439/b/Programmers-Model/System-address-map

@skotopes
Copy link
Member

skotopes commented Dec 18, 2024

Double edge sword: on the one side it will do what you expect it to do when we talk about pointers, but then if it is value then it will cause more damage.
I wish there were some statistics to choose better option.

Btw, do you mind to split SCB flags+semaphore fix into separate PR, so I can fast track it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants