[BUG] Chip Stack Protection in LwIP UDP receive path

[wy-hh]

Reproduction steps

#25485 enables protection to check PacketBufferHandle accessing and LwIP UDP receive path doesn't require LockChipStack/UnlockChipStack for PacketBufferHandle protection.
So our Wi-Fi platform gets assert on PacketBufferHandle allocation when a UDP packet received,

Is that OK to add LockChipStack()/UnlockChipStack() in UDPEndPointImplLwIP::LwIPReceiveUDPMessage to protect PacketBufferHandle access?

Here is call stack when our platform hit this unsafe access.

#0  vAssertCalled () at ../../examples/lighting-app/bouffalolab/bl602/third_party/connectedhomeip/examples/platform/bouffalolab/common/plat/main.cpp:191
#1  0x2303783c in abort () at ../../examples/lighting-app/bouffalolab/bl602/third_party/connectedhomeip/third_party/bouffalolab/repo/components/libc/newlibc/assert.c:55
#2  0x230845bc in chipAbort () at ../../examples/lighting-app/bouffalolab/bl602/third_party/connectedhomeip/src/lib/support/CodeUtils.h:518
#3  chipDie () at ../../examples/lighting-app/bouffalolab/bl602/third_party/connectedhomeip/src/lib/support/CodeUtils.h:518
#4  chip::Platform::Internal::AssertChipStackLockedByCurrentThread (file=<optimized out>, line=70) at ../../examples/lighting-app/bouffalolab/bl602/third_party/connectedhomeip/src/platform/LockTracker.cpp:36
#5  0x23084ba8 in chip::System::Stats::GetResourcesInUse () at ../../examples/lighting-app/bouffalolab/bl602/third_party/connectedhomeip/src/system/SystemStats.cpp:70
#6  0x23084bf4 in chip::System::Stats::UpdateLwipPbufCounts () at ../../examples/lighting-app/bouffalolab/bl602/third_party/connectedhomeip/src/system/SystemStats.cpp:135
#7  0x230848b8 in chip::System::PacketBufferHandle::New (aAvailableSize=<optimized out>, aReservedSize=aReservedSize@entry=0) at ../../examples/lighting-app/bouffalolab/bl602/third_party/connectedhomeip/src/system/SystemPacketBuffer.cpp:478
#8  0x230aee72 in chip::Inet::UDPEndPointImplLwIP::LwIPReceiveUDPMessage (arg=0x4201929c <chip::DeviceLayer::Internal::GenericConnectivityManagerImpl_UDP<chip::DeviceLayer::ConnectivityManagerImpl>::_UDPEndPointManager()::sUDPEndPointManagerImpl+120>, pcb=0x42048250 <memp_memory_UDP_PCB_base+72>, p=0x4203b95c <rx_payload_desc+28>, addr=0x42014160 <ip_data+20>, port=<optimized out>) at ../../examples/lighting-app/bouffalolab/bl602/third_party/connectedhomeip/src/inet/UDPEndPointImplLwIP.cpp:423
#9  0x23040086 in udp_input (p=p@entry=0x4203b95c <rx_payload_desc+28>, inp=inp@entry=0x42020cbc <wifiMgmr+40>) at ../../examples/lighting-app/bouffalolab/bl602/third_party/connectedhomeip/third_party/bouffalolab/repo/components/network/lwip/src/core/udp.c:404
#10 0x230443e4 in ip4_input (p=p@entry=0x4203b95c <rx_payload_desc+28>, inp=inp@entry=0x42020cbc <wifiMgmr+40>) at ../../examples/lighting-app/bouffalolab/bl602/third_party/connectedhomeip/third_party/bouffalolab/repo/components/network/lwip/src/core/ipv4/ip4.c:703
#11 0x23037e4e in ethernet_input (p=0x4203b95c <rx_payload_desc+28>, netif=0x42020cbc <wifiMgmr+40>) at ../../examples/lighting-app/bouffalolab/bl602/third_party/connectedhomeip/third_party/bouffalolab/repo/components/network/lwip/src/netif/ethernet.c:186
#12 0x2304b1ce in tcpip_thread_handle_msg (msg=0x42046d50 <memp_memory_TCPIP_MSG_INPKT_base+496>) at ../../examples/lighting-app/bouffalolab/bl602/third_party/connectedhomeip/third_party/bouffalolab/repo/components/network/lwip/src/api/tcpip.c:175
#13 0x2304b21a in tcpip_thread (arg=<optimized out>) at ../../examples/lighting-app/bouffalolab/bl602/third_party/connectedhomeip/third_party/bouffalolab/repo/components/network/lwip/src/api/tcpip.c:149

Bug prevalence

whenever I do this

GitHub hash of the SDK that was being used

8afc1f5

Platform

other

Platform Version(s)

No response

Anything else?

No response

[bzbarsky-apple]

@vivien-apple @tcarmelveilleux @wqx6 @kpschoedel The LwIP bits absolutely rely on allocating packetbuffers in LwIPReceiveUDPMessage, which is very much not on the Matter main thread.

The options seems to be:

1. Allow races on the stats (and possible corruption of the stats), remove the asserts Use assertChipStackLockedByCurrentThread when accessing statics in src/system/SystemStats.cpp #25485 aded.
2. Add some sort of narrowly-scoped mutex to the stats themselves, to make sure that access to them is serialized. Not sure what we have in the tree that is cross-platform in the way of mutexes.
3. Work only with struct pbuf in the receive path (including for making the copies, etc) and only create the PacketBufferHandle once we are on the Matter thread.
4. Add lock/unlock of the Matter stack around the packet buffer manipulation.

Any other ideas?

[tcarmelveilleux]

The lwIP pbuf allocation is already thread safe. The stat race should be allowed.

[EliRibble]

./chip-tool pairing ble-wifi 1234 myssid mywifipassword 20202021 3840

Get the following output:

...
[1678495324.096918][119298:119299] CHIP:BLE: Device 41:19:A8:99:58:DE does not look like a CHIP device.
[1678495324.099635][119298:119299] CHIP:BLE: New device scanned: C3:0E:D6:37:1F:41
[1678495324.099688][119298:119299] CHIP:BLE: Device discriminator match. Attempting to connect.
[1678495324.099720][119298:119299] CHIP:DL: Chip stack locking error at '../../examples/chip-tool/third_party/connectedhomeip/src/system/SystemStats.cpp:70'. Code is unsafe/racy
[1678495324.099744][119298:119299] CHIP:-: chipDie chipDie chipDie
Aborted

[tcarmelveilleux]

This is causing crashes on Linux and should be backed out. The cost far outweigh the benefits. @vivien-apple