On 23 Jun 2025, at 14:37, Aaron Conole wrote:
> Eelco Chaudron via dev <ovs-dev@openvswitch.org> writes:
>
>> If an MLD packet is not large enough to contain the
>> message-specific data, it may lead to a NULL pointer access.
>> This patch fixes the issue by adding appropriate length checks.
>>
>> Fixes: 06994f879c9d ("mcast-snooping: Add Multicast Listener Discovery
>> support")
>> Signed-off-by: Eelco Chaudron <echau...@redhat.com>
>> ---
>
> Hi Eelco,
>
>> lib/mcast-snooping.c | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/lib/mcast-snooping.c b/lib/mcast-snooping.c
>> index b279c1229..1f290083e 100644
>> --- a/lib/mcast-snooping.c
>> +++ b/lib/mcast-snooping.c
>> @@ -543,6 +543,14 @@ mcast_snooping_add_mld(struct mcast_snooping *ms,
>> offset += MLD_HEADER_LEN;
>> addr = dp_packet_at(p, offset, sizeof(struct in6_addr));
>>
>> + if (!addr) {
>> + /* We error out if the provided packet is not large enough to handle
>> + * the types below. The BUILD_ASSERT() ensures that we can always
>> reach
>> + * the MLD2_REPORT type. */
>> + BUILD_ASSERT(sizeof(struct mld2_record) > sizeof(struct in6_addr));
>
> Would it make sense instead to have a BUILD_ASSERT_DECL in lib/packets.h
> that covers the ovs_16aligned_in6_addr type? I think it might make it a
> bit more readable (it's a bit jarring to see the build-time assert
> here). Then the mld2_record should be covered by default (since the
> condition for sizeof union ovs_16aligned_in6_addr >= struct in6_addr
> should carry forward).
Thanks for the suggestion. I'm not sure I fully follow the idea of moving the
BUILD_ASSERT_DECL to lib/packets.h. From my understanding, the assertion isn't
related to the layout or size of the packet structures in general, but rather
to the specific logic flow in this function.
In particular, the addr variable is only meaningful in two of the three switch
cases. The build-time assertion ensures that the third case is valid —
specifically, that sizeof(struct mld2_record) is always greater than or equal
to sizeof(struct in6_addr), which is required for the access pattern used later
in the code.
So while I agree the assert is a bit odd inline, it seems context-specific
enough that putting it in a shared header like packets.h might reduce clarity
rather than improve it.
Thoughts?
> Otherwise, the change looks good to me.
>
>> + return 0;
>> + }
>> +
>> switch (mld->type) {
>> case MLD_REPORT:
>> ret = mcast_snooping_add_group(ms, addr, vlan, port,
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
