From: Willem de Bruijn
> Sent: 25 April 2019 14:57
...
> > I've just done a bit of software archaeology.
> >
> > Prior to 2.6.14-rc3 the send code ignored sll_halen, it was only set by the
> > receive code.
> > So it is not surprising that old application code leaves it as zero.
> >
> > The old receive code also always set msg_namelen = sizeof (struct
> > sockaddr_ll).
> > The receive code now sets:
> > msg_namelen = offsetof(struct sockaddr_ll, sll_addr) + saddr->sll_halen;
> > For ethernet this changes the msg_namelen from 20 to 18.
> > A side effect (no one has noticed for years) is that you can't send a reply
> > by passing back the received address buffer.
>
> Great find, thanks. I hadn't thought of going back that far, but
> clearly should in these legacy caller questions..
Fortunately I didn't have to find the pre-git sources :-)
> > Looking at it all again how about:
> > char *addr = NULL;
> > ...
> > err = -EINVAL;
> > if (msg->msg_namelen < offsetof(struct sockaddr_ll,
> > sll_addr))
> > goto out;
> > proto = saddr->sll_protocol;
> > dev = dev_get_by_index(sock_net(sk),
> > saddr->sll_ifindex);
> > if (dev && sock->type == SOCK_DGRAM) {
> > if (msg->msg_namelen < dev->addr_len +
> > offsetof(struct sockaddr_ll, sll_addr))
> > goto out_unlock;
> > addr = saddr->sll_addr;
> > }
>
> Yes, given the above, this looks great to me.
>
> In general I'm hesitant to loosen interface constraints. As you can
> never tighten them again.
Indeed.
> But given the change on recv, it seems
> correct here. That is technically a separate issue, so worth a
> separate patch, I think. If that's not too pedantic. Else at least an
> extra Fixes tag.
Or leave the above using 'sizeof' and change the receive code to pad the
address to sizeof struct sockaddr_ll.
Which is definitely a completely different fix.
The rx code seems to be:
if (sock->type == SOCK_PACKET) {
__sockaddr_check_size(sizeof(struct sockaddr_pkt));
msg->msg_namelen = sizeof(struct sockaddr_pkt);
} else {
struct sockaddr_ll *sll = &PACKET_SKB_CB(skb)->sa.ll;
msg->msg_namelen = sll->sll_halen +
offsetof(struct sockaddr_ll, sll_addr);
}
memcpy(msg->msg_name, &PACKET_SKB_CB(skb)->sa,
msg->msg_namelen);
Hopefully the buffer is always big enough!
Assuming that the code that sets up the address zaps the last two bytes
the SOCK_DGRAM side just needs a max(, sizeof (struct sockaddr_ll)).
Zeroing the 8 byte field before the mac address is put into it is cheap
(one 64bit write on 64bit systems).
I guess this would have 'Fixes' tag for the 2.6.14 git tag!
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT,
UK
Registration No: 1397386 (Wales)
