* [PATCH v2 net-next 0/2] kcm: fix two syzcaller issues
@ 2018-01-24 20:35 Tom Herbert
2018-01-24 20:35 ` [PATCH v2 net-next 1/2] kcm: Only allow TCP sockets to be attached to a KCM mux Tom Herbert
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Tom Herbert @ 2018-01-24 20:35 UTC (permalink / raw
To: davem; +Cc: netdev, rohit, jchapman, g.nault, Tom Herbert
In this patch set:
- Don't allow attaching non-TCP or listener sockets to a KCM mux.
- In kcm_attach Check if sk_user_data is already set. This is
under lock to avoid race conditions. More work is need to make
all of the users of sk_user_data to use the same locking.
- v2
Remove unncessary check for not PF_KCM in kcm_attach (suggested by
Guillaume Nault)
Tom Herbert (2):
kcm: Only allow TCP sockets to be attached to a KCM mux
kcm: Check if sk_user_data already set in kcm_attach
net/kcm/kcmsock.c | 25 +++++++++++++++++++++----
1 file changed, 21 insertions(+), 4 deletions(-)
--
2.11.0
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 net-next 1/2] kcm: Only allow TCP sockets to be attached to a KCM mux
2018-01-24 20:35 [PATCH v2 net-next 0/2] kcm: fix two syzcaller issues Tom Herbert
@ 2018-01-24 20:35 ` Tom Herbert
2018-01-24 20:49 ` Eric Dumazet
2018-01-24 20:35 ` [PATCH v2 net-next 2/2] kcm: Check if sk_user_data already set in kcm_attach Tom Herbert
2018-01-24 20:54 ` [PATCH v2 net-next 0/2] kcm: fix two syzcaller issues David Miller
2 siblings, 1 reply; 7+ messages in thread
From: Tom Herbert @ 2018-01-24 20:35 UTC (permalink / raw
To: davem; +Cc: netdev, rohit, jchapman, g.nault, Tom Herbert
TCP sockets for IPv4 and IPv6 that are not listeners or in closed
stated are allowed to be attached to a KCM mux.
Fixes: ab7ac4eb9832 ("kcm: Kernel Connection Multiplexor module")
Reported-by: syzbot+8865eaff7f9acd593945@syzkaller.appspotmail.com
Signed-off-by: Tom Herbert <tom@quantonium.net>
---
net/kcm/kcmsock.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/net/kcm/kcmsock.c b/net/kcm/kcmsock.c
index d4e98f20fc2a..7632797fb68e 100644
--- a/net/kcm/kcmsock.c
+++ b/net/kcm/kcmsock.c
@@ -1387,8 +1387,13 @@ static int kcm_attach(struct socket *sock, struct socket *csock,
if (!csk)
return -EINVAL;
- /* We must prevent loops or risk deadlock ! */
- if (csk->sk_family == PF_KCM)
+ /* Only allow TCP sockets to be attached for now */
+ if ((csk->sk_family != AF_INET && csk->sk_family != AF_INET6) ||
+ csk->sk_protocol != IPPROTO_TCP)
+ return -EOPNOTSUPP;
+
+ /* Don't allow listeners or closed sockets */
+ if (csk->sk_state == TCP_LISTEN || csk->sk_state == TCP_CLOSE)
return -EOPNOTSUPP;
psock = kmem_cache_zalloc(kcm_psockp, GFP_KERNEL);
--
2.11.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2 net-next 2/2] kcm: Check if sk_user_data already set in kcm_attach
2018-01-24 20:35 [PATCH v2 net-next 0/2] kcm: fix two syzcaller issues Tom Herbert
2018-01-24 20:35 ` [PATCH v2 net-next 1/2] kcm: Only allow TCP sockets to be attached to a KCM mux Tom Herbert
@ 2018-01-24 20:35 ` Tom Herbert
2018-01-24 20:52 ` Eric Dumazet
2018-01-26 21:07 ` Eric Dumazet
2018-01-24 20:54 ` [PATCH v2 net-next 0/2] kcm: fix two syzcaller issues David Miller
2 siblings, 2 replies; 7+ messages in thread
From: Tom Herbert @ 2018-01-24 20:35 UTC (permalink / raw
To: davem; +Cc: netdev, rohit, jchapman, g.nault, Tom Herbert
This is needed to prevent sk_user_data being overwritten.
The check is done under the callback lock. This should prevent
a socket from being attached twice to a KCM mux. It also prevents
a socket from being attached for other use cases of sk_user_data
as long as the other cases set sk_user_data under the lock.
Followup work is needed to unify all the use cases of sk_user_data
to use the same locking.
Reported-by: syzbot+114b15f2be420a8886c3@syzkaller.appspotmail.com
Fixes: ab7ac4eb9832 ("kcm: Kernel Connection Multiplexor module")
Signed-off-by: Tom Herbert <tom@quantonium.net>
---
net/kcm/kcmsock.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/net/kcm/kcmsock.c b/net/kcm/kcmsock.c
index 7632797fb68e..4a8d407f8902 100644
--- a/net/kcm/kcmsock.c
+++ b/net/kcm/kcmsock.c
@@ -1410,9 +1410,18 @@ static int kcm_attach(struct socket *sock, struct socket *csock,
return err;
}
- sock_hold(csk);
-
write_lock_bh(&csk->sk_callback_lock);
+
+ /* Check if sk_user_data is aready by KCM or someone else.
+ * Must be done under lock to prevent race conditions.
+ */
+ if (csk->sk_user_data) {
+ write_unlock_bh(&csk->sk_callback_lock);
+ strp_done(&psock->strp);
+ kmem_cache_free(kcm_psockp, psock);
+ return -EALREADY;
+ }
+
psock->save_data_ready = csk->sk_data_ready;
psock->save_write_space = csk->sk_write_space;
psock->save_state_change = csk->sk_state_change;
@@ -1420,8 +1429,11 @@ static int kcm_attach(struct socket *sock, struct socket *csock,
csk->sk_data_ready = psock_data_ready;
csk->sk_write_space = psock_write_space;
csk->sk_state_change = psock_state_change;
+
write_unlock_bh(&csk->sk_callback_lock);
+ sock_hold(csk);
+
/* Finished initialization, now add the psock to the MUX. */
spin_lock_bh(&mux->lock);
head = &mux->psocks;
--
2.11.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 net-next 1/2] kcm: Only allow TCP sockets to be attached to a KCM mux
2018-01-24 20:35 ` [PATCH v2 net-next 1/2] kcm: Only allow TCP sockets to be attached to a KCM mux Tom Herbert
@ 2018-01-24 20:49 ` Eric Dumazet
0 siblings, 0 replies; 7+ messages in thread
From: Eric Dumazet @ 2018-01-24 20:49 UTC (permalink / raw
To: Tom Herbert, davem; +Cc: netdev, rohit, jchapman, g.nault
On Wed, 2018-01-24 at 12:35 -0800, Tom Herbert wrote:
> TCP sockets for IPv4 and IPv6 that are not listeners or in closed
> stated are allowed to be attached to a KCM mux.
>
> Fixes: ab7ac4eb9832 ("kcm: Kernel Connection Multiplexor module")
> Reported-by: syzbot+8865eaff7f9acd593945@syzkaller.appspotmail.com
> Signed-off-by: Tom Herbert <tom@quantonium.net>
> ---
> net/kcm/kcmsock.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/net/kcm/kcmsock.c b/net/kcm/kcmsock.c
> index d4e98f20fc2a..7632797fb68e 100644
> --- a/net/kcm/kcmsock.c
> +++ b/net/kcm/kcmsock.c
> @@ -1387,8 +1387,13 @@ static int kcm_attach(struct socket *sock, struct socket *csock,
> if (!csk)
> return -EINVAL;
>
> - /* We must prevent loops or risk deadlock ! */
> - if (csk->sk_family == PF_KCM)
> + /* Only allow TCP sockets to be attached for now */
> + if ((csk->sk_family != AF_INET && csk->sk_family != AF_INET6) ||
> + csk->sk_protocol != IPPROTO_TCP)
> + return -EOPNOTSUPP;
> +
> + /* Don't allow listeners or closed sockets */
> + if (csk->sk_state == TCP_LISTEN || csk->sk_state == TCP_CLOSE)
> return -EOPNOTSUPP;
>
> psock = kmem_cache_zalloc(kcm_psockp, GFP_KERNEL);
Reviewed-by: Eric Dumazet <edumazet@google.com>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 net-next 2/2] kcm: Check if sk_user_data already set in kcm_attach
2018-01-24 20:35 ` [PATCH v2 net-next 2/2] kcm: Check if sk_user_data already set in kcm_attach Tom Herbert
@ 2018-01-24 20:52 ` Eric Dumazet
2018-01-26 21:07 ` Eric Dumazet
1 sibling, 0 replies; 7+ messages in thread
From: Eric Dumazet @ 2018-01-24 20:52 UTC (permalink / raw
To: Tom Herbert, davem; +Cc: netdev, rohit, jchapman, g.nault
On Wed, 2018-01-24 at 12:35 -0800, Tom Herbert wrote:
> This is needed to prevent sk_user_data being overwritten.
> The check is done under the callback lock. This should prevent
> a socket from being attached twice to a KCM mux. It also prevents
> a socket from being attached for other use cases of sk_user_data
> as long as the other cases set sk_user_data under the lock.
> Followup work is needed to unify all the use cases of sk_user_data
> to use the same locking.
>
Reviewed-by: Eric Dumazet <edumazet@google.com>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 net-next 0/2] kcm: fix two syzcaller issues
2018-01-24 20:35 [PATCH v2 net-next 0/2] kcm: fix two syzcaller issues Tom Herbert
2018-01-24 20:35 ` [PATCH v2 net-next 1/2] kcm: Only allow TCP sockets to be attached to a KCM mux Tom Herbert
2018-01-24 20:35 ` [PATCH v2 net-next 2/2] kcm: Check if sk_user_data already set in kcm_attach Tom Herbert
@ 2018-01-24 20:54 ` David Miller
2 siblings, 0 replies; 7+ messages in thread
From: David Miller @ 2018-01-24 20:54 UTC (permalink / raw
To: tom; +Cc: netdev, rohit, jchapman, g.nault
From: Tom Herbert <tom@quantonium.net>
Date: Wed, 24 Jan 2018 12:35:39 -0800
> In this patch set:
>
> - Don't allow attaching non-TCP or listener sockets to a KCM mux.
> - In kcm_attach Check if sk_user_data is already set. This is
> under lock to avoid race conditions. More work is need to make
> all of the users of sk_user_data to use the same locking.
>
> - v2
> Remove unncessary check for not PF_KCM in kcm_attach (suggested by
> Guillaume Nault)
Series applied, thanks Tom.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 net-next 2/2] kcm: Check if sk_user_data already set in kcm_attach
2018-01-24 20:35 ` [PATCH v2 net-next 2/2] kcm: Check if sk_user_data already set in kcm_attach Tom Herbert
2018-01-24 20:52 ` Eric Dumazet
@ 2018-01-26 21:07 ` Eric Dumazet
1 sibling, 0 replies; 7+ messages in thread
From: Eric Dumazet @ 2018-01-26 21:07 UTC (permalink / raw
To: Tom Herbert, davem; +Cc: netdev, rohit, jchapman, g.nault
On Wed, 2018-01-24 at 12:35 -0800, Tom Herbert wrote:
> This is needed to prevent sk_user_data being overwritten.
> The check is done under the callback lock. This should prevent
> a socket from being attached twice to a KCM mux. It also prevents
> a socket from being attached for other use cases of sk_user_data
> as long as the other cases set sk_user_data under the lock.
> Followup work is needed to unify all the use cases of sk_user_data
> to use the same locking.
>
> Reported-by: syzbot+114b15f2be420a8886c3@syzkaller.appspotmail.com
> Fixes: ab7ac4eb9832 ("kcm: Kernel Connection Multiplexor module")
> Signed-off-by: Tom Herbert <tom@quantonium.net>
> ---
> net/kcm/kcmsock.c | 16 ++++++++++++++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/net/kcm/kcmsock.c b/net/kcm/kcmsock.c
> index 7632797fb68e..4a8d407f8902 100644
> --- a/net/kcm/kcmsock.c
> +++ b/net/kcm/kcmsock.c
> @@ -1410,9 +1410,18 @@ static int kcm_attach(struct socket *sock, struct socket *csock,
> return err;
> }
>
> - sock_hold(csk);
> -
> write_lock_bh(&csk->sk_callback_lock);
> +
> + /* Check if sk_user_data is aready by KCM or someone else.
> + * Must be done under lock to prevent race conditions.
> + */
> + if (csk->sk_user_data) {
> + write_unlock_bh(&csk->sk_callback_lock);
> + strp_done(&psock->strp);
Although it seems psock->strp->stopped wont be set ?
We should hit WARN_ON(!strp->stopped);
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-01-26 21:08 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-01-24 20:35 [PATCH v2 net-next 0/2] kcm: fix two syzcaller issues Tom Herbert
2018-01-24 20:35 ` [PATCH v2 net-next 1/2] kcm: Only allow TCP sockets to be attached to a KCM mux Tom Herbert
2018-01-24 20:49 ` Eric Dumazet
2018-01-24 20:35 ` [PATCH v2 net-next 2/2] kcm: Check if sk_user_data already set in kcm_attach Tom Herbert
2018-01-24 20:52 ` Eric Dumazet
2018-01-26 21:07 ` Eric Dumazet
2018-01-24 20:54 ` [PATCH v2 net-next 0/2] kcm: fix two syzcaller issues David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.