Bug in the RTM_SETLINK kernel API for setting MAC address

Bug in the RTM_SETLINK kernel API for setting MAC address

[Pavlin Radoslavov]

It appears there is a bug in the RTM_SETLINK kernel API for setting
the MAC address on an interface.

E.g., below is the relevant sample code for setting the Ethernet MAC
address payload that works on 2.6.17.

    /* Add the MAC address as an attribute */
    struct sockaddr_storage ss_mac;
    struct sockaddr* sa_mac_p = (struct sockaddr *)&ss_mac;
    size_t sa_mac_len = 0;
    memset(&ss_mac, 0, sizeof(ss_mac));
    sa_mac_p->sa_family = ARPHRD_ETHER;
    sa_mac_len = sizeof(sa_mac_p->sa_family) + ETH_ALEN;

    memcpy(sa_mac_p->sa_data, &ether_addr, ETH_ALEN);
    rta_len = RTA_LENGTH(sa_mac_len);
    rtattr = IFLA_RTA(ifinfomsg);
    rtattr->rta_type = IFLA_ADDRESS;
    /*
     * XXX
     * rtattr->rta_len = rta_len;
     */
    rtattr->rta_len = RTA_LENGTH(ETH_ALEN);
    memcpy(RTA_DATA(rtattr), sa_mac_p, sa_mac_len);
    nlh->nlmsg_len = NLMSG_ALIGN(nlh->nlmsg_len) + rta_len;

    if (ns.sendto(buffer, nlh->nlmsg_len, 0, (struct sockaddr *)&snl,
                  sizeof(snl)) != (ssize_t)nlh->nlmsg_len) {
        /* ERROR */
    }

Note that the payload with the MAC address has to be
"struct sockaddr" (or equivalent) and the length of that payload is
the equivalent of "sizeof(sa_family) + mac_address_size".

However, the rta_len of the corresponding message MUST be set to
"mac_address_size" rather than the real payload size which is
"sizeof(sa_family) + mac_address_size".
I believe this is incorrect, and rta_len is suppose to be set to the
real payload size.

The particular problematic code in the kernel that checks for the
    payload size is inside net/core/rtnetlink.c, do_setlink():

    ...
    if (ida[IFLA_ADDRESS - 1]) {
        ...
        if (ida[IFLA_ADDRESS - 1]->rta_len != RTA_LENGTH(dev->addr_len))
            goto out;
        err = dev->set_mac_address(dev, RTA_DATA(ida[IFLA_ADDRESS - 1]));
        ...


Where dev->set_mac_address() (typically/always?) expects to see a
second argument of type "struct sockaddr".

Thanks,
Pavlin

P.S. Please CC to me in your replies, because I am not subscribed to
the list.

Re: Bug in the RTM_SETLINK kernel API for setting MAC address

[David Miller]

From: Pavlin Radoslavov <pavlin@icir.org>
Date: Mon, 07 Aug 2006 20:40:24 -0700

> Note that the payload with the MAC address has to be
> "struct sockaddr" (or equivalent) and the length of that payload is
> the equivalent of "sizeof(sa_family) + mac_address_size".

It should just be the MAC address, that's why the kernel side
is coded the way it is.

Where does this sockaddr come from?

I don't see how it could work with the sockaddr there in
2.6.17, as 2.6.17 makes the same exact length check:

		if (ida[IFLA_ADDRESS - 1]->rta_len != RTA_LENGTH(dev->addr_len))
			goto out;

Re: Bug in the RTM_SETLINK kernel API for setting MAC address

[Pavlin Radoslavov]

> > Note that the payload with the MAC address has to be
> > "struct sockaddr" (or equivalent) and the length of that payload is
> > the equivalent of "sizeof(sa_family) + mac_address_size".
> 
> It should just be the MAC address, that's why the kernel side
> is coded the way it is.

I couldn't find any documentation about the API, so I wasn't sure
whether it is actually suppose to be the MAC address or "sockaddr".

In fact, earlier version of our userland code was assuming it
is just the MAC address, until we found that it doesn't work on
recent kernels.

> Where does this sockaddr come from?

The set_mac_address() functions for each network device driver make
that assumption:

static int set_mac_address(struct net_device *dev, void *p)
{
        int i;
        struct sockaddr *addr = p;

> I don't see how it could work with the sockaddr there in
> 2.6.17, as 2.6.17 makes the same exact length check:
> 
> 		if (ida[IFLA_ADDRESS - 1]->rta_len != RTA_LENGTH(dev->addr_len))
> 			goto out;

Note that in my example rta_len was tweaked to match the MAC address
size, but the the payload itself and the netlink message nlmsg_len
actually match the sockaddr alignment.

The real mismatch is that ida[IFLA_ADDRESS - 1] is (as you say)
suppose to be a MAC address, but the set_mac_address() functions
for each device assume that the RTA_DATA(ida[IFLA_ADDRESS - 1])
payload is a sockaddr.

Pavlin

Re: Bug in the RTM_SETLINK kernel API for setting MAC address

[David Miller]

From: Pavlin Radoslavov <pavlin@icir.org>
Date: Mon, 07 Aug 2006 22:31:59 -0700

> The real mismatch is that ida[IFLA_ADDRESS - 1] is (as you say)
> suppose to be a MAC address, but the set_mac_address() functions
> for each device assume that the RTA_DATA(ida[IFLA_ADDRESS - 1])
> payload is a sockaddr.

That's because ->set_mac_address() is usually invoked via
dev_set_mac_address() which in turn is invoked from places
SIOCSIFHWADDR ioctl() processing which does want the sockaddr
wrapped around the MAC address.

So the netlink code is definitely doing the wrong thing if
it wants merely the MAC address in the attribute.

Since changing all the drivers is a pain, what we probably
should do is have the netlink code allocate a sockaddr,
place the MAC address attribute in to that allocated sockaddr,
and pass it into ->set_mac_address().

This patch should do the trick, can you test it out?

Thanks.

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 20e5bb7..30cc1ba 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -394,6 +394,9 @@ static int do_setlink(struct sk_buff *sk
 	}
 
 	if (ida[IFLA_ADDRESS - 1]) {
+		struct sockaddr *sa;
+		int len;
+
 		if (!dev->set_mac_address) {
 			err = -EOPNOTSUPP;
 			goto out;
@@ -405,7 +408,17 @@ static int do_setlink(struct sk_buff *sk
 		if (ida[IFLA_ADDRESS - 1]->rta_len != RTA_LENGTH(dev->addr_len))
 			goto out;
 
-		err = dev->set_mac_address(dev, RTA_DATA(ida[IFLA_ADDRESS - 1]));
+		len = sizeof(sa_family_t) + dev->addr_len;
+		sa = kmalloc(len, GFP_KERNEL);
+		if (!sa) {
+			err = -ENOMEM;
+			goto out;
+		}
+		sa->sa_family = dev->type;
+		memcpy(sa->sa_data, RTA_DATA(ida[IFLA_ADDRESS - 1]),
+		       dev->addr_len);
+		err = dev->set_mac_address(dev, sa);
+		kfree(sa);
 		if (err)
 			goto out;
 		send_addr_notify = 1;

Re: Bug in the RTM_SETLINK kernel API for setting MAC address

[Pavlin Radoslavov]

David Miller <davem@davemloft.net> wrote:

> > The real mismatch is that ida[IFLA_ADDRESS - 1] is (as you say)
> > suppose to be a MAC address, but the set_mac_address() functions
> > for each device assume that the RTA_DATA(ida[IFLA_ADDRESS - 1])
> > payload is a sockaddr.
> 
> That's because ->set_mac_address() is usually invoked via
> dev_set_mac_address() which in turn is invoked from places
> SIOCSIFHWADDR ioctl() processing which does want the sockaddr
> wrapped around the MAC address.
> 
> So the netlink code is definitely doing the wrong thing if
> it wants merely the MAC address in the attribute.
> 
> Since changing all the drivers is a pain, what we probably
> should do is have the netlink code allocate a sockaddr,
> place the MAC address attribute in to that allocated sockaddr,
> and pass it into ->set_mac_address().
> 
> This patch should do the trick, can you test it out?

Yes, it works.

Can I presume that the fix will be in the next kernel release
(2.6.17.8 or 2.6.18), so we will know to reverse our userland
work-around changes when the kernel is out.

Thanks,
Pavlin

Re: Bug in the RTM_SETLINK kernel API for setting MAC address

[David Miller]

From: Pavlin Radoslavov <pavlin@icir.org>
Date: Tue, 08 Aug 2006 11:26:44 -0700

> Can I presume that the fix will be in the next kernel release
> (2.6.17.8 or 2.6.18), so we will know to reverse our userland
> work-around changes when the kernel is out.

Yes, I will submit it for both 2.6.18 and 2.6.17.x-stable