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

net: arp: Fix ARP protocol handler to not use Ethernet hdr directly #85955

Merged
merged 1 commit into from
Feb 20, 2025

Conversation

jukkar
Copy link
Member

@jukkar jukkar commented Feb 18, 2025

The ARP protocol handler cannot directly access the Ethernet header because the caller has removed the header already when the handler is called.

@@ -787,14 +787,6 @@ enum net_verdict net_arp_input(struct net_pkt *pkt,
struct net_pkt *reply;
struct in_addr *addr;

if (net_pkt_get_len(pkt) < (sizeof(struct net_arp_hdr) -
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it ok to drop the length check though? I think we should still verify the packet length before accessing the ARP header. It could be simplified to just net_pkt_get_len(pkt) < (sizeof(struct net_arp_hdr) I believe.

Copy link
Member Author

Choose a reason for hiding this comment

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

I dropped it because it would not work any more, but I suppose we can add at least that ARP header len check there.

Copy link
Contributor

@clamattia clamattia left a comment

Choose a reason for hiding this comment

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

Nice patch.

Just an idea:
net_arp_input only uses &eth_hdr->dst and &eth_hdr->src. They could use net_pkt_lladdr_dst(pkt) and net_pkt_lladdr_src(pkt) directly instead, so we need not pass the header as separate pointer?
Note it is also passed to arp_prepare_reply and then unused there. We can probably just remove it there.

@jukkar jukkar added this to the v4.1.0 milestone Feb 18, 2025
@pdgendt pdgendt added the bug The issue is a bug, or the PR is fixing a bug label Feb 18, 2025
@jukkar jukkar force-pushed the fix/arp-l2-handling branch from 9efbd06 to 6d46adc Compare February 18, 2025 18:24
@jukkar
Copy link
Member Author

jukkar commented Feb 18, 2025

Note to self to update the commit message next.
Fixed the commit message to reflect what the commit is doing.

The ARP protocol handler cannot directly access the Ethernet header
because the caller has removed the header already when the handler
is called. So change net_arp_input() and pass source and destination
MAC address there instead of bogus pointer that was pointing to ARP
header instead of Ethernet header. This requires changes to ARP tests.

Signed-off-by: Jukka Rissanen <[email protected]>
@jukkar jukkar force-pushed the fix/arp-l2-handling branch from 6d46adc to 8310457 Compare February 19, 2025 06:36
Copy link
Contributor

@clamattia clamattia left a comment

Choose a reason for hiding this comment

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

lgtm as far as I can tell.

@kartben kartben merged commit 9ba79f0 into zephyrproject-rtos:main Feb 20, 2025
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Networking bug The issue is a bug, or the PR is fixing a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants