netdev - RE: [PATCH 15/17] switch kernel_sendmsg() and kernel_recvmsg() to iov_iter

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 15 Apr 2015 09:08:27 +0000
From:	David Laight <David.Laight@...LAB.COM>
To:	'Al Viro' <viro@...IV.linux.org.uk>
CC:	"davem@...emloft.net" <davem@...emloft.net>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH 15/17] switch kernel_sendmsg() and kernel_recvmsg() to
 iov_iter_kvec()

From: Al Viro
> Sent: 14 April 2015 17:59
> On Tue, Apr 14, 2015 at 04:36:36PM +0000, David Laight wrote:
> > From: Al Viro
> > > Sent: 14 April 2015 17:34
> > > On Tue, Apr 14, 2015 at 04:21:02PM +0000, David Laight wrote:
> > >
> > > > Massive NAK.
> > > > This breaks any code that is using msg_control to set SCTP parameters
> > > > when sending data.
> > >
> > > 	Huh?  ->sendmsg() expects ->msg_control already in kernel space;
> > > it's ->recvmsg() that plays silly buggers with userland pointers there.
> >
> > I read your commit message as implying that you hadn't found any
> > users of kernel_sendmsg() that used msg_control.
> > Not that the data was always read from kernel space.
> 
> Sigh...  The situation is:
> 	* ->sendmsg() expects ->msg_control copied to userland.  sendmsg(2),
> sendto(2), etc. do that copying.  See ___sys_sendmsg() - there we have
>                 /*
>                  * Careful! Before this, msg_sys->msg_control contains a user pointer.
>                  * Afterwards, it will be a kernel pointer. Thus the compiler-assisted
>                  * checking falls down on this.
>                  */
>                 if (copy_from_user(ctl_buf,
>                                    (void __user __force *)msg_sys->msg_control,
>                                    ctl_len))
>                         goto out_freectl;
>                 msg_sys->msg_control = ctl_buf;
> As the result, ->sendmsg() instances access ->msg_control contents as normal
> kernel data.
> 	* ->recvmsg() expects ->msg_control to point to userland.  See
> net/core/scm.c for the helpers used to store into it.  recvmsg(2) et.al.
> simply leave the userland pointer there; worse, that pointer might be
> to native or to compat variants, and layouts _are_ different.  Thus those
> if (MSG_CMSG_COMPAT & msg->msg_flags) in net/core/scm.c...
> 	* kernel-side users of ->sendmsg() do not depend on setfs() for
> access to their ->msg_control, simply because ->sendmsg() won't be using
> copy_from_user()/get_user() to access it anyway.
> 	* kernel-side users of ->recvmsg() are less lucky - most of them
> don't give a damn either (they have NULL ->msg_control), but there's an
> exception (somewhere in sunrpc, IIRC).  So there we need to keep
> playing with setfs(), even though the data side would be just fine without
> that.

Apart from any other code that is using the interface.
I know you guys don't do anything to help out of tree code, but removing the setfs()
stuff from the kernel_recvmsg() code would break anything using sctp.
It shouldn't need some code lurking in sunrpc for you to leave the setfs().

In any case, how much does the setfs() cost?
I suspect it is just modifying a flag in 'current'.

A comment in kernel_recvmsg() saying that the setfs() is for msg_control
might be useful.
Then one in kelnel_sendmsg() saying that setfs() isn't needed because
msg_control is always kernel - just to avoid any confusion.

	David
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html