[Acked,w/,cmt,Lucid,CVE-2013-7266] net: rework recvmsg handler msg_name and msg_namelen logic
diff mbox

Message ID 20140120153832.GA6786@dm
State New
Headers show

Commit Message

Andy Whitcroft Jan. 20, 2014, 3:38 p.m. UTC
On Mon, Jan 20, 2014 at 01:22:50PM +0000, Luis Henriques wrote:
> On Mon, Jan 20, 2014 at 12:31:14PM +0000, Andy Whitcroft wrote:
> > On Fri, Jan 17, 2014 at 02:31:09PM +0000, Luis Henriques wrote:
> > > From: Hannes Frederic Sowa <hannes@stressinduktion.org>
> > > 
> > > CVE-2013-7266
> > > 
> > > BugLink: http://bugs.launchpad.net/bugs/1267081
> > > 
> > > This patch now always passes msg->msg_namelen as 0. recvmsg handlers must
> > > set msg_namelen to the proper size <= sizeof(struct sockaddr_storage)
> > > to return msg_name to the user.
> > > 
> > > This prevents numerous uninitialized memory leaks we had in the
> > > recvmsg handlers and makes it harder for new code to accidentally leak
> > > uninitialized memory.
> > > 
> > > Optimize for the case recvfrom is called with NULL as address. We don't
> > > need to copy the address at all, so set it to NULL before invoking the
> > > recvmsg handler. We can do so, because all the recvmsg handlers must
> > > cope with the case a plain read() is called on them. read() also sets
> > > msg_name to NULL.
> > > 
> > > Also document these changes in include/linux/net.h as suggested by David
> > > Miller.
> > > 
> > > Changes since RFC:
> > > 
> > > Set msg->msg_name = NULL if user specified a NULL in msg_name but had a
> > > non-null msg_namelen in verify_iovec/verify_compat_iovec. This doesn't
> > > affect sendto as it would bail out earlier while trying to copy-in the
> > > address. It also more naturally reflects the logic by the callers of
> > > verify_iovec.
> > > 
> > > With this change in place I could remove "
> > > if (!uaddr || msg_sys->msg_namelen == 0)
> > > 	msg->msg_name = NULL
> > > ".
> > > 
> > > This change does not alter the user visible error logic as we ignore
> > > msg_namelen as long as msg_name is NULL.
> > > 
> > > Also remove two unnecessary curly brackets in ___sys_recvmsg and change
> > > comments to netdev style.
> > > 
> > > Cc: David Miller <davem@davemloft.net>
> > > Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
> > > Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> > > Signed-off-by: David S. Miller <davem@davemloft.net>
> > > (back ported from commit f3d3342602f8bcbf37d7c46641cb9bca7618eb1c)
> > > Signed-off-by: Luis Henriques <luis.henriques@canonical.com>
> > > ---
> > >  drivers/isdn/mISDN/socket.c  | 13 ++++---------
> > >  drivers/net/pppoe.c          |  2 --
> > >  drivers/net/pppol2tp.c       |  2 --
> > >  include/linux/net.h          |  8 ++++++++
> > >  net/appletalk/ddp.c          | 16 +++++++---------
> > >  net/atm/common.c             |  2 --
> > >  net/ax25/af_ax25.c           |  4 ++--
> > >  net/bluetooth/af_bluetooth.c |  2 --
> > >  net/bluetooth/hci_sock.c     |  2 --
> > >  net/bluetooth/rfcomm/sock.c  |  3 ---
> > >  net/compat.c                 |  3 ++-
> > >  net/core/iovec.c             |  3 ++-
> > >  net/ipx/af_ipx.c             |  3 +--
> > >  net/irda/af_irda.c           |  4 ----
> > >  net/iucv/af_iucv.c           |  2 --
> > >  net/key/af_key.c             |  1 -
> > >  net/llc/af_llc.c             |  2 --
> > >  net/netlink/af_netlink.c     |  2 --
> > >  net/netrom/af_netrom.c       |  3 +--
> > >  net/packet/af_packet.c       | 32 +++++++++++++++-----------------
> > >  net/rds/recv.c               |  2 --
> > >  net/rose/af_rose.c           |  8 +++++---
> > >  net/rxrpc/ar-recvmsg.c       |  9 ++++++---
> > >  net/socket.c                 | 19 +++++++++++--------
> > >  net/tipc/socket.c            |  6 ------
> > >  net/unix/af_unix.c           |  5 -----
> > >  net/x25/af_x25.c             |  3 +--
> > >  27 files changed, 65 insertions(+), 96 deletions(-)
> > > 
> > > diff --git a/drivers/isdn/mISDN/socket.c b/drivers/isdn/mISDN/socket.c
> > > index feb0fa4..db69cb4 100644
> > > --- a/drivers/isdn/mISDN/socket.c
> > > +++ b/drivers/isdn/mISDN/socket.c
> > > @@ -115,7 +115,6 @@ mISDN_sock_recvmsg(struct kiocb *iocb, struct socket *sock,
> > >  {
> > >  	struct sk_buff		*skb;
> > >  	struct sock		*sk = sock->sk;
> > > -	struct sockaddr_mISDN	*maddr;
> > >  
> > >  	int		copied, err;
> > >  
> > > @@ -133,9 +132,9 @@ mISDN_sock_recvmsg(struct kiocb *iocb, struct socket *sock,
> > >  	if (!skb)
> > >  		return err;
> > >  
> > > -	if (msg->msg_namelen >= sizeof(struct sockaddr_mISDN)) {
> > > -		msg->msg_namelen = sizeof(struct sockaddr_mISDN);
> > > -		maddr = (struct sockaddr_mISDN *)msg->msg_name;
> > > +	if (msg->msg_name) {
> > > +		struct sockaddr_mISDN *maddr = msg->msg_name;
> > > +
> > >  		maddr->family = AF_ISDN;
> > >  		maddr->dev = _pms(sk)->dev->id;
> > >  		if ((sk->sk_protocol == ISDN_P_LAPD_TE) ||
> > > @@ -148,11 +147,7 @@ mISDN_sock_recvmsg(struct kiocb *iocb, struct socket *sock,
> > >  			maddr->sapi = _pms(sk)->ch.addr & 0xFF;
> > >  			maddr->tei =  (_pms(sk)->ch.addr >> 8) & 0xFF;
> > >  		}
> > > -	} else {
> > > -		if (msg->msg_namelen)
> > > -			printk(KERN_WARNING "%s: too small namelen %d\n",
> > > -			    __func__, msg->msg_namelen);
> > > -		msg->msg_namelen = 0;
> > > +		msg->msg_namelen = sizeof(*maddr);
> > >  	}
> > >  
> > >  	copied = skb->len + MISDN_HEADER_LEN;
> > > diff --git a/drivers/net/pppoe.c b/drivers/net/pppoe.c
> > > index 2559991..343fd1e 100644
> > > --- a/drivers/net/pppoe.c
> > > +++ b/drivers/net/pppoe.c
> > > @@ -992,8 +992,6 @@ static int pppoe_recvmsg(struct kiocb *iocb, struct socket *sock,
> > >  	if (error < 0)
> > >  		goto end;
> > >  
> > > -	m->msg_namelen = 0;
> > > -
> > >  	if (skb) {
> > >  		total_len = min_t(size_t, total_len, skb->len);
> > >  		error = skb_copy_datagram_iovec(skb, 0, m->msg_iov, total_len);
> > > diff --git a/drivers/net/pppol2tp.c b/drivers/net/pppol2tp.c
> > > index 9235901..4cdc1cf 100644
> > > --- a/drivers/net/pppol2tp.c
> > > +++ b/drivers/net/pppol2tp.c
> > > @@ -829,8 +829,6 @@ static int pppol2tp_recvmsg(struct kiocb *iocb, struct socket *sock,
> > >  	if (sk->sk_state & PPPOX_BOUND)
> > >  		goto end;
> > >  
> > > -	msg->msg_namelen = 0;
> > > -
> > >  	err = 0;
> > >  	skb = skb_recv_datagram(sk, flags & ~MSG_DONTWAIT,
> > >  				flags & MSG_DONTWAIT, &err);
> > > diff --git a/include/linux/net.h b/include/linux/net.h
> > > index 529a093..e40cbcc 100644
> > > --- a/include/linux/net.h
> > > +++ b/include/linux/net.h
> > > @@ -187,6 +187,14 @@ struct proto_ops {
> > >  				      int optname, char __user *optval, int __user *optlen);
> > >  	int		(*sendmsg)   (struct kiocb *iocb, struct socket *sock,
> > >  				      struct msghdr *m, size_t total_len);
> > > +	/* Notes for implementing recvmsg:
> > > +	 * ===============================
> > > +	 * msg->msg_namelen should get updated by the recvmsg handlers
> > > +	 * iff msg_name != NULL. It is by default 0 to prevent
> > > +	 * returning uninitialized memory to user space.  The recvfrom
> > > +	 * handlers can assume that msg.msg_name is either NULL or has
> > > +	 * a minimum size of sizeof(struct sockaddr_storage).
> > > +	 */
> > >  	int		(*recvmsg)   (struct kiocb *iocb, struct socket *sock,
> > >  				      struct msghdr *m, size_t total_len,
> > >  				      int flags);
> > > diff --git a/net/appletalk/ddp.c b/net/appletalk/ddp.c
> > > index b1a4290..5eae360 100644
> > > --- a/net/appletalk/ddp.c
> > > +++ b/net/appletalk/ddp.c
> > > @@ -1703,7 +1703,6 @@ static int atalk_recvmsg(struct kiocb *iocb, struct socket *sock, struct msghdr
> > >  			 size_t size, int flags)
> > >  {
> > >  	struct sock *sk = sock->sk;
> > > -	struct sockaddr_at *sat = (struct sockaddr_at *)msg->msg_name;
> > >  	struct ddpehdr *ddp;
> > >  	int copied = 0;
> > >  	int offset = 0;
> > > @@ -1728,14 +1727,13 @@ static int atalk_recvmsg(struct kiocb *iocb, struct socket *sock, struct msghdr
> > >  	}
> > >  	err = skb_copy_datagram_iovec(skb, offset, msg->msg_iov, copied);
> > >  
> > > -	if (!err) {
> > > -		if (sat) {
> > > -			sat->sat_family      = AF_APPLETALK;
> > > -			sat->sat_port        = ddp->deh_sport;
> > > -			sat->sat_addr.s_node = ddp->deh_snode;
> > > -			sat->sat_addr.s_net  = ddp->deh_snet;
> > > -		}
> > > -		msg->msg_namelen = sizeof(*sat);
> > > +	if (!err && msg->msg_name) {
> > > +		struct sockaddr_at *sat = msg->msg_name;
> > > +		sat->sat_family      = AF_APPLETALK;
> > > +		sat->sat_port        = ddp->deh_sport;
> > > +		sat->sat_addr.s_node = ddp->deh_snode;
> > > +		sat->sat_addr.s_net  = ddp->deh_snet;
> > > +		msg->msg_namelen     = sizeof(*sat);
> > >  	}
> > >  
> > >  	skb_free_datagram(sk, skb);	/* Free the datagram. */
> > > diff --git a/net/atm/common.c b/net/atm/common.c
> > > index 65737b8..0baf05e 100644
> > > --- a/net/atm/common.c
> > > +++ b/net/atm/common.c
> > > @@ -473,8 +473,6 @@ int vcc_recvmsg(struct kiocb *iocb, struct socket *sock, struct msghdr *msg,
> > >  	struct sk_buff *skb;
> > >  	int copied, error = -EINVAL;
> > >  
> > > -	msg->msg_namelen = 0;
> > > -
> > >  	if (sock->state != SS_CONNECTED)
> > >  		return -ENOTCONN;
> > >  	if (flags & ~MSG_DONTWAIT)		/* only handle MSG_DONTWAIT */
> > > diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
> > > index 8613bd1..6b9d62b 100644
> > > --- a/net/ax25/af_ax25.c
> > > +++ b/net/ax25/af_ax25.c
> > > @@ -1648,11 +1648,11 @@ static int ax25_recvmsg(struct kiocb *iocb, struct socket *sock,
> > >  
> > >  	skb_copy_datagram_iovec(skb, 0, msg->msg_iov, copied);
> > >  
> > > -	if (msg->msg_namelen != 0) {
> > > -		struct sockaddr_ax25 *sax = (struct sockaddr_ax25 *)msg->msg_name;
> > > +	if (msg->msg_name) {
> > >  		ax25_digi digi;
> > >  		ax25_address src;
> > >  		const unsigned char *mac = skb_mac_header(skb);
> > > +		struct sockaddr_ax25 *sax = msg->msg_name;
> > >  
> > >  		memset(sax, 0, sizeof(struct full_sockaddr_ax25));
> > >  		ax25_addr_parse(mac + 1, skb->data - mac - 1, &src, NULL,
> > > diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
> > > index d7239dd..143b8a7 100644
> > > --- a/net/bluetooth/af_bluetooth.c
> > > +++ b/net/bluetooth/af_bluetooth.c
> > > @@ -240,8 +240,6 @@ int bt_sock_recvmsg(struct kiocb *iocb, struct socket *sock,
> > >  	if (flags & (MSG_OOB))
> > >  		return -EOPNOTSUPP;
> > >  
> > > -	msg->msg_namelen = 0;
> > > -
> > >  	if (!(skb = skb_recv_datagram(sk, flags, noblock, &err))) {
> > >  		if (sk->sk_shutdown & RCV_SHUTDOWN)
> > >  			return 0;
> > > diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
> > > index 6c00bf7..bb2548b 100644
> > > --- a/net/bluetooth/hci_sock.c
> > > +++ b/net/bluetooth/hci_sock.c
> > > @@ -370,8 +370,6 @@ static int hci_sock_recvmsg(struct kiocb *iocb, struct socket *sock,
> > >  	if (!(skb = skb_recv_datagram(sk, flags, noblock, &err)))
> > >  		return err;
> > >  
> > > -	msg->msg_namelen = 0;
> > > -
> > >  	copied = skb->len;
> > >  	if (len < copied) {
> > >  		msg->msg_flags |= MSG_TRUNC;
> > > diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
> > > index 1db0132..3fabaad 100644
> > > --- a/net/bluetooth/rfcomm/sock.c
> > > +++ b/net/bluetooth/rfcomm/sock.c
> > > @@ -652,15 +652,12 @@ static int rfcomm_sock_recvmsg(struct kiocb *iocb, struct socket *sock,
> > >  
> > >  	if (test_and_clear_bit(RFCOMM_DEFER_SETUP, &d->flags)) {
> > >  		rfcomm_dlc_accept(d);
> > > -		msg->msg_namelen = 0;
> > >  		return 0;
> > >  	}
> > >  
> > >  	if (flags & MSG_OOB)
> > >  		return -EOPNOTSUPP;
> > >  
> > > -	msg->msg_namelen = 0;
> > > -
> > >  	BT_DBG("sk %p size %zu", sk, size);
> > >  
> > >  	lock_sock(sk);
> > > diff --git a/net/compat.c b/net/compat.c
> > > index 9559afc..305bca6 100644
> > > --- a/net/compat.c
> > > +++ b/net/compat.c
> > > @@ -89,7 +89,8 @@ int verify_compat_iovec(struct msghdr *kern_msg, struct iovec *kern_iov,
> > >  			if (err < 0)
> > >  				return err;
> > >  		}
> > > -		kern_msg->msg_name = kern_address;
> > > +		if (kern_msg->msg_name)
> > > +			kern_msg->msg_name = kern_address;
> > >  	} else
> > >  		kern_msg->msg_name = NULL;
> > >  
> > > diff --git a/net/core/iovec.c b/net/core/iovec.c
> > > index f911e66..39369e9 100644
> > > --- a/net/core/iovec.c
> > > +++ b/net/core/iovec.c
> > > @@ -47,7 +47,8 @@ int verify_iovec(struct msghdr *m, struct iovec *iov, struct sockaddr *address,
> > >  			if (err < 0)
> > >  				return err;
> > >  		}
> > > -		m->msg_name = address;
> > > +		if (m->msg_name)
> > > +			m->msg_name = address;
> > >  	} else {
> > >  		m->msg_name = NULL;
> > >  	}
> > > diff --git a/net/ipx/af_ipx.c b/net/ipx/af_ipx.c
> > > index 66c7a20..25931b3 100644
> > > --- a/net/ipx/af_ipx.c
> > > +++ b/net/ipx/af_ipx.c
> > > @@ -1808,8 +1808,6 @@ static int ipx_recvmsg(struct kiocb *iocb, struct socket *sock,
> > >  	if (skb->tstamp.tv64)
> > >  		sk->sk_stamp = skb->tstamp;
> > >  
> > > -	msg->msg_namelen = sizeof(*sipx);
> > > -
> > >  	if (sipx) {
> > >  		sipx->sipx_family	= AF_IPX;
> > >  		sipx->sipx_port		= ipx->ipx_source.sock;
> > > @@ -1817,6 +1815,7 @@ static int ipx_recvmsg(struct kiocb *iocb, struct socket *sock,
> > >  		sipx->sipx_network	= IPX_SKB_CB(skb)->ipx_source_net;
> > >  		sipx->sipx_type 	= ipx->ipx_type;
> > >  		sipx->sipx_zero		= 0;
> > > +		msg->msg_namelen	= sizeof(*sipx);
> > >  	}
> > >  	rc = copied;
> > >  
> > > diff --git a/net/irda/af_irda.c b/net/irda/af_irda.c
> > > index bfb325d..7cb7613 100644
> > > --- a/net/irda/af_irda.c
> > > +++ b/net/irda/af_irda.c
> > > @@ -1338,8 +1338,6 @@ static int irda_recvmsg_dgram(struct kiocb *iocb, struct socket *sock,
> > >  	if ((err = sock_error(sk)) < 0)
> > >  		return err;
> > >  
> > > -	msg->msg_namelen = 0;
> > > -
> > >  	skb = skb_recv_datagram(sk, flags & ~MSG_DONTWAIT,
> > >  				flags & MSG_DONTWAIT, &err);
> > >  	if (!skb)
> > > @@ -1402,8 +1400,6 @@ static int irda_recvmsg_stream(struct kiocb *iocb, struct socket *sock,
> > >  	target = sock_rcvlowat(sk, flags & MSG_WAITALL, size);
> > >  	timeo = sock_rcvtimeo(sk, noblock);
> > >  
> > > -	msg->msg_namelen = 0;
> > > -
> > >  	do {
> > >  		int chunk;
> > >  		struct sk_buff *skb = skb_dequeue(&sk->sk_receive_queue);
> > > diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> > > index f605b23..bada1b9 100644
> > > --- a/net/iucv/af_iucv.c
> > > +++ b/net/iucv/af_iucv.c
> > > @@ -1160,8 +1160,6 @@ static int iucv_sock_recvmsg(struct kiocb *iocb, struct socket *sock,
> > >  	struct sk_buff *skb, *rskb, *cskb;
> > >  	int err = 0;
> > >  
> > > -	msg->msg_namelen = 0;
> > > -
> > >  	if ((sk->sk_state == IUCV_DISCONN || sk->sk_state == IUCV_SEVERED) &&
> > >  	    skb_queue_empty(&iucv->backlog_skb_q) &&
> > >  	    skb_queue_empty(&sk->sk_receive_queue) &&
> > > diff --git a/net/key/af_key.c b/net/key/af_key.c
> > > index 9d22e46..b6a6b85 100644
> > > --- a/net/key/af_key.c
> > > +++ b/net/key/af_key.c
> > > @@ -3593,7 +3593,6 @@ static int pfkey_recvmsg(struct kiocb *kiocb,
> > >  	if (flags & ~(MSG_PEEK|MSG_DONTWAIT|MSG_TRUNC|MSG_CMSG_COMPAT))
> > >  		goto out;
> > >  
> > > -	msg->msg_namelen = 0;
> > >  	skb = skb_recv_datagram(sk, flags, flags & MSG_DONTWAIT, &err);
> > >  	if (skb == NULL)
> > >  		goto out;
> > > diff --git a/net/llc/af_llc.c b/net/llc/af_llc.c
> > > index 8a814a5..606b6ad 100644
> > > --- a/net/llc/af_llc.c
> > > +++ b/net/llc/af_llc.c
> > > @@ -674,8 +674,6 @@ static int llc_ui_recvmsg(struct kiocb *iocb, struct socket *sock,
> > >  	int target;	/* Read at least this many bytes */
> > >  	long timeo;
> > >  
> > > -	msg->msg_namelen = 0;
> > > -
> > >  	lock_sock(sk);
> > >  	copied = -ENOTCONN;
> > >  	if (unlikely(sk->sk_type == SOCK_STREAM && sk->sk_state == TCP_LISTEN))
> > > diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> > > index fc91ff6..39a6d5d 100644
> > > --- a/net/netlink/af_netlink.c
> > > +++ b/net/netlink/af_netlink.c
> > > @@ -1400,8 +1400,6 @@ static int netlink_recvmsg(struct kiocb *kiocb, struct socket *sock,
> > >  	}
> > >  #endif
> > >  
> > > -	msg->msg_namelen = 0;
> > > -
> > >  	copied = data_skb->len;
> > >  	if (len < copied) {
> > >  		msg->msg_flags |= MSG_TRUNC;
> > > diff --git a/net/netrom/af_netrom.c b/net/netrom/af_netrom.c
> > > index d240523..b3c9b48 100644
> > > --- a/net/netrom/af_netrom.c
> > > +++ b/net/netrom/af_netrom.c
> > > @@ -1185,10 +1185,9 @@ static int nr_recvmsg(struct kiocb *iocb, struct socket *sock,
> > >  		sax->sax25_family = AF_NETROM;
> > >  		skb_copy_from_linear_data_offset(skb, 7, sax->sax25_call.ax25_call,
> > >  			      AX25_ADDR_LEN);
> > > +		msg->msg_namelen = sizeof(*sax);
> > >  	}
> > >  
> > > -	msg->msg_namelen = sizeof(*sax);
> > > -
> > >  	skb_free_datagram(sk, skb);
> > >  
> > >  	release_sock(sk);
> > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> > > index 728c080..1de1992 100644
> > > --- a/net/packet/af_packet.c
> > > +++ b/net/packet/af_packet.c
> > > @@ -1423,7 +1423,6 @@ static int packet_recvmsg(struct kiocb *iocb, struct socket *sock,
> > >  	struct sock *sk = sock->sk;
> > >  	struct sk_buff *skb;
> > >  	int copied, err;
> > > -	struct sockaddr_ll *sll;
> > >  
> > >  	err = -EINVAL;
> > >  	if (flags & ~(MSG_PEEK|MSG_DONTWAIT|MSG_TRUNC|MSG_CMSG_COMPAT))
> > > @@ -1455,22 +1454,10 @@ static int packet_recvmsg(struct kiocb *iocb, struct socket *sock,
> > >  	if (skb == NULL)
> > >  		goto out;
> > >  
> > > -	/*
> > > -	 *	If the address length field is there to be filled in, we fill
> > > -	 *	it in now.
> > > +	/* You lose any data beyond the buffer you gave. If it worries
> > > +	 * a user program they can ask the device for its MTU
> > > +	 * anyway.
> > >  	 */
> > > -
> > > -	sll = &PACKET_SKB_CB(skb)->sa.ll;
> > > -	if (sock->type == SOCK_PACKET)
> > > -		msg->msg_namelen = sizeof(struct sockaddr_pkt);
> > > -	else
> > > -		msg->msg_namelen = sll->sll_halen + offsetof(struct sockaddr_ll, sll_addr);
> > > -
> > > -	/*
> > > -	 *	You lose any data beyond the buffer you gave. If it worries a
> > > -	 *	user program they can ask the device for its MTU anyway.
> > > -	 */
> > > -
> > >  	copied = skb->len;
> > >  	if (copied > len) {
> > >  		copied = len;
> > > @@ -1483,9 +1470,20 @@ static int packet_recvmsg(struct kiocb *iocb, struct socket *sock,
> > >  
> > >  	sock_recv_timestamp(msg, sk, skb);
> > >  
> > > -	if (msg->msg_name)
> > > +	if (msg->msg_name) {
> > > +		/* If the address length field is there to be filled
> > > +		 * in, we fill it in now.
> > > +		 */
> > > +		if (sock->type == SOCK_PACKET) {
> > > +			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);
> > > +	}
> > >  
> > >  	if (pkt_sk(sk)->auxdata) {
> > >  		struct tpacket_auxdata aux;
> > > diff --git a/net/rds/recv.c b/net/rds/recv.c
> > > index c45a881c..a11cab9 100644
> > > --- a/net/rds/recv.c
> > > +++ b/net/rds/recv.c
> > > @@ -410,8 +410,6 @@ int rds_recvmsg(struct kiocb *iocb, struct socket *sock, struct msghdr *msg,
> > >  
> > >  	rdsdebug("size %zu flags 0x%x timeo %ld\n", size, msg_flags, timeo);
> > >  
> > > -	msg->msg_namelen = 0;
> > > -
> > >  	if (msg_flags & MSG_OOB)
> > >  		goto out;
> > >  
> > > diff --git a/net/rose/af_rose.c b/net/rose/af_rose.c
> > > index 2984999..08a86f6 100644
> > > --- a/net/rose/af_rose.c
> > > +++ b/net/rose/af_rose.c
> > > @@ -1238,7 +1238,6 @@ static int rose_recvmsg(struct kiocb *iocb, struct socket *sock,
> > >  {
> > >  	struct sock *sk = sock->sk;
> > >  	struct rose_sock *rose = rose_sk(sk);
> > > -	struct sockaddr_rose *srose = (struct sockaddr_rose *)msg->msg_name;
> > >  	size_t copied;
> > >  	unsigned char *asmptr;
> > >  	struct sk_buff *skb;
> > > @@ -1274,8 +1273,11 @@ static int rose_recvmsg(struct kiocb *iocb, struct socket *sock,
> > >  
> > >  	skb_copy_datagram_iovec(skb, 0, msg->msg_iov, copied);
> > >  
> > > -	if (srose != NULL) {
> > > -		memset(srose, 0, msg->msg_namelen);
> > > +	if (msg->msg_name) {
> > > +		struct sockaddr_rose *srose;
> > > +
> > > +		memset(msg->msg_name, 0, sizeof(struct full_sockaddr_rose));
> > > +		srose = msg->msg_name;
> > >  		srose->srose_family = AF_ROSE;
> > >  		srose->srose_addr   = rose->dest_addr;
> > >  		srose->srose_call   = rose->dest_call;
> > > diff --git a/net/rxrpc/ar-recvmsg.c b/net/rxrpc/ar-recvmsg.c
> > > index a39bf97..d5630d9 100644
> > > --- a/net/rxrpc/ar-recvmsg.c
> > > +++ b/net/rxrpc/ar-recvmsg.c
> > > @@ -142,10 +142,13 @@ int rxrpc_recvmsg(struct kiocb *iocb, struct socket *sock,
> > >  
> > >  		/* copy the peer address and timestamp */
> > >  		if (!continue_call) {
> > > -			if (msg->msg_name && msg->msg_namelen > 0)
> > > +			if (msg->msg_name) {
> > > +				size_t len =
> > > +					sizeof(call->conn->trans->peer->srx);
> > >  				memcpy(msg->msg_name,
> > > -				       &call->conn->trans->peer->srx,
> > > -				       sizeof(call->conn->trans->peer->srx));
> > > +				       &call->conn->trans->peer->srx, len);
> > > +				msg->msg_namelen = len;
> > > +			}
> > >  			sock_recv_timestamp(msg, &rx->sk, skb);
> > >  		}
> > >  
> > > diff --git a/net/socket.c b/net/socket.c
> > > index bf9fc68..e6c3396 100644
> > > --- a/net/socket.c
> > > +++ b/net/socket.c
> > > @@ -1744,8 +1744,10 @@ SYSCALL_DEFINE6(recvfrom, int, fd, void __user *, ubuf, size_t, size,
> > >  	msg.msg_iov = &iov;
> > >  	iov.iov_len = size;
> > >  	iov.iov_base = ubuf;
> > > -	msg.msg_name = (struct sockaddr *)&address;
> > > -	msg.msg_namelen = sizeof(address);
> > > +	/* Save some cycles and don't copy the address if not needed */
> > > +	msg.msg_name = addr ? (struct sockaddr *)&address : NULL;
> > > +	/* We assume all kernel code knows the size of sockaddr_storage */
> > > +	msg.msg_namelen = 0;
> > >  	if (sock->file->f_flags & O_NONBLOCK)
> > >  		flags |= MSG_DONTWAIT;
> > >  	err = sock_recvmsg(sock, &msg, size, flags);
> > > @@ -2017,18 +2019,16 @@ SYSCALL_DEFINE3(recvmsg, int, fd, struct msghdr __user *, msg,
> > >  			goto out_put;
> > >  	}
> > >  
> > > -	/*
> > > -	 *      Save the user-mode address (verify_iovec will change the
> > > -	 *      kernel msghdr to use the kernel address space)
> > > +	/* Save the user-mode address (verify_iovec will change the
> > > +	 * kernel msghdr to use the kernel address space)
> > >  	 */
> > > -
> > >  	uaddr = (__force void __user *)msg_sys.msg_name;
> > >  	uaddr_len = COMPAT_NAMELEN(msg);
> > > -	if (MSG_CMSG_COMPAT & flags) {
> > > +	if (MSG_CMSG_COMPAT & flags)
> > >  		err = verify_compat_iovec(&msg_sys, iov,
> > >  					  (struct sockaddr *)&addr,
> > >  					  VERIFY_WRITE);
> > > -	} else
> > > +	else
> > >  		err = verify_iovec(&msg_sys, iov,
> > >  				   (struct sockaddr *)&addr,
> > >  				   VERIFY_WRITE);
> > > @@ -2039,6 +2039,9 @@ SYSCALL_DEFINE3(recvmsg, int, fd, struct msghdr __user *, msg,
> > >  	cmsg_ptr = (unsigned long)msg_sys.msg_control;
> > >  	msg_sys.msg_flags = flags & (MSG_CMSG_CLOEXEC|MSG_CMSG_COMPAT);
> > >  
> > > +	/* We assume all kernel code knows the size of sockaddr_storage */
> > > +	msg_sys.msg_namelen = 0;
> > > +
> > >  	if (sock->file->f_flags & O_NONBLOCK)
> > >  		flags |= MSG_DONTWAIT;
> > >  	err = sock_recvmsg(sock, &msg_sys, total_len, flags);
> > > diff --git a/net/tipc/socket.c b/net/tipc/socket.c
> > > index b453345..024f490 100644
> > > --- a/net/tipc/socket.c
> > > +++ b/net/tipc/socket.c
> > > @@ -917,9 +917,6 @@ static int recv_msg(struct kiocb *iocb, struct socket *sock,
> > >  		goto exit;
> > >  	}
> > >  
> > > -	/* will be updated in set_orig_addr() if needed */
> > > -	m->msg_namelen = 0;
> > > -
> > >  restart:
> > >  
> > >  	/* Look for a message in receive queue; wait if necessary */
> > > @@ -1053,9 +1050,6 @@ static int recv_stream(struct kiocb *iocb, struct socket *sock,
> > >  		goto exit;
> > >  	}
> > >  
> > > -	/* will be updated in set_orig_addr() if needed */
> > > -	m->msg_namelen = 0;
> > > -
> > >  restart:
> > >  
> > >  	/* Look for a message in receive queue; wait if necessary */
> > > diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> > > index d65e7f0..fc57017 100644
> > > --- a/net/unix/af_unix.c
> > > +++ b/net/unix/af_unix.c
> > > @@ -1668,7 +1668,6 @@ static void unix_copy_addr(struct msghdr *msg, struct sock *sk)
> > >  {
> > >  	struct unix_sock *u = unix_sk(sk);
> > >  
> > > -	msg->msg_namelen = 0;
> > >  	if (u->addr) {
> > >  		msg->msg_namelen = u->addr->len;
> > >  		memcpy(msg->msg_name, u->addr->name, u->addr->len);
> > > @@ -1691,8 +1690,6 @@ static int unix_dgram_recvmsg(struct kiocb *iocb, struct socket *sock,
> > >  	if (flags&MSG_OOB)
> > >  		goto out;
> > >  
> > > -	msg->msg_namelen = 0;
> > > -
> > >  	mutex_lock(&u->readlock);
> > >  
> > >  	skb = skb_recv_datagram(sk, flags, noblock, &err);
> > > @@ -1818,8 +1815,6 @@ static int unix_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
> > >  	target = sock_rcvlowat(sk, flags&MSG_WAITALL, size);
> > >  	timeo = sock_rcvtimeo(sk, flags&MSG_DONTWAIT);
> > >  
> > > -	msg->msg_namelen = 0;
> > > -
> > >  	/* Lock the socket to prevent queue disordering
> > >  	 * while sleeps in memcpy_tomsg
> > >  	 */
> > > diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c
> > > index 2e9e300..40c447f 100644
> > > --- a/net/x25/af_x25.c
> > > +++ b/net/x25/af_x25.c
> > > @@ -1294,10 +1294,9 @@ static int x25_recvmsg(struct kiocb *iocb, struct socket *sock,
> > >  	if (sx25) {
> > >  		sx25->sx25_family = AF_X25;
> > >  		sx25->sx25_addr   = x25->dest_addr;
> > > +		msg->msg_namelen = sizeof(*sx25);
> > >  	}
> > >  
> > > -	msg->msg_namelen = sizeof(struct sockaddr_x25);
> > > -
> > >  	lock_sock(sk);
> > >  	x25_check_rbuf(sk);
> > >  	release_sock(sk);
> > 
> > Looks to apply all the bits we have.  The other parts are indeed MIA
> > (new code in later releases).  This patch therefore looks ok to me:
> > 
> > Acked-by: Andy Whitcroft <apw@canonical.com>
> > 
> > However, I note in mainline we have the commit below on top for rose
> > (which Lucid does have).  Do we need it too?
> > 
> >   commit f81152e35001e91997ec74a7b4e040e6ab0acccf
> >   Author: Florian Westphal <fw@strlen.de>
> >   Date:   Mon Dec 23 00:32:31 2013 +0100
> > 
> >     net: rose: restore old recvmsg behavior
> >     
> >     recvmsg handler in net/rose/af_rose.c performs size-check ->msg_namelen.
> 
> Thanks Andy, it seems to make sense for this commit to be applied as well.
> I missed this fix, although I've applied it to the stable kernels I'm
> maintaining...
> 
> So, once we have the required review ACKs, please apply it on top of the
> CVE fix backport (its a clean cherry-pick for Lucid).  Alternatively, I
> can resubmit the CVE fix to include the 2 patches.

Sounds fine to apply once this is acked, it is minute compared.  Full
patch is below.

-apw

commit f81152e35001e91997ec74a7b4e040e6ab0acccf
Author: Florian Westphal <fw@strlen.de>
Date:   Mon Dec 23 00:32:31 2013 +0100

    net: rose: restore old recvmsg behavior
    
    recvmsg handler in net/rose/af_rose.c performs size-check ->msg_namelen.
    
    After commit f3d3342602f8bcbf37d7c46641cb9bca7618eb1c
    (net: rework recvmsg handler msg_name and msg_namelen logic), we now
    always take the else branch due to namelen being initialized to 0.
    
    Digging in netdev-vger-cvs git repo shows that msg_namelen was
    initialized with a fixed-size since at least 1995, so the else branch
    was never taken.
    
    Compile tested only.
    
    Signed-off-by: Florian Westphal <fw@strlen.de>
    Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
    Signed-off-by: David S. Miller <davem@davemloft.net>

Patch
diff mbox

diff --git a/net/rose/af_rose.c b/net/rose/af_rose.c
index 33af772..62ced65 100644
--- a/net/rose/af_rose.c
+++ b/net/rose/af_rose.c
@@ -1253,6 +1253,7 @@  static int rose_recvmsg(struct kiocb *iocb, struct socket *sock,
 
        if (msg->msg_name) {
                struct sockaddr_rose *srose;
+               struct full_sockaddr_rose *full_srose = msg->msg_name;
 
                memset(msg->msg_name, 0, sizeof(struct full_sockaddr_rose));
                srose = msg->msg_name;
@@ -1260,18 +1261,9 @@  static int rose_recvmsg(struct kiocb *iocb, struct socket *sock,
                srose->srose_addr   = rose->dest_addr;
                srose->srose_call   = rose->dest_call;
                srose->srose_ndigis = rose->dest_ndigis;
-               if (msg->msg_namelen >= sizeof(struct full_sockaddr_rose)) {
-                       struct full_sockaddr_rose *full_srose = (struct full_sockaddr_rose *)msg->msg_name;
-                       for (n = 0 ; n < rose->dest_ndigis ; n++)
-                               full_srose->srose_digis[n] = rose->dest_digis[n];
-                       msg->msg_namelen = sizeof(struct full_sockaddr_rose);
-               } else {
-                       if (rose->dest_ndigis >= 1) {
-                               srose->srose_ndigis = 1;
-                               srose->srose_digi = rose->dest_digis[0];
-                       }
-                       msg->msg_namelen = sizeof(struct sockaddr_rose);
-               }
+               for (n = 0 ; n < rose->dest_ndigis ; n++)
+                       full_srose->srose_digis[n] = rose->dest_digis[n];
+               msg->msg_namelen = sizeof(struct full_sockaddr_rose);
        }
 
        skb_free_datagram(sk, skb);