Skip to content

Conversation

@derMihai
Copy link

@derMihai derMihai commented Jan 6, 2026

Description

The SPI framing protocol assumes SPI_BUF_SIZE transfer sizes, but the tx skb size is not extended to cover that. This results in out-of-bounds reads, sending random kernel memory down the bus.

I fixed this by re-allocating the tx skb whenever its size is smaller than SPI_BUF_SIZE. This is done before the SPI transfer. A more efficient approach would be to do this in process_tx_packet(), where the skb is usually re-allocated anyway to meet alignment requirements. But since that happens in the transport-independent part of the driver it would require some more refactoring.

The esp_spi_work() function, where I added the new logic, was already nesting a lot so I took the liberty to refactor it a bit, including throwing away the mutex and instead enforcing serial execution with alloc_ordered_workqueue(). If hope that's ok.

Testing

I tested this by running a BLE speed test for about 10 mins (l2cat, part of bluer) on the following setup:

  • ESP32-C6 as controller
  • Raspberry Pi 3b with 6.12.47+rpt-rpi-v8
  • my other Linux PC as BLE peer

Checklist

Before submitting a Pull Request, please ensure the following:

  • 🚨 This PR does not introduce breaking changes.
  • All CI checks (GH Actions) pass.
  • Documentation is updated as needed.
  • Tests are updated or added as necessary.
  • Code is well-commented, especially in complex areas.
  • Git history is clean — commits are squashed to the minimum necessary.

@CLAassistant
Copy link

CLAassistant commented Jan 6, 2026

CLA assistant check
All committers have signed the CLA.

@derMihai derMihai marked this pull request as ready for review January 6, 2026 10:46
@mantriyogesh
Copy link
Collaborator

Hello @derMihai ,Thank you for the thorough explanation. We appreciate your time in detailing the change.
We will review the code and implement the changes.

While I ponder how to simplify the solution somewhat, to make the change more transport-agnostic.
I would also need to review any other place in other transport same issue is experienced.

Thanks a lot for your efforts.

@derMihai
Copy link
Author

derMihai commented Jan 6, 2026

One solution could be to delegate the skb allocation to the transport layer. We can remove the generic esp_alloc_skb() and add a member skb_realloc() to struct esp_if_ops, which re-allocates any skb from upper layers to meet the transport layer alignment and size requirements. But I cannot help much with SDIO.

@mantriyogesh
Copy link
Collaborator

Hello @derMihai,

Please find the attached modified patch, which sits on top of current master, 711a1b7. Would you be able to test it on your setup and let me know whether it fixes the issue?

Please note that I have not tested this patch myself.

Summary of changes

  • SPI buffer allocation is now done once with a fixed size of 1600 bytes, instead of realloc–memcpy–free later. The approach reuses the TX SKB (which we fully control) as-is.
  • SKB padding has been added for tx_skb->len < 1600 case, so the lower driver always sees a full 1600-byte buffer, consistent with the previous behavior.
  • Mutex and workqueue–related changes were intentionally avoided. These are relatively straightforward and can be addressed in a separate commit; this patch focuses on fixing the current critical bug.

Thanks in advance for your testing and feedback.

@mantriyogesh
Copy link
Collaborator

Oops! Missed to attach the file. PTA:

0001-fix-ng-host-spi-fix-OOB-reads-when-tx-skb-len-SPI_BU.patch

@derMihai
Copy link
Author

derMihai commented Jan 7, 2026

Thank you, this works just fine. Are you taking over to a new pull request?

@mantriyogesh
Copy link
Collaborator

If you are fine with it, I will just merge the patch I had attached with you being author. (Let me know !)

Although, Merging this might need to verify testing on stdio, just to make sure, nothing is broken.

(internal) @Shreyas0-7 , Can you please cross check this on SDIO?

@derMihai
Copy link
Author

derMihai commented Jan 7, 2026

Sure. You don't necessarily have to add me as author, I merely reported the bug.

@mantriyogesh
Copy link
Collaborator

Screenshot 2026-01-21 at 11 30 46 AM

@derMihai , I think I made mistake in last patch shared.
This symbol you exposed in your changes, say from Makefile?

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