[PATCH net] igmp: use the joined device when leaving a multicast group

[PATCH net] igmp: use the joined device when leaving a multicast group

[Junyoung Jang]

ip_mc_leave_group() resolves in_dev from the leave request before
matching the request against the socket's multicast membership list.
This lookup can return NULL even while the interface recorded in the
matched membership (iml->multi.imr_ifindex) is still alive:

1. An application joins a group using a local IPv4 address, that
   secondary address is later removed while the interface stays up,
   and the application leaves using the same 8-byte ip_mreq.
2. An unprivileged user joins a group with a 12-byte ip_mreqn
   specifying a valid imr_ifindex and a nonlocal imr_address, then
   leaves with an 8-byte ip_mreq containing that nonlocal address
   and no ifindex.

Because imr_address is nonzero, the early -ENODEV check is bypassed.
The loop then removes and frees the socket's ip_mc_socklist entry
(iml) and returns 0, but skips ip_mc_dec_group() because in_dev is
NULL. Closing the socket cannot release the group via
ip_mc_drop_socket() because the socket membership entry has already
been unlinked. This leaks the interface's struct ip_mc_list reference
and multicast MAC filter beyond the socket's lifetime, allowing an
unprivileged user to repeatedly leak multicast group entries on a
live interface.

Fix this by looking up in_dev from the matched membership's saved
imr_ifindex under RTNL, matching ip_mc_drop_socket(). If the interface
was genuinely removed, inetdev_by_index() still returns NULL and the
existing hot-unplug cleanup behavior is preserved.

Fixes: 4eba7bb1d72d ("ipv4: igmp: Allow removing groups from a removed interface")
Signed-off-by: Junyoung Jang <graypanda.inzag@gmail.com>
---
Tested on Linux 7.4-rc1 in QEMU/KVM as an unprivileged user (UID 1000)
for both the unprivileged ifindex/nonlocal-address mismatch and the
secondary address removal scenario.

Unpatched 7.4-rc1 guest output:
  IGMP_VM_BEGIN kernel=7.4.0-rc1 expect=--expect-leak
  IGMP_VM_CASE=nonlocal-address
  uid=1000
  control_explicit_ifindex
  after_drop=0
  after_close=0
  case_address_only
  join=0
  drop=0
  after_drop=1
  after_close=1
  joins_ok=16
  drops_ok=16
  closes_ok=16
  groups_after=16
  result=PASS
  IGMP_VM_CASE=removed-secondary-address
  uid=1000
  address_removed=1
  primary_present=1
  after_remove=1
  drop=0
  after_drop=1
  after_close=1
  result=PASS

Patched 7.4-rc1 guest output:
  IGMP_VM_BEGIN kernel=7.4.0-rc1 expect=--expect-fixed
  IGMP_VM_CASE=nonlocal-address
  uid=1000
  control_explicit_ifindex
  after_drop=0
  after_close=0
  case_address_only
  join=0
  drop=0
  after_drop=0
  after_close=0
  joins_ok=16
  drops_ok=16
  closes_ok=16
  groups_after=0
  result=PASS
  IGMP_VM_CASE=removed-secondary-address
  uid=1000
  address_removed=1
  primary_present=1
  after_remove=1
  drop=0
  after_drop=0
  after_close=0
  result=PASS

Reproducer (igmp-multicast-reference-leak.c):

/* SPDX-License-Identifier: GPL-2.0-only */
#define _GNU_SOURCE
#include <arpa/inet.h>
#include <errno.h>
#include <ifaddrs.h>
#include <net/if.h>
#include <netinet/in.h>
#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/socket.h>
#include <unistd.h>

/*
 * Run in a disposable network namespace. The affected kernel retains
 * groups after a successful address-only leave and socket close.
 */
#define BATCH_GROUPS 16

_Static_assert(sizeof(struct ip_mreqn) == 12, "ip_mreqn must include ifindex");
_Static_assert(sizeof(struct ip_mreq) == 8, "ip_mreq must omit ifindex");

struct observation {
	int before;
	int after_join;
	int after_drop;
	int after_close;
};

struct accumulation {
	int before;
	int after;
	int joins;
	int drops;
	int closes;
};

struct interface_state {
	bool up;
	bool primary_present;
	bool additional_present;
};

static void die(const char *operation)
{
	fprintf(stderr, "%s: %s\n", operation, strerror(errno));
	exit(EXIT_FAILURE);
}

static struct in_addr address(const char *text)
{
	struct in_addr addr;

	if (inet_pton(AF_INET, text, &addr) != 1) {
		fprintf(stderr, "invalid IPv4 address: %s\n", text);
		exit(EXIT_FAILURE);
	}
	return addr;
}

static struct interface_state churn_interface_state(void)
{
	struct in_addr primary = address("192.0.2.1");
	struct in_addr additional = address("192.0.2.10");
	struct interface_state state = { 0 };
	struct ifaddrs *interfaces, *ifa;

	if (getifaddrs(&interfaces))
		die("getifaddrs");
	for (ifa = interfaces; ifa; ifa = ifa->ifa_next) {
		struct sockaddr_in *in;

		/* IPv4 aliases may be reported as eth0:1 by getifaddrs(). */
		if (strncmp(ifa->ifa_name, "eth0", 4) ||
		    (ifa->ifa_name[4] && ifa->ifa_name[4] != ':'))
			continue;
		state.up |= !!(ifa->ifa_flags & IFF_UP);
		if (!ifa->ifa_addr || ifa->ifa_addr->sa_family != AF_INET)
			continue;
		in = (struct sockaddr_in *)ifa->ifa_addr;
		state.primary_present |= in->sin_addr.s_addr == primary.s_addr;
		state.additional_present |= in->sin_addr.s_addr == additional.s_addr;
	}
	freeifaddrs(interfaces);
	return state;
}

/* /proc/net/igmp prints each group as a hexadecimal __be32 value. */
static int group_present(unsigned int ifindex, struct in_addr group)
{
	char line[256], device[IFNAMSIZ];
	unsigned int index, group_count, raw_group, users;
	bool on_interface = false;
	int found = 0;
	FILE *file;

	file = fopen("/proc/net/igmp", "r");
	if (!file)
		die("fopen /proc/net/igmp");

	while (fgets(line, sizeof(line), file)) {
		if (sscanf(line, " %u %15s : %u", &index, device,
			   &group_count) == 3) {
			on_interface = index == ifindex;
			continue;
		}
		if (on_interface &&
		    sscanf(line, " %x %u", &raw_group, &users) == 2 &&
		    raw_group == group.s_addr) {
			found = 1;
			break;
		}
	}
	if (ferror(file))
		die("read /proc/net/igmp");
	if (fclose(file))
		die("fclose /proc/net/igmp");
	return found;
}

static struct ip_mreqn join_request(struct in_addr group,
				    struct in_addr interface_addr, int ifindex)
{
	struct ip_mreqn request = {
		.imr_multiaddr = group,
		.imr_address = interface_addr,
		.imr_ifindex = ifindex,
	};

	return request;
}

static struct observation probe(const char *name, struct in_addr group,
				struct in_addr interface_addr,
				unsigned int ifindex, bool drop_with_ifindex)
{
	struct ip_mreqn join = join_request(group, interface_addr, ifindex);
	struct ip_mreq drop = {
		.imr_multiaddr = group,
		.imr_interface = interface_addr,
	};
	struct observation result;
	int fd;

	printf("%s\n", name);
	result.before = group_present(ifindex, group);
	printf("before=%d\n", result.before);
	fd = socket(AF_INET, SOCK_DGRAM | SOCK_CLOEXEC, 0);
	if (fd < 0)
		die("socket");
	printf("socket=%d\n", fd);
	if (setsockopt(fd, IPPROTO_IP, IP_ADD_MEMBERSHIP,
		       &join, sizeof(join)))
		die("IP_ADD_MEMBERSHIP");
	printf("join=0\n");
	result.after_join = group_present(ifindex, group);
	printf("after_join=%d\n", result.after_join);

	if (drop_with_ifindex) {
		if (setsockopt(fd, IPPROTO_IP, IP_DROP_MEMBERSHIP,
			       &join, sizeof(join)))
			die("IP_DROP_MEMBERSHIP (ip_mreqn)");
	} else if (setsockopt(fd, IPPROTO_IP, IP_DROP_MEMBERSHIP,
			      &drop, sizeof(drop))) {
		die("IP_DROP_MEMBERSHIP (ip_mreq)");
	}
	printf("drop=0\n");
	result.after_drop = group_present(ifindex, group);
	printf("after_drop=%d\n", result.after_drop);
	if (close(fd))
		die("close");
	printf("close=0\n");
	result.after_close = group_present(ifindex, group);
	printf("after_close=%d\n", result.after_close);
	return result;
}

static int count_batch_groups(unsigned int ifindex)
{
	char text[INET_ADDRSTRLEN];
	int count = 0;

	for (int i = 1; i <= BATCH_GROUPS; i++) {
		snprintf(text, sizeof(text), "239.250.34.%d", i);
		count += group_present(ifindex, address(text));
	}
	return count;
}

static struct accumulation accumulate_groups(struct in_addr interface_addr,
					     unsigned int ifindex)
{
	char text[INET_ADDRSTRLEN];
	struct accumulation result = { 0 };
	struct ip_mreqn join;
	struct ip_mreq drop;
	int fd;

	result.before = count_batch_groups(ifindex);
	printf("groups_before=%d\n", result.before);
	for (int i = 1; i <= BATCH_GROUPS; i++) {
		snprintf(text, sizeof(text), "239.250.34.%d", i);
		join = join_request(address(text), interface_addr, ifindex);
		drop.imr_multiaddr = join.imr_multiaddr;
		drop.imr_interface = interface_addr;
		fd = socket(AF_INET, SOCK_DGRAM | SOCK_CLOEXEC, 0);
		if (fd < 0)
			die("batch socket");
		if (setsockopt(fd, IPPROTO_IP, IP_ADD_MEMBERSHIP,
			       &join, sizeof(join)))
			die("batch IP_ADD_MEMBERSHIP");
		result.joins++;
		if (setsockopt(fd, IPPROTO_IP, IP_DROP_MEMBERSHIP,
			       &drop, sizeof(drop)))
			die("batch IP_DROP_MEMBERSHIP");
		result.drops++;
		if (close(fd))
			die("batch close");
		result.closes++;
	}
	result.after = count_batch_groups(ifindex);
	printf("joins_ok=%d\ndrops_ok=%d\ncloses_ok=%d\ngroups_after=%d\n",
	       result.joins, result.drops, result.closes, result.after);
	return result;
}

static int run_original(bool expect_leak, unsigned int ifindex)
{
	struct in_addr interface_addr = address("203.0.113.77");
	struct observation control, affected;
	struct accumulation batch;
	bool pass;

	control = probe("control_explicit_ifindex", address("239.250.33.1"),
			interface_addr, ifindex, true);
	affected = probe("case_address_only", address("239.250.33.2"),
			 interface_addr, ifindex, false);
	batch = accumulate_groups(interface_addr, ifindex);
	pass = control.before == 0 && control.after_join == 1 &&
	       control.after_drop == 0 && control.after_close == 0 &&
	       affected.before == 0 && affected.after_join == 1 &&
	       affected.after_drop == expect_leak &&
	       affected.after_close == expect_leak &&
	       batch.before == 0 && batch.joins == BATCH_GROUPS &&
	       batch.drops == BATCH_GROUPS && batch.closes == BATCH_GROUPS &&
	       batch.after == (expect_leak ? BATCH_GROUPS : 0);
	printf("result=%s\n", pass ? "PASS" : "FAIL");
	return pass ? EXIT_SUCCESS : EXIT_FAILURE;
}

/*
 * The caller first adds 192.0.2.1/24 to eth0, then 192.0.2.10/24 as a
 * secondary address. After "ready_to_remove_address=1", it deletes only
 * the secondary address and sends one byte on stdin; eth0 remains up.
 */
static int run_address_churn(bool expect_leak, unsigned int ifindex)
{
	struct in_addr group = address("239.250.35.1");
	struct ip_mreq request = {
		.imr_multiaddr = group,
		.imr_interface = address("192.0.2.10"),
	};
	int before, after_join, after_remove, after_drop, after_close, fd;
	struct interface_state before_state, after_state;
	char token;
	ssize_t n;
	bool pass;

	printf("case_address_churn\n");
	before_state = churn_interface_state();
	printf("address_present_before=%d\ninterface_up_before=%d\n",
	       before_state.additional_present, before_state.up);
	before = group_present(ifindex, group);
	printf("before=%d\n", before);
	fd = socket(AF_INET, SOCK_DGRAM | SOCK_CLOEXEC, 0);
	if (fd < 0)
		die("socket");
	printf("socket=%d\n", fd);
	if (setsockopt(fd, IPPROTO_IP, IP_ADD_MEMBERSHIP,
		       &request, sizeof(request)))
		die("address-churn IP_ADD_MEMBERSHIP");
	printf("join=0\n");
	after_join = group_present(ifindex, group);
	printf("after_join=%d\n", after_join);
	printf("ready_to_remove_address=1\n");
	if (fflush(stdout))
		die("fflush");
	do {
		n = read(STDIN_FILENO, &token, 1);
	} while (n < 0 && errno == EINTR);
	if (n != 1) {
		fprintf(stderr, "expected one-byte address-removal signal\n");
		return EXIT_FAILURE;
	}
	after_state = churn_interface_state();
	printf("address_removed=%d\ninterface_up=%d\nprimary_present=%d\n",
	       !after_state.additional_present, after_state.up,
	       after_state.primary_present);
	after_remove = group_present(ifindex, group);
	printf("after_remove=%d\n", after_remove);
	if (setsockopt(fd, IPPROTO_IP, IP_DROP_MEMBERSHIP,
		       &request, sizeof(request)))
		die("address-churn IP_DROP_MEMBERSHIP");
	printf("drop=0\n");
	after_drop = group_present(ifindex, group);
	printf("after_drop=%d\n", after_drop);
	if (close(fd))
		die("close");
	printf("close=0\n");
	after_close = group_present(ifindex, group);
	printf("after_close=%d\n", after_close);
	pass = before_state.up && before_state.primary_present &&
	       before_state.additional_present && after_state.up &&
	       after_state.primary_present && !after_state.additional_present &&
	       before == 0 && after_join == 1 && after_remove == 1 &&
	       after_drop == expect_leak && after_close == expect_leak;
	printf("result=%s\n", pass ? "PASS" : "FAIL");
	return pass ? EXIT_SUCCESS : EXIT_FAILURE;
}

int main(int argc, char **argv)
{
	bool churn = argc == 3 && !strcmp(argv[1], "--address-churn");
	const char *expectation = churn ? argv[2] : argc == 2 ? argv[1] : NULL;
	const char *ifname = churn ? "eth0" : "lo";
	bool expect_leak;
	unsigned int ifindex;

	if (!expectation ||
	    (strcmp(expectation, "--expect-leak") &&
	     strcmp(expectation, "--expect-fixed"))) {
		fprintf(stderr,
			"usage: %s [--address-churn] --expect-leak|--expect-fixed\n",
			argv[0]);
		return 2;
	}
	expect_leak = !strcmp(expectation, "--expect-leak");
	ifindex = if_nametoindex(ifname);
	if (!ifindex)
		die("if_nametoindex");
	printf("uid=%u\ninterface=%s ifindex=%u\nexpectation=%s\n",
	       (unsigned int)getuid(), ifname, ifindex,
	       expect_leak ? "leak" : "fixed");

	if (churn)
		return run_address_churn(expect_leak, ifindex);
	return run_original(expect_leak, ifindex);
}

 net/ipv4/igmp.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/ipv4/igmp.c b/net/ipv4/igmp.c
index 144fca158..52cde8782 100644
--- a/net/ipv4/igmp.c
+++ b/net/ipv4/igmp.c
@@ -2463,6 +2463,8 @@ int ip_mc_leave_group(struct sock *sk, struct ip_mreqn *imr)
 				iml->multi.imr_address.s_addr)
 			continue;
 
+		/* The drop request may not resolve to the joined device. */
+		in_dev = inetdev_by_index(net, iml->multi.imr_ifindex);
 		(void) ip_mc_leave_src(sk, iml, in_dev);
 
 		*imlp = iml->next_rcu;
-- 
2.43.0

Re: [PATCH net] igmp: use the joined device when leaving a multicast group

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.