From: Al Viro <viro@ZenIV.linux.org.uk>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: George Spelvin <linux@horizon.com>,
linux-fsdevel <linux-fsdevel@vger.kernel.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
David Miller <davem@davemloft.net>,
Marcel Holtmann <marcel@holtmann.org>
Subject: Re: Update of file offset on write() etc. is non-atomic with I/O
Date: Mon, 10 Mar 2014 15:55:20 +0000 [thread overview]
Message-ID: <20140310155520.GA8443@ZenIV.linux.org.uk> (raw)
In-Reply-To: <20140305000411.GV18016@ZenIV.linux.org.uk>
[-- Attachment #1: Type: text/plain, Size: 3663 bytes --]
On Wed, Mar 05, 2014 at 12:04:11AM +0000, Al Viro wrote:
> There's also a pile of crap around sockfd_lookup/sockfd_put, related
> to that. Moreover, there's net/compat.c, which probably ought to
> have the compat syscalls themselves moved to net/socket.c (under
> ifdef CONFIG_COMPAT) and switched to sockfd_lookup_light().
> There's l2tp_tunnel_sock_lookup(), which is simply broken - it assumes
> that if tunnel->fd still resolves to a socket, that socket must
> be l2tp one. Trivial to drive into BUG_ON(), in queue_work() callback,
> no less... There's bluetooth, assuming that pretty much the same
> (that if it got a file descriptor that resolves to a socket, it must
> be a bluetooth one). BTW, I wonder what will happen if one gives
> iscsi_sw_tcp_conn_bind() descriptor of a socket of sufficiently
> weird sort...
>
> Then there's staging/usbip with its sockfd_to_socket(), which is more or
> less parallel to sockfd_lookup(). And open-coded analogs in nbd and
> ncpfs...
OK, I've gone through most of that; bluetooth is, indeed, oopsable (as simple
as e.g.
int sv[2];
int fd = socket(PF_BLUETOOTH, SOCK_RAW, BTPROTO_CMTP);
struct cmtp_connadd_req r = {};
socketpair(PF_LOCAL, SOCK_STREAM, 0, sv);
r.sock = sv[0];
ioctl(fd, CMTPCONNADD, (unsigned long)&r);
and similar with BNEP) and that one is easy to fix. l2tp I'd rather leave
for net folks to deal with - the problem there is that we stash sock *and*
descriptor number into struct l2tp_tunnel in l2tp_tunnel_create() and expect
l2tp_tunnel_sock_lookup() to find that descriptor (tunnel->fd) resolving
to nothing (if it got already closed) or to the same socket. Unfortunately,
the caller (l2tp_tunnel_del_work()) expects to find l2tp socket in the latter
case, so having it replaced with unrelated socket will do nasty things
to that caller. It looks rather silly, actually - the actual fuckup happens
when l2tp_tunnel_del_work() passes what has come from socket->sk to
l2tp_tunnel_sock_put(), which does
struct l2tp_tunnel *tunnel = l2tp_sock_to_tunnel(sk);
to find the tunnel its caller already had. Looks too convoluted for its
own good, and my first inclination would be to collapse l2tp_tunnel_sock_*
into the (only) caller, but I'm not sure if I'm not missing some subtle
race prevention in those back-and-forth lookups. In any case, it can
lead to l2tp_sock_to_tunnel() called on a sock that has nothing to do with
l2tp, so we do have a bug there.
I've attached bluetooth fixes; this stuff is obviously better off in one of
the net trees. Not sure if it's worth Cc:stable - up to Marcel and Davem.
These bugs are oopsable, but you need CAP_NET_ADMIN to step into those...
I think that what's in vfs.git#for-linus right now is OK to pull; it survives
all the beating I could think of. Linus, could you pull from the usual place?
git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git for-linus
Shortlog:
Al Viro (3):
ocfs2 syncs the wrong range...
sockfd_lookup_light(): switch to fdget^W^Waway from fget_light
get rid of fget_light()
Linus Torvalds (1):
vfs: atomic f_pos accesses as per POSIX
Diffstat:
fs/file.c | 56 +++++++++++++++++++++++++++++++++++++++++++-------------
fs/file_table.c | 1 +
fs/namei.c | 2 +-
fs/ocfs2/file.c | 8 ++++----
fs/open.c | 4 ++++
fs/read_write.c | 40 ++++++++++++++++++++++++++--------------
include/linux/file.h | 27 +++++++++++++++------------
include/linux/fs.h | 8 ++++++--
net/socket.c | 13 +++++++------
9 files changed, 107 insertions(+), 52 deletions(-)
[-- Attachment #2: 0001-bluetooth-hidp_connection_add-unsafe-use-of-l2cap_pi.patch --]
[-- Type: text/plain, Size: 1026 bytes --]
>From f49d9ab3220ece0b635b18212bfc44444e9b5f41 Mon Sep 17 00:00:00 2001
From: Al Viro <viro@zeniv.linux.org.uk>
Date: Sun, 9 Mar 2014 13:11:59 -0400
Subject: [PATCH 1/3] bluetooth: hidp_connection_add() unsafe use of
l2cap_pi()
it's OK after we'd verified the sockets, but not before that.
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
net/bluetooth/hidp/core.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index d9fb934..6134618 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -1332,13 +1332,14 @@ int hidp_connection_add(struct hidp_connadd_req *req,
{
struct hidp_session *session;
struct l2cap_conn *conn;
- struct l2cap_chan *chan = l2cap_pi(ctrl_sock->sk)->chan;
+ struct l2cap_chan *chan;
int ret;
ret = hidp_verify_sockets(ctrl_sock, intr_sock);
if (ret)
return ret;
+ chan = l2cap_pi(ctrl_sock->sk)->chan;
conn = NULL;
l2cap_chan_lock(chan);
if (chan->conn) {
--
1.7.10.4
[-- Attachment #3: 0002-cmtp-cmtp_add_connection-should-verify-that-it-s-dea.patch --]
[-- Type: text/plain, Size: 1007 bytes --]
>From 790f94a74f8214baab44eb346a346640e6335319 Mon Sep 17 00:00:00 2001
From: Al Viro <viro@zeniv.linux.org.uk>
Date: Mon, 10 Mar 2014 10:50:10 -0400
Subject: [PATCH 2/3] cmtp: cmtp_add_connection() should verify that it's
dealing with l2cap socket
... rather than relying on ciptool(8) never passing it anything else. Give
it e.g. an AF_UNIX connected socket (from socketpair(2)) and it'll oops,
trying to evaluate &l2cap_pi(sock->sk)->chan->dst...
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
net/bluetooth/cmtp/core.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/net/bluetooth/cmtp/core.c b/net/bluetooth/cmtp/core.c
index 67fe5e8..fd57db8 100644
--- a/net/bluetooth/cmtp/core.c
+++ b/net/bluetooth/cmtp/core.c
@@ -333,6 +333,8 @@ int cmtp_add_connection(struct cmtp_connadd_req *req, struct socket *sock)
int i, err;
BT_DBG("");
+ if (!l2cap_is_socket(sock))
+ return -EBADFD;
session = kzalloc(sizeof(struct cmtp_session), GFP_KERNEL);
if (!session)
--
1.7.10.4
[-- Attachment #4: 0003-bnep-bnep_add_connection-should-verify-that-it-s-dea.patch --]
[-- Type: text/plain, Size: 849 bytes --]
>From 4d86300249b507f27e8c55c0d3bf1fa2653f9b17 Mon Sep 17 00:00:00 2001
From: Al Viro <viro@zeniv.linux.org.uk>
Date: Mon, 10 Mar 2014 11:08:35 -0400
Subject: [PATCH 3/3] bnep: bnep_add_connection() should verify that it's
dealing with l2cap socket
same story as cmtp
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
net/bluetooth/bnep/core.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/bluetooth/bnep/core.c b/net/bluetooth/bnep/core.c
index a841d3e..c7a19a1 100644
--- a/net/bluetooth/bnep/core.c
+++ b/net/bluetooth/bnep/core.c
@@ -533,6 +533,9 @@ int bnep_add_connection(struct bnep_connadd_req *req, struct socket *sock)
BT_DBG("");
+ if (!l2cap_is_socket(sock))
+ return -EBADFD;
+
baswap((void *) dst, &l2cap_pi(sock->sk)->chan->dst);
baswap((void *) src, &l2cap_pi(sock->sk)->chan->src);
--
1.7.10.4
next prev parent reply other threads:[~2014-03-10 15:55 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-03-03 21:03 Update of file offset on write() etc. is non-atomic with I/O George Spelvin
2014-03-03 21:26 ` Al Viro
2014-03-03 21:52 ` Linus Torvalds
2014-03-03 22:01 ` Al Viro
2014-03-03 22:17 ` Linus Torvalds
2014-03-03 23:28 ` Al Viro
2014-03-03 23:34 ` Linus Torvalds
2014-03-03 23:42 ` Al Viro
2014-03-03 23:59 ` Linus Torvalds
2014-03-04 0:23 ` Al Viro
2014-03-04 0:42 ` Linus Torvalds
2014-03-04 1:05 ` Al Viro
2014-03-04 20:00 ` Al Viro
2014-03-04 21:17 ` Linus Torvalds
2014-03-05 0:04 ` Al Viro
2014-03-10 15:55 ` Al Viro [this message]
2014-03-03 22:55 ` Linus Torvalds
2014-03-03 23:23 ` Linus Torvalds
2014-03-03 23:39 ` Al Viro
2014-03-03 23:54 ` Linus Torvalds
2014-03-03 23:54 ` Al Viro
2014-03-04 20:11 ` Cedric Blancher
2014-03-04 0:07 ` George Spelvin
2014-05-04 7:04 ` Michael Kerrisk
[not found] <a8df285f-de7f-4a3a-9a19-e0ad07ab3a5c@blur>
2014-02-20 18:15 ` Zuckerman, Boris
2014-02-20 18:29 ` Al Viro
2014-02-21 6:01 ` Michael Kerrisk (man-pages)
2014-02-23 1:18 ` Kevin Easton
2014-02-23 7:38 ` Michael Kerrisk (man-pages)
-- strict thread matches above, loose matches on Subject: below --
2014-02-17 15:41 Michael Kerrisk (man-pages)
2014-02-18 13:00 ` Michael Kerrisk
2014-02-20 17:14 ` Linus Torvalds
2014-03-03 17:36 ` Linus Torvalds
2014-03-03 21:45 ` Al Viro
2014-03-03 21:56 ` Linus Torvalds
2014-03-03 22:09 ` Al Viro
2014-03-03 22:20 ` Linus Torvalds
2014-03-03 22:01 ` Linus Torvalds
2014-03-03 22:10 ` Al Viro
2014-03-03 22:22 ` Linus Torvalds
2014-03-06 15:03 ` Michael Kerrisk (man-pages)
2014-03-07 3:38 ` Yongzhi Pan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20140310155520.GA8443@ZenIV.linux.org.uk \
--to=viro@zeniv.linux.org.uk \
--cc=davem@davemloft.net \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@horizon.com \
--cc=marcel@holtmann.org \
--cc=torvalds@linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).