Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib: fix a Null Pointer Dereference bug in sockopt.c #18067

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mugitya03
Copy link

Dear Developers,

We found a potential Null Pointer Dereference bug in file lib/sockopt.c. Here are the details:

The function getsockopt_cmsg_data can return a NULL.

static void *getsockopt_cmsg_data(struct msghdr *msgh, int level, int type)
{
	struct cmsghdr *cmsg;

	for (cmsg = CMSG_FIRSTHDR(msgh); cmsg != NULL;
	     cmsg = CMSG_NXTHDR(msgh, cmsg))
		if (cmsg->cmsg_level == level && cmsg->cmsg_type == type)
			return CMSG_DATA(cmsg);

	return NULL;
}

The null value returned by function getsockopt_cmsg_data is dereferenced in function getsockopt_ipv6_ifindex

static int getsockopt_ipv6_ifindex(struct msghdr *msgh)
{
	struct in6_pktinfo *pktinfo;

	pktinfo = getsockopt_cmsg_data(msgh, IPPROTO_IPV6, IPV6_PKTINFO);

	return pktinfo->ipi6_ifindex;
}

Thus, we add a null value check and return 0 if pointer pktinfo is null.

@Jafaral
Copy link
Member

Jafaral commented Feb 8, 2025

@Mergifyio backport dev/10.3 stable/10.2 stable/10.1 stable/10.0

Copy link

mergify bot commented Feb 8, 2025

backport dev/10.3 stable/10.2 stable/10.1 stable/10.0

🟠 Waiting for conditions to match

  • merged [📌 backport requirement]

@eqvinox
Copy link
Contributor

eqvinox commented Feb 11, 2025

For this to happen, basically the kernel must be broken such that IPV6_PKTINFO does not work correctly. If the kernel is not working correctly, we can just close up shop and go home. Agreement on the community call is that we should in fact crash if this happens.

If anything, you can replace this with an assert or assertf, but even that is somewhat of a superfluous change.

Copy link
Contributor

@eqvinox eqvinox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as commented

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants