Eelco Chaudron <echau...@redhat.com> writes:
> On 23 Jun 2025, at 15:10, Aaron Conole wrote:
>
>> Eelco Chaudron <echau...@redhat.com> writes:
>>
>>> 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?
>>
>> Sure - here's the definition of mld2_record::
>>
>> #define MLD2_RECORD_LEN 20
>> struct mld2_record {
>> uint8_t type;
>> uint8_t aux_len;
>> ovs_be16 nsrcs;
>> union ovs_16aligned_in6_addr maddr;
>> };
>> BUILD_ASSERT_DECL(MLD2_RECORD_LEN == sizeof(struct mld2_record));
>>
>> Notice that mld2_record already contains a copy of
>> ovs_16aligned_in6_addr type. That type looks like::
>>
>> union ovs_16aligned_in6_addr {
>> ovs_be16 be16[8];
>> ovs_16aligned_be32 be32[4];
>> };
>>
>> So I suggest adding::
>> BUILD_ASSERT_DECL(sizeof(ovs_16aligned_in6_addr) >= sizeof(struct
>> in6_addr))
>>
>> mld2_record contains a copy, so it should always be >= the size, and
>> therefore we should have a correct structure.
>>
>> Does it make sense?
>
> Thanks, I understand now, your suggestion would also work.
>
> The reason I placed the BUILD_ASSERT() directly in the function was to
> serve as a local safeguard: if anyone ever adds a new case later on,
> the assertion helps ensure the addr access logic still makes sense,
> i.e. the required size assumption still holds, or else the if (!addr)
> would never trigger as expected.
>
> Your proposed BUILD_ASSERT_DECL in packets.h assumes knowledge about
> the internal structure of mld2_record, specifically that it already
> includes an IPv6 addr. This ties the check to the structure details
> rather than the functional logic in this code.
>
> My intent with the current placement is to keep the check closely tied
> to the usage, making it clearer why it matters in this specific
> control path, rather than relying on assumptions elsewhere.
>
> So, my preferred way forward would be to keep the patch as is.
>
> Thoughts?
Wouldn't that make more sense at the point we use the mld2_record type
(in the MLD2_REPORT case)? After all, 'addr' in this case does not seem
to be directly tied to the mld2 record type (since it's just a pointer
to data in the packet). And the `if (!addr)` check you introduced
should catch the basic addr offset issues.
Maybe I'm missing something, but this particular BUILD_ASSERT seems to
tie the structure details tighter here. Going further, mld2 records can
have multiple ip6 address blocks so maybe it could be reasonable to
change the structure in the future - and then this part would have to be
removed (because with this patch it is a tight coupling).
Not sure - I don't want to bog the patch discussion down too much in the
weeds, but I still don't get this particular assert in this location.
> //Eelco
>
>>>> 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
