On Thu, Apr 25, 2019 at 10:35 AM David Laight <david.lai...@aculab.com> wrote:
>
> 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!
The kernel buffer is always sockaddr_storage with _K_SS_MAXSIZE or
128B. Before copying into skb->cb[] there is the
sock_skb_cb_check_size BUILD_BUG_ON based on the assumption that every
lladdr len <= MAX_ADDR_LEN (32B). As far as I can telll, that does
seem to hold (INFINIBAND_ALEN being the largest at 32). So while there
is no explicit check on the memcpy from skb->cb into msg_name, this is
safe.
> 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!
The uaddr_len change is commit 0fb375fb9b93 ("[AF_PACKET]: Allow for >
8 byte hardware addresses.")?
That both introduced the recv change to return a length based on
dev->addr_len even the total is if below sizeof(struct sockaddr_ll)
- else
- msg->msg_namelen = sizeof(struct sockaddr_ll);
[..]
+ else
+ msg->msg_namelen = sll->sll_halen + offsetof(struct
sockaddr_ll, sll_addr);
and the new sendmsg check
if (msg->msg_namelen < sizeof(struct sockaddr_ll))
goto out;
+ if (msg->msg_namelen < (saddr->sll_halen +
offsetof(struct sockaddr_ll, sll_addr)))
+ goto out;
but without removing the preexisting check above it, thus causing the
recvfrom () followed by sendto() to fail.
