| Message ID | 20190426192954.146301-1-willemdebruijn.kernel@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] packet: in recvmsg msg_name return at least sockaddr_ll | expand |
On Fri, Apr 26, 2019 at 3:29 PM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > From: Willem de Bruijn <willemb@google.com> > > Packet send checks that msg_name is at least sizeof sockaddr_ll. > Packet recv must return at least this length, so that its output > can be passed unmodified to packet send. > > This ceased to be true since adding support for lladdr longer than > sll_addr. Since, the return value uses true address length. > > Always return at least sizeof sockaddr_ll, even if address length > is shorter. Zero the padding bytes. > > Fixes: 0fb375fb9b93 ("[AF_PACKET]: Allow for > 8 byte hardware addresses.") > Suggested-by: David Laight <David.Laight@aculab.com> > Signed-off-by: Willem de Bruijn <willemb@google.com> > --- > net/packet/af_packet.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c > index 13301e36b4a28..ca38e75c702e7 100644 > --- a/net/packet/af_packet.c > +++ b/net/packet/af_packet.c > @@ -3358,9 +3358,14 @@ static int packet_recvmsg(struct socket *sock, struct msghdr *msg, size_t len, > msg->msg_namelen = sizeof(struct sockaddr_pkt); > } else { > struct sockaddr_ll *sll = &PACKET_SKB_CB(skb)->sa.ll; > - Did not mean to remove this whitespace line. Will fix up, but will leave this out for review as is for now. > msg->msg_namelen = sll->sll_halen + > offsetof(struct sockaddr_ll, sll_addr); > + if (msg->msg_namelen < sizeof(struct sockaddr_ll)) { > + memset(msg->msg_name + > + offsetof(struct sockaddr_ll, sll_addr), > + 0, sizeof(sll->sll_addr)); > + msg->msg_namelen = sizeof(struct sockaddr_ll); > + } > } > memcpy(msg->msg_name, &PACKET_SKB_CB(skb)->sa, > msg->msg_namelen); > -- > 2.21.0.593.g511ec345e18-goog >
From: Willem de Bruijn [mailto:willemdebruijn.kernel@gmail.com] > Sent: 26 April 2019 20:30 > Packet send checks that msg_name is at least sizeof sockaddr_ll. > Packet recv must return at least this length, so that its output > can be passed unmodified to packet send. > > This ceased to be true since adding support for lladdr longer than > sll_addr. Since, the return value uses true address length. > > Always return at least sizeof sockaddr_ll, even if address length > is shorter. Zero the padding bytes. > > Fixes: 0fb375fb9b93 ("[AF_PACKET]: Allow for > 8 byte hardware addresses.") > Suggested-by: David Laight <David.Laight@aculab.com> > Signed-off-by: Willem de Bruijn <willemb@google.com> > --- > net/packet/af_packet.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c > index 13301e36b4a28..ca38e75c702e7 100644 > --- a/net/packet/af_packet.c > +++ b/net/packet/af_packet.c > @@ -3358,9 +3358,14 @@ static int packet_recvmsg(struct socket *sock, struct msghdr *msg, size_t len, > 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); > + if (msg->msg_namelen < sizeof(struct sockaddr_ll)) { > + memset(msg->msg_name + > + offsetof(struct sockaddr_ll, sll_addr), > + 0, sizeof(sll->sll_addr)); > + msg->msg_namelen = sizeof(struct sockaddr_ll); > + } > } > memcpy(msg->msg_name, &PACKET_SKB_CB(skb)->sa, > msg->msg_namelen); That memcpy() carefully overwrites the zeroed bytes. You need a separate 'copy_len' that isn't updated (from 18 to 20). David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Mon, Apr 29, 2019 at 5:03 AM David Laight <David.Laight@aculab.com> wrote: > > From: Willem de Bruijn [mailto:willemdebruijn.kernel@gmail.com] > > Sent: 26 April 2019 20:30 > > Packet send checks that msg_name is at least sizeof sockaddr_ll. > > Packet recv must return at least this length, so that its output > > can be passed unmodified to packet send. > > > > This ceased to be true since adding support for lladdr longer than > > sll_addr. Since, the return value uses true address length. > > > > Always return at least sizeof sockaddr_ll, even if address length > > is shorter. Zero the padding bytes. > > > > Fixes: 0fb375fb9b93 ("[AF_PACKET]: Allow for > 8 byte hardware addresses.") > > Suggested-by: David Laight <David.Laight@aculab.com> > > Signed-off-by: Willem de Bruijn <willemb@google.com> > > --- > > net/packet/af_packet.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c > > index 13301e36b4a28..ca38e75c702e7 100644 > > --- a/net/packet/af_packet.c > > +++ b/net/packet/af_packet.c > > @@ -3358,9 +3358,14 @@ static int packet_recvmsg(struct socket *sock, struct msghdr *msg, size_t len, > > 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); > > + if (msg->msg_namelen < sizeof(struct sockaddr_ll)) { > > + memset(msg->msg_name + > > + offsetof(struct sockaddr_ll, sll_addr), > > + 0, sizeof(sll->sll_addr)); > > + msg->msg_namelen = sizeof(struct sockaddr_ll); > > + } > > } > > memcpy(msg->msg_name, &PACKET_SKB_CB(skb)->sa, > > msg->msg_namelen); > > That memcpy() carefully overwrites the zeroed bytes. > You need a separate 'copy_len' that isn't updated (from 18 to 20). Argh. Thanks! if (msg->msg_name) { + int copy_len; + /* If the address length field is there to be filled * in, we fill it in now. */ if (sock->type == SOCK_PACKET) { __sockaddr_check_size(sizeof(struct sockaddr_pkt)); msg->msg_namelen = sizeof(struct sockaddr_pkt); + copy_len = msg->msg_namelen; } else { struct sockaddr_ll *sll = &PACKET_SKB_CB(skb)->sa.ll; msg->msg_namelen = sll->sll_halen + offsetof(struct sockaddr_ll, sll_addr); + copy_len = msg->msg_namelen; + if (msg->msg_namelen < sizeof(struct sockaddr_ll)) { + memset(msg->msg_name + + offsetof(struct sockaddr_ll, sll_addr), + 0, sizeof(sll->sll_addr)); + msg->msg_namelen = sizeof(struct sockaddr_ll); + } } - memcpy(msg->msg_name, &PACKET_SKB_CB(skb)->sa, - msg->msg_namelen); + memcpy(msg->msg_name, &PACKET_SKB_CB(skb)->sa, copy_len); } Can then also change memset to zero only two bytes in the Ethernet case. + if (msg->msg_namelen < sizeof(struct sockaddr_ll)) { + msg->msg_namelen = sizeof(struct sockaddr_ll); + memset(msg->msg_name + copy_len, 0, + msg->namelen - copy_len); + }
> Can then also change memset to zero only two bytes in the Ethernet case. > > + if (msg->msg_namelen < sizeof(struct sockaddr_ll)) { > + msg->msg_namelen = sizeof(struct sockaddr_ll); > + memset(msg->msg_name + copy_len, 0, > + msg->namelen - copy_len); copy_len not defined .... > + } Except that has to be a real memset() not an inlined direct write of an 8byte register (or 2 writes on a 32bit systems). David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Mon, Apr 29, 2019 at 9:19 AM David Laight <David.Laight@aculab.com> wrote: > > > Can then also change memset to zero only two bytes in the Ethernet case. > > > > + if (msg->msg_namelen < sizeof(struct sockaddr_ll)) { > > + msg->msg_namelen = sizeof(struct sockaddr_ll); > > + memset(msg->msg_name + copy_len, 0, > > + msg->namelen - copy_len); > > copy_len not defined .... Was a quick sketch of an iteration on the above, sorry if unclear. Intended to be defined before the branch. > > > + } > > Except that has to be a real memset() not an inlined direct > write of an 8byte register (or 2 writes on a 32bit systems). I wasn't sure whether a 2 byte store would be optimized in a similar manner. That might even be architecture dependent, I imagine? Will leave as is. Thanks for the quick response! > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales)
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index 13301e36b4a28..ca38e75c702e7 100644 --- a/net/packet/af_packet.c +++ b/net/packet/af_packet.c @@ -3358,9 +3358,14 @@ static int packet_recvmsg(struct socket *sock, struct msghdr *msg, size_t len, 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); + if (msg->msg_namelen < sizeof(struct sockaddr_ll)) { + memset(msg->msg_name + + offsetof(struct sockaddr_ll, sll_addr), + 0, sizeof(sll->sll_addr)); + msg->msg_namelen = sizeof(struct sockaddr_ll); + } } memcpy(msg->msg_name, &PACKET_SKB_CB(skb)->sa, msg->msg_namelen);