eclipse-threadx / netxduo

Eclipse ThreadX - NetXDuo is an advanced, industrial-grade TCP/IP network stack designed specifically for deeply embedded real-time and IoT applications
https://github.com/eclipse-threadx/rtos-docs/blob/main/rtos-docs/netx-duo/index.md
MIT License
242 stars 137 forks source link

_nx_ip_checksum_compute calculates invalid checksum when packet chunk is not aligned to machine natural boundary #146

Open elrafoon opened 1 year ago

elrafoon commented 1 year ago

Description

My target system:

Function _nx_ip_checksum_compute calculates invalid checksum when packet chunk is not aligned to machine natural boundary (4 or 8 bytes).

Packet chunk is defined by pointers nx_packet_prepend_ptr and nx_packet_append_ptr.

I run to this when debugging ICMP packets being dropped on Renesas RA6M3 MCU (it's cortex-m4) - IPv4 header checksums were incorrectly calculated by _nx_ip_checksum_compute.

There is no way to guarantee that nx_packet_prepend_ptr will point to 4-byte aligned memory location, because that ipv4 header is part of ethernet packet received.

Packet looks like this:

So ipv4 header always starts on offset 14. Since packet received by ethernet mac is stored on 32-byte aligned memory location, after adding that offset it's impossible for ipv4 header to start on 4-byte aligned memory location.

To reproduce

To ease bug reproduction I wrote small C program nx_ip_cs_test.c containing just the function _nx_ip_checksum_compute and a few typedefs. nx_ip_cs_test.tar.gz

I also added very simple implementation of ipv4 checksum in function ipv4_simple_crc_calculate which works in all cases (but its performance is suboptimal).

Program calculates checksum of a sample IPv4 header located on misaligned memory location using both _nx_ip_checksum_compute and ipv4_simple_crc_calculate, prints the result and asserts if calculated inverted checksum is non-zero. (ipv4 header checksum calculation shall yield zero after final bit inversion, because the header already contains the crc).

To reproduce, just compile and run the program (you can use run.sh script). I tried x86, x86-64 and ARM Cortex-A7 (using iMX6UL machine). With optimizations enabled (-O3), it fails on all three architectures. With optimizations disabled (-O0), it succeeds only on x86-64.

Expected behaviour

_nx_ip_checksum_compute shall yield 0xFFFF for correct ipv4 headers.

Impact

IPv4 does not work at all.

elrafoon commented 1 year ago

I made a fix for myself, until this is resolved somehow.

It certainly has lower performance then original 32-bit-addition code, but works: fix-nx_ip_checksum_compute.tar.gz

goldscott commented 1 year ago

Can you tag Renesas here too please? I am not sure who wrote the network driver (Renesas or Microsoft), but it sounds like maybe the driver isn't inserting padding or taking the alignment into account...

elrafoon commented 1 year ago

I would gladly tag Renesas, but I don't know how to do that. Could you help me with that please?

But essentially, how can network driver guarantee that IPv4 header is 4-byte aligned? It's embedded in a ethernet packet received and driver can't do anything about it - receive buffer is 4-byte aligned, but that doesn't help. Also in this case IPv4 header is followed by ICMP payload, so padding the header from back isn't possible either.

Also, IPv4 header can be received from ppp link, or from some kind of tunnel, where 4-byte alignment of the header can not be forced.

hwmaier commented 1 year ago

We have been using the Renesas Ethernet drivers and never had that issue, FSP related issues can be logged here https://github.com/renesas/fsp

elrafoon commented 1 year ago

I don't think this has anything to do with renesas driver - did you read my reasoning?

And why would the test program containing just _nx_ip_checksum_compute function and main() fail? (did you look at it?)

Maybe someone modified _nx_ip_checksum_compute function recently, but it's impossible to tell from git-log of this repository.

hwmaier commented 1 year ago

@elrafoon I read your reasoning but I also know that we had no issues with running TCP/IP protocol on Ethernet and that's the experience I wanted to share as fellow user. I checked our FSP configuration which shows the driver to be configured to add 2 bytes of padding at offset 14 which is the default setting by Renesas. Maybe try this setting. Otherwise ask in the Renesas forums, Renesas is very helpful and responsive with their support.

2023-01-27_19-21-37

elrafoon commented 1 year ago

Thank you @hwmaier, it shall work with this padding, I will try it. I'm new to Renesas hardware, still learning, didn't know about this.

But still I think that depending on 4-byte alignment in _nx_ip_checksum_compute is wrong - do you think it's always possible to guarantee such alignment? Not just in ethernet case, but in general? (ppp, tunnels, ip-in-something, etc.) (I think it's not).

Also, if such alignment is strictly required, then _nx_ip_checksum_compute should assert that, at least in debug build, not just fail silently.

TiejunMS commented 1 year ago

Good suggestion, @elrafoon ! We will add such assert in the future.