[PATCH net] net: mctp: Don't access ifa_index when missing

[PATCH net] net: mctp: Don't access ifa_index when missing

[Matt Johnston]

In mctp_dump_addrinfo, ifa_index can be used to filter interfaces, but
only when the struct ifaddrmsg is provided. Otherwise it will be
comparing to uninitialised memory - reproducible in the syzkaller case from
dhcpd, or busybox "ip addr show".

The kernel MCTP implementation has always filtered by ifa_index, so
existing userspace programs expecting to dump MCTP addresses must
already be passing a valid ifa_index value (either 0 or a real index).

BUG: KMSAN: uninit-value in mctp_dump_addrinfo+0x208/0xac0 net/mctp/device.c:128
 mctp_dump_addrinfo+0x208/0xac0 net/mctp/device.c:128
 rtnl_dump_all+0x3ec/0x5b0 net/core/rtnetlink.c:4380
 rtnl_dumpit+0xd5/0x2f0 net/core/rtnetlink.c:6824
 netlink_dump+0x97b/0x1690 net/netlink/af_netlink.c:2309

Fixes: 583be982d934 ("mctp: Add device handling and netlink interface")
Reported-by: syzbot+e76d52dadc089b9d197f@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/all/68135815.050a0220.3a872c.000e.GAE@google.com/
Reported-by: syzbot+1065a199625a388fce60@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/all/681357d6.050a0220.14dd7d.000d.GAE@google.com/
Signed-off-by: Matt Johnston <matt@codeconstruct.com.au>
---
 net/mctp/device.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/net/mctp/device.c b/net/mctp/device.c
index 8e0724c56723de328592bfe5c6fc8085cd3102fe..7780acdb99dedca1cd6a17e4d6bf917c7f7f370f 100644
--- a/net/mctp/device.c
+++ b/net/mctp/device.c
@@ -117,11 +117,17 @@ static int mctp_dump_addrinfo(struct sk_buff *skb, struct netlink_callback *cb)
 	struct net_device *dev;
 	struct ifaddrmsg *hdr;
 	struct mctp_dev *mdev;
-	int ifindex, rc;
+	int ifindex = 0, rc;
 
-	hdr = nlmsg_data(cb->nlh);
-	// filter by ifindex if requested
-	ifindex = hdr->ifa_index;
+	/* Filter by ifindex if a header is provided */
+	if (cb->nlh->nlmsg_len >= nlmsg_msg_size(sizeof(*hdr))) {
+		hdr = nlmsg_data(cb->nlh);
+		/* Userspace programs providing AF_MCTP must be expecting ifa_index filter
+		 * behaviour, as will those setting strict_check.
+		 */
+		if (hdr->ifa_family == AF_MCTP || cb->strict_check)
+			ifindex = hdr->ifa_index;
+	}
 
 	rcu_read_lock();
 	for_each_netdev_dump(net, dev, mcb->ifindex) {

---
base-commit: ebd297a2affadb6f6f4d2e5d975c1eda18ac762d
change-id: 20250505-mctp-addr-dump-673e0fdc7894

Best regards,
-- 
Matt Johnston <matt@codeconstruct.com.au>

Re: [PATCH net] net: mctp: Don't access ifa_index when missing

[Simon Horman]

On Mon, May 05, 2025 at 05:05:12PM +0800, Matt Johnston wrote:
> In mctp_dump_addrinfo, ifa_index can be used to filter interfaces, but
> only when the struct ifaddrmsg is provided. Otherwise it will be
> comparing to uninitialised memory - reproducible in the syzkaller case from
> dhcpd, or busybox "ip addr show".
> 
> The kernel MCTP implementation has always filtered by ifa_index, so
> existing userspace programs expecting to dump MCTP addresses must
> already be passing a valid ifa_index value (either 0 or a real index).
> 
> BUG: KMSAN: uninit-value in mctp_dump_addrinfo+0x208/0xac0 net/mctp/device.c:128
>  mctp_dump_addrinfo+0x208/0xac0 net/mctp/device.c:128
>  rtnl_dump_all+0x3ec/0x5b0 net/core/rtnetlink.c:4380
>  rtnl_dumpit+0xd5/0x2f0 net/core/rtnetlink.c:6824
>  netlink_dump+0x97b/0x1690 net/netlink/af_netlink.c:2309
> 
> Fixes: 583be982d934 ("mctp: Add device handling and netlink interface")
> Reported-by: syzbot+e76d52dadc089b9d197f@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/all/68135815.050a0220.3a872c.000e.GAE@google.com/
> Reported-by: syzbot+1065a199625a388fce60@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/all/681357d6.050a0220.14dd7d.000d.GAE@google.com/
> Signed-off-by: Matt Johnston <matt@codeconstruct.com.au>

Thanks Matt,

FWIIW, this looks good to me.

Reviewed-by: Simon Horman <horms@kernel.org>

> ---
>  net/mctp/device.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/net/mctp/device.c b/net/mctp/device.c
> index 8e0724c56723de328592bfe5c6fc8085cd3102fe..7780acdb99dedca1cd6a17e4d6bf917c7f7f370f 100644
> --- a/net/mctp/device.c
> +++ b/net/mctp/device.c
> @@ -117,11 +117,17 @@ static int mctp_dump_addrinfo(struct sk_buff *skb, struct netlink_callback *cb)
>  	struct net_device *dev;
>  	struct ifaddrmsg *hdr;
>  	struct mctp_dev *mdev;
> -	int ifindex, rc;
> +	int ifindex = 0, rc;
>  
> -	hdr = nlmsg_data(cb->nlh);
> -	// filter by ifindex if requested
> -	ifindex = hdr->ifa_index;
> +	/* Filter by ifindex if a header is provided */
> +	if (cb->nlh->nlmsg_len >= nlmsg_msg_size(sizeof(*hdr))) {
> +		hdr = nlmsg_data(cb->nlh);

FWIIW, I think the scope of the declaration of hdr can be reduced to this block.
(Less positive ease, so to speak.)

> +		/* Userspace programs providing AF_MCTP must be expecting ifa_index filter
> +		 * behaviour, as will those setting strict_check.
> +		 */
> +		if (hdr->ifa_family == AF_MCTP || cb->strict_check)
> +			ifindex = hdr->ifa_index;
> +	}
>  
>  	rcu_read_lock();
>  	for_each_netdev_dump(net, dev, mcb->ifindex) {
> 
> ---
> base-commit: ebd297a2affadb6f6f4d2e5d975c1eda18ac762d
> change-id: 20250505-mctp-addr-dump-673e0fdc7894
> 
> Best regards,
> -- 
> Matt Johnston <matt@codeconstruct.com.au>
> 

Re: [PATCH net] net: mctp: Don't access ifa_index when missing

[Jakub Kicinski]

On Tue, 6 May 2025 17:07:53 +0100 Simon Horman wrote:
> > +	if (cb->nlh->nlmsg_len >= nlmsg_msg_size(sizeof(*hdr))) {
> > +		hdr = nlmsg_data(cb->nlh);  
> 
> FWIIW, I think the scope of the declaration of hdr can be reduced to this block.
> (Less positive ease, so to speak.)

We wouldn't be able to sizeof(*hdr) if we move it?

I have a different request :) Matt, once this ends up in net-next
(end of this week) could you refactor it to use nlmsg_payload() ?
It doesn't exist in net but this is exactly why it was added.

Re: [PATCH net] net: mctp: Don't access ifa_index when missing

[Jakub Kicinski]

On Mon, 05 May 2025 17:05:12 +0800 Matt Johnston wrote:
> +		/* Userspace programs providing AF_MCTP must be expecting ifa_index filter
> +		 * behaviour, as will those setting strict_check.
> +		 */
> +		if (hdr->ifa_family == AF_MCTP || cb->strict_check)
> +			ifindex = hdr->ifa_index;

The use of cb->strict_check is a bit strange here. I could be wrong but
I though cb->strict_check should only impact validation. Not be used
for changing behavior.

If you have a reason to believe all user space passes a valid header -
how about we just return an error if message is too short?
IPv4 and IPv6 seem to return an error if message is short and
cb->strict_check, so they are more strict. MCTP doesn't have a ton of
legacy user space, we don't have to be lenient at all. My intuition
would be to always act like IP acts under cb->strict_check

Re: [PATCH net] net: mctp: Don't access ifa_index when missing

[Matt Johnston]

On Tue, 2025-05-06 at 18:06 -0700, Jakub Kicinski wrote:
> On Mon, 05 May 2025 17:05:12 +0800 Matt Johnston wrote:
> > +		/* Userspace programs providing AF_MCTP must be expecting ifa_index filter
> > +		 * behaviour, as will those setting strict_check.
> > +		 */
> > +		if (hdr->ifa_family == AF_MCTP || cb->strict_check)
> > +			ifindex = hdr->ifa_index;
> 
> The use of cb->strict_check is a bit strange here. I could be wrong but
> I though cb->strict_check should only impact validation. Not be used
> for changing behavior.

It was following behaviour of inet_dump_addr()/inet6_dump_addr() where
filtering is applied if strict check is set.
I don't have strong opinion whether strict_check makes sense for MCTP though
- it depends on
whether userspace expects strict_check to apply to all families, or just
inet4/inet6.

> If you have a reason to believe all user space passes a valid header -
> how about we just return an error if message is too short?
> IPv4 and IPv6 seem to return an error if message is short and
> cb->strict_check, so they are more strict. MCTP doesn't have a ton of
> legacy user space, we don't have to be lenient at all. My intuition
> would be to always act like IP acts under cb->strict_check

The problem is that programs will pass ifa_family=AF_UNSPEC with a short
header, no strict_check 
(eg busybox "ip addr show").
An AF_UNSPEC request will reach mctp_dump_addrinfo(), so we don't want that
returning an error.
Maybe mctp_dump_addrinfo() should ignore AF_UNSPEC requests entirely, and
only populate
a response when ifa_family=AF_MCTP. That would be OK for the existing mctp
userspace programs 
I know about, though there may be other users that are calling with AF_UNSPEC
but filtering 
userspace-side for AF_MCTP addresses.

Matt

Re: [PATCH net] net: mctp: Don't access ifa_index when missing

[Jakub Kicinski]

On Wed, 07 May 2025 09:24:29 +0800 Matt Johnston wrote:
> On Tue, 2025-05-06 at 18:06 -0700, Jakub Kicinski wrote:
> > On Mon, 05 May 2025 17:05:12 +0800 Matt Johnston wrote:  
> > > +		/* Userspace programs providing AF_MCTP must be expecting ifa_index filter
> > > +		 * behaviour, as will those setting strict_check.
> > > +		 */
> > > +		if (hdr->ifa_family == AF_MCTP || cb->strict_check)
> > > +			ifindex = hdr->ifa_index;  
> > 
> > The use of cb->strict_check is a bit strange here. I could be wrong but
> > I though cb->strict_check should only impact validation. Not be used
> > for changing behavior.  
> 
> It was following behaviour of inet_dump_addr()/inet6_dump_addr() where
> filtering is applied if strict check is set.
> I don't have strong opinion whether strict_check makes sense for MCTP though
> - it depends on
> whether userspace expects strict_check to apply to all families, or just
> inet4/inet6.

I see your point. And existing user space may expect filtering
even if !cb->strict_check but family is set to AF_MCTP?

> > If you have a reason to believe all user space passes a valid header -
> > how about we just return an error if message is too short?
> > IPv4 and IPv6 seem to return an error if message is short and
> > cb->strict_check, so they are more strict. MCTP doesn't have a ton of
> > legacy user space, we don't have to be lenient at all. My intuition
> > would be to always act like IP acts under cb->strict_check  
> 
> The problem is that programs will pass ifa_family=AF_UNSPEC with a short
> header, no strict_check 
> (eg busybox "ip addr show").
> An AF_UNSPEC request will reach mctp_dump_addrinfo(), so we don't want that
> returning an error.
> Maybe mctp_dump_addrinfo() should ignore AF_UNSPEC requests entirely, and
> only populate
> a response when ifa_family=AF_MCTP. That would be OK for the existing mctp
> userspace programs 
> I know about, though there may be other users that are calling with AF_UNSPEC
> but filtering 
> userspace-side for AF_MCTP addresses.

Right, and looks like IP filters with strict_check regardless of family.
So we'd be even further away from that behavior if we never filtered
with AF_UNSPEC.

Re: [PATCH net] net: mctp: Don't access ifa_index when missing

[Matt Johnston]

On Tue, 2025-05-06 at 18:41 -0700, Jakub Kicinski wrote:
> On Wed, 07 May 2025 09:24:29 +0800 Matt Johnston wrote:
> > On Tue, 2025-05-06 at 18:06 -0700, Jakub Kicinski wrote:
> > > On Mon, 05 May 2025 17:05:12 +0800 Matt Johnston wrote:  
> > > > +		/* Userspace programs providing AF_MCTP must be expecting ifa_index filter
> > > > +		 * behaviour, as will those setting strict_check.
> > > > +		 */
> > > > +		if (hdr->ifa_family == AF_MCTP || cb->strict_check)
> > > > +			ifindex = hdr->ifa_index;  
> > > 
> > > The use of cb->strict_check is a bit strange here. I could be wrong but
> > > I though cb->strict_check should only impact validation. Not be used
> > > for changing behavior.  
> > 
> > It was following behaviour of inet_dump_addr()/inet6_dump_addr() where
> > filtering is applied if strict check is set.
> > I don't have strong opinion whether strict_check makes sense for MCTP though
> > - it depends on
> > whether userspace expects strict_check to apply to all families, or just
> > inet4/inet6.
> 
> I see your point. And existing user space may expect filtering
> even if !cb->strict_check but family is set to AF_MCTP?

Yes, given mctp_dump_addrinfo() has always applied a filter, mctp-specific
programs likely expect that behaviour.

> > > If you have a reason to believe all user space passes a valid header -
> > > how about we just return an error if message is too short?
> > > IPv4 and IPv6 seem to return an error if message is short and
> > > cb->strict_check, so they are more strict. MCTP doesn't have a ton of
> > > legacy user space, we don't have to be lenient at all. My intuition
> > > would be to always act like IP acts under cb->strict_check  
> > 
> > The problem is that programs will pass ifa_family=AF_UNSPEC with a short
> > header, no strict_check 
> > (eg busybox "ip addr show").
> > An AF_UNSPEC request will reach mctp_dump_addrinfo(), so we don't want that
> > returning an error.
> > Maybe mctp_dump_addrinfo() should ignore AF_UNSPEC requests entirely, and
> > only populate
> > a response when ifa_family=AF_MCTP. That would be OK for the existing mctp
> > userspace programs 
> > I know about, though there may be other users that are calling with AF_UNSPEC
> > but filtering 
> > userspace-side for AF_MCTP addresses.
> 
> Right, and looks like IP filters with strict_check regardless of family.
> So we'd be even further away from that behavior if we never filtered
> with AF_UNSPEC.

Re: [PATCH net] net: mctp: Don't access ifa_index when missing

[Jakub Kicinski]

On Wed, 07 May 2025 10:13:19 +0800 Matt Johnston wrote:
> > I see your point. And existing user space may expect filtering
> > even if !cb->strict_check but family is set to AF_MCTP?  
> 
> Yes, given mctp_dump_addrinfo() has always applied a filter, mctp-specific
> programs likely expect that behaviour.

Okay, so would this make all known user space happy?

	if (!msg short) {
		ifindex = ifm->ifa_index
	} else {
		if (cb->strict_check)
			return error
	}

I suspect busybox doesn't set strict check, otherwise it'd trip up in IP

Re: [PATCH net] net: mctp: Don't access ifa_index when missing

[Matt Johnston]

On Tue, 2025-05-06 at 19:20 -0700, Jakub Kicinski wrote:
> On Wed, 07 May 2025 10:13:19 +0800 Matt Johnston wrote:
> > > I see your point. And existing user space may expect filtering
> > > even if !cb->strict_check but family is set to AF_MCTP?  
> > 
> > Yes, given mctp_dump_addrinfo() has always applied a filter, mctp-specific
> > programs likely expect that behaviour.
> 
> Okay, so would this make all known user space happy?
> 
> 	if (!msg short) {
> 		ifindex = ifm->ifa_index
> 	} else {
> 		if (cb->strict_check)
> 			return error
> 	}

I think that would work well. Some old non-mctp programs might send a full
header but garbage ifa_index (the original reason for strict_check), but that
would just filter out some interfaces which should be OK - that userspace
wouldn't be handling AF_MCTP responses anyway. I'll give it some testing and
get a v2. Thanks for the review.

I'll have a look at nlmsg_payload() for later. 

Cheers,
Matt

Re: [PATCH net] net: mctp: Don't access ifa_index when missing

[Simon Horman]

On Tue, May 06, 2025 at 05:58:30PM -0700, Jakub Kicinski wrote:
> On Tue, 6 May 2025 17:07:53 +0100 Simon Horman wrote:
> > > +	if (cb->nlh->nlmsg_len >= nlmsg_msg_size(sizeof(*hdr))) {
> > > +		hdr = nlmsg_data(cb->nlh);  
> > 
> > FWIIW, I think the scope of the declaration of hdr can be reduced to this block.
> > (Less positive ease, so to speak.)
> 
> We wouldn't be able to sizeof(*hdr) if we move it?

Yes, of course.
Sorry for missing that.

> 
> I have a different request :) Matt, once this ends up in net-next
> (end of this week) could you refactor it to use nlmsg_payload() ?
> It doesn't exist in net but this is exactly why it was added.
>