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).
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
