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?
>> 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
