All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net] sctp: use memdup_user instead of vmemdup_user
@ 2019-03-20  6:49 ` Xin Long
  0 siblings, 0 replies; 6+ messages in thread
From: Xin Long @ 2019-03-20  6:49 UTC (permalink / raw)
  To: network dev, linux-sctp
  Cc: davem, Marcelo Ricardo Leitner, Neil Horman, syzkaller

In sctp_setsockopt_bindx()/__sctp_setsockopt_connectx(), it allocates
memory with addrs_size which is passed from userspace. We used flag
GFP_USER to put some more restrictions on it in Commit cacc06215271
("sctp: use GFP_USER for user-controlled kmalloc").

However, since Commit c981f254cc82 ("sctp: use vmemdup_user() rather
than badly open-coding memdup_user()"), vmemdup_user() has been used,
which doesn't check GFP_USER flag when goes to vmalloc_*(). So when
addrs_size is a huge value, it could exhaust memory and even trigger
oom killer.

This patch is to use memdup_user() instead, in which GFP_USER would
work to limit the memory allocation with a huge addrs_size.

Note we can't fix it by limiting 'addrs_size', as there's no demand
for it from RFC.

Reported-by: syzbot+ec1b7575afef85a0e5ca@syzkaller.appspotmail.com
Fixes: c981f254cc82 ("sctp: use vmemdup_user() rather than badly open-coding memdup_user()")
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/socket.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 6140471..09ad5b2 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -999,7 +999,7 @@ static int sctp_setsockopt_bindx(struct sock *sk,
 	if (unlikely(addrs_size <= 0))
 		return -EINVAL;
 
-	kaddrs = vmemdup_user(addrs, addrs_size);
+	kaddrs = memdup_user(addrs, addrs_size);
 	if (unlikely(IS_ERR(kaddrs)))
 		return PTR_ERR(kaddrs);
 
@@ -1007,7 +1007,7 @@ static int sctp_setsockopt_bindx(struct sock *sk,
 	addr_buf = kaddrs;
 	while (walk_size < addrs_size) {
 		if (walk_size + sizeof(sa_family_t) > addrs_size) {
-			kvfree(kaddrs);
+			kfree(kaddrs);
 			return -EINVAL;
 		}
 
@@ -1018,7 +1018,7 @@ static int sctp_setsockopt_bindx(struct sock *sk,
 		 * causes the address buffer to overflow return EINVAL.
 		 */
 		if (!af || (walk_size + af->sockaddr_len) > addrs_size) {
-			kvfree(kaddrs);
+			kfree(kaddrs);
 			return -EINVAL;
 		}
 		addrcnt++;
@@ -1054,7 +1054,7 @@ static int sctp_setsockopt_bindx(struct sock *sk,
 	}
 
 out:
-	kvfree(kaddrs);
+	kfree(kaddrs);
 
 	return err;
 }
@@ -1329,7 +1329,7 @@ static int __sctp_setsockopt_connectx(struct sock *sk,
 	if (unlikely(addrs_size <= 0))
 		return -EINVAL;
 
-	kaddrs = vmemdup_user(addrs, addrs_size);
+	kaddrs = memdup_user(addrs, addrs_size);
 	if (unlikely(IS_ERR(kaddrs)))
 		return PTR_ERR(kaddrs);
 
@@ -1349,7 +1349,7 @@ static int __sctp_setsockopt_connectx(struct sock *sk,
 	err = __sctp_connect(sk, kaddrs, addrs_size, flags, assoc_id);
 
 out_free:
-	kvfree(kaddrs);
+	kfree(kaddrs);
 
 	return err;
 }
-- 
2.1.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH net] sctp: use memdup_user instead of vmemdup_user
@ 2019-03-20  6:49 ` Xin Long
  0 siblings, 0 replies; 6+ messages in thread
From: Xin Long @ 2019-03-20  6:49 UTC (permalink / raw)
  To: network dev, linux-sctp
  Cc: davem, Marcelo Ricardo Leitner, Neil Horman, syzkaller

In sctp_setsockopt_bindx()/__sctp_setsockopt_connectx(), it allocates
memory with addrs_size which is passed from userspace. We used flag
GFP_USER to put some more restrictions on it in Commit cacc06215271
("sctp: use GFP_USER for user-controlled kmalloc").

However, since Commit c981f254cc82 ("sctp: use vmemdup_user() rather
than badly open-coding memdup_user()"), vmemdup_user() has been used,
which doesn't check GFP_USER flag when goes to vmalloc_*(). So when
addrs_size is a huge value, it could exhaust memory and even trigger
oom killer.

This patch is to use memdup_user() instead, in which GFP_USER would
work to limit the memory allocation with a huge addrs_size.

Note we can't fix it by limiting 'addrs_size', as there's no demand
for it from RFC.

Reported-by: syzbot+ec1b7575afef85a0e5ca@syzkaller.appspotmail.com
Fixes: c981f254cc82 ("sctp: use vmemdup_user() rather than badly open-coding memdup_user()")
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/socket.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 6140471..09ad5b2 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -999,7 +999,7 @@ static int sctp_setsockopt_bindx(struct sock *sk,
 	if (unlikely(addrs_size <= 0))
 		return -EINVAL;
 
-	kaddrs = vmemdup_user(addrs, addrs_size);
+	kaddrs = memdup_user(addrs, addrs_size);
 	if (unlikely(IS_ERR(kaddrs)))
 		return PTR_ERR(kaddrs);
 
@@ -1007,7 +1007,7 @@ static int sctp_setsockopt_bindx(struct sock *sk,
 	addr_buf = kaddrs;
 	while (walk_size < addrs_size) {
 		if (walk_size + sizeof(sa_family_t) > addrs_size) {
-			kvfree(kaddrs);
+			kfree(kaddrs);
 			return -EINVAL;
 		}
 
@@ -1018,7 +1018,7 @@ static int sctp_setsockopt_bindx(struct sock *sk,
 		 * causes the address buffer to overflow return EINVAL.
 		 */
 		if (!af || (walk_size + af->sockaddr_len) > addrs_size) {
-			kvfree(kaddrs);
+			kfree(kaddrs);
 			return -EINVAL;
 		}
 		addrcnt++;
@@ -1054,7 +1054,7 @@ static int sctp_setsockopt_bindx(struct sock *sk,
 	}
 
 out:
-	kvfree(kaddrs);
+	kfree(kaddrs);
 
 	return err;
 }
@@ -1329,7 +1329,7 @@ static int __sctp_setsockopt_connectx(struct sock *sk,
 	if (unlikely(addrs_size <= 0))
 		return -EINVAL;
 
-	kaddrs = vmemdup_user(addrs, addrs_size);
+	kaddrs = memdup_user(addrs, addrs_size);
 	if (unlikely(IS_ERR(kaddrs)))
 		return PTR_ERR(kaddrs);
 
@@ -1349,7 +1349,7 @@ static int __sctp_setsockopt_connectx(struct sock *sk,
 	err = __sctp_connect(sk, kaddrs, addrs_size, flags, assoc_id);
 
 out_free:
-	kvfree(kaddrs);
+	kfree(kaddrs);
 
 	return err;
 }
-- 
2.1.0

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH net] sctp: use memdup_user instead of vmemdup_user
  2019-03-20  6:49 ` Xin Long
@ 2019-03-20 11:24   ` Neil Horman
  -1 siblings, 0 replies; 6+ messages in thread
From: Neil Horman @ 2019-03-20 11:24 UTC (permalink / raw)
  To: Xin Long
  Cc: network dev, linux-sctp, davem, Marcelo Ricardo Leitner, syzkaller

On Wed, Mar 20, 2019 at 02:49:38PM +0800, Xin Long wrote:
> In sctp_setsockopt_bindx()/__sctp_setsockopt_connectx(), it allocates
> memory with addrs_size which is passed from userspace. We used flag
> GFP_USER to put some more restrictions on it in Commit cacc06215271
> ("sctp: use GFP_USER for user-controlled kmalloc").
> 
> However, since Commit c981f254cc82 ("sctp: use vmemdup_user() rather
> than badly open-coding memdup_user()"), vmemdup_user() has been used,
> which doesn't check GFP_USER flag when goes to vmalloc_*(). So when
> addrs_size is a huge value, it could exhaust memory and even trigger
> oom killer.
> 
> This patch is to use memdup_user() instead, in which GFP_USER would
> work to limit the memory allocation with a huge addrs_size.
> 
> Note we can't fix it by limiting 'addrs_size', as there's no demand
> for it from RFC.
> 
> Reported-by: syzbot+ec1b7575afef85a0e5ca@syzkaller.appspotmail.com
> Fixes: c981f254cc82 ("sctp: use vmemdup_user() rather than badly open-coding memdup_user()")
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  net/sctp/socket.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 6140471..09ad5b2 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -999,7 +999,7 @@ static int sctp_setsockopt_bindx(struct sock *sk,
>  	if (unlikely(addrs_size <= 0))
>  		return -EINVAL;
>  
> -	kaddrs = vmemdup_user(addrs, addrs_size);
> +	kaddrs = memdup_user(addrs, addrs_size);
>  	if (unlikely(IS_ERR(kaddrs)))
>  		return PTR_ERR(kaddrs);
>  
> @@ -1007,7 +1007,7 @@ static int sctp_setsockopt_bindx(struct sock *sk,
>  	addr_buf = kaddrs;
>  	while (walk_size < addrs_size) {
>  		if (walk_size + sizeof(sa_family_t) > addrs_size) {
> -			kvfree(kaddrs);
> +			kfree(kaddrs);
>  			return -EINVAL;
>  		}
>  
> @@ -1018,7 +1018,7 @@ static int sctp_setsockopt_bindx(struct sock *sk,
>  		 * causes the address buffer to overflow return EINVAL.
>  		 */
>  		if (!af || (walk_size + af->sockaddr_len) > addrs_size) {
> -			kvfree(kaddrs);
> +			kfree(kaddrs);
>  			return -EINVAL;
>  		}
>  		addrcnt++;
> @@ -1054,7 +1054,7 @@ static int sctp_setsockopt_bindx(struct sock *sk,
>  	}
>  
>  out:
> -	kvfree(kaddrs);
> +	kfree(kaddrs);
>  
>  	return err;
>  }
> @@ -1329,7 +1329,7 @@ static int __sctp_setsockopt_connectx(struct sock *sk,
>  	if (unlikely(addrs_size <= 0))
>  		return -EINVAL;
>  
> -	kaddrs = vmemdup_user(addrs, addrs_size);
> +	kaddrs = memdup_user(addrs, addrs_size);
>  	if (unlikely(IS_ERR(kaddrs)))
>  		return PTR_ERR(kaddrs);
>  
> @@ -1349,7 +1349,7 @@ static int __sctp_setsockopt_connectx(struct sock *sk,
>  	err = __sctp_connect(sk, kaddrs, addrs_size, flags, assoc_id);
>  
>  out_free:
> -	kvfree(kaddrs);
> +	kfree(kaddrs);
>  
>  	return err;
>  }
> -- 
> 2.1.0
> 
> 
Acked-by: Neil Horman <nhorman@tuxdriver.com>


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH net] sctp: use memdup_user instead of vmemdup_user
@ 2019-03-20 11:24   ` Neil Horman
  0 siblings, 0 replies; 6+ messages in thread
From: Neil Horman @ 2019-03-20 11:24 UTC (permalink / raw)
  To: Xin Long
  Cc: network dev, linux-sctp, davem, Marcelo Ricardo Leitner, syzkaller

On Wed, Mar 20, 2019 at 02:49:38PM +0800, Xin Long wrote:
> In sctp_setsockopt_bindx()/__sctp_setsockopt_connectx(), it allocates
> memory with addrs_size which is passed from userspace. We used flag
> GFP_USER to put some more restrictions on it in Commit cacc06215271
> ("sctp: use GFP_USER for user-controlled kmalloc").
> 
> However, since Commit c981f254cc82 ("sctp: use vmemdup_user() rather
> than badly open-coding memdup_user()"), vmemdup_user() has been used,
> which doesn't check GFP_USER flag when goes to vmalloc_*(). So when
> addrs_size is a huge value, it could exhaust memory and even trigger
> oom killer.
> 
> This patch is to use memdup_user() instead, in which GFP_USER would
> work to limit the memory allocation with a huge addrs_size.
> 
> Note we can't fix it by limiting 'addrs_size', as there's no demand
> for it from RFC.
> 
> Reported-by: syzbot+ec1b7575afef85a0e5ca@syzkaller.appspotmail.com
> Fixes: c981f254cc82 ("sctp: use vmemdup_user() rather than badly open-coding memdup_user()")
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  net/sctp/socket.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 6140471..09ad5b2 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -999,7 +999,7 @@ static int sctp_setsockopt_bindx(struct sock *sk,
>  	if (unlikely(addrs_size <= 0))
>  		return -EINVAL;
>  
> -	kaddrs = vmemdup_user(addrs, addrs_size);
> +	kaddrs = memdup_user(addrs, addrs_size);
>  	if (unlikely(IS_ERR(kaddrs)))
>  		return PTR_ERR(kaddrs);
>  
> @@ -1007,7 +1007,7 @@ static int sctp_setsockopt_bindx(struct sock *sk,
>  	addr_buf = kaddrs;
>  	while (walk_size < addrs_size) {
>  		if (walk_size + sizeof(sa_family_t) > addrs_size) {
> -			kvfree(kaddrs);
> +			kfree(kaddrs);
>  			return -EINVAL;
>  		}
>  
> @@ -1018,7 +1018,7 @@ static int sctp_setsockopt_bindx(struct sock *sk,
>  		 * causes the address buffer to overflow return EINVAL.
>  		 */
>  		if (!af || (walk_size + af->sockaddr_len) > addrs_size) {
> -			kvfree(kaddrs);
> +			kfree(kaddrs);
>  			return -EINVAL;
>  		}
>  		addrcnt++;
> @@ -1054,7 +1054,7 @@ static int sctp_setsockopt_bindx(struct sock *sk,
>  	}
>  
>  out:
> -	kvfree(kaddrs);
> +	kfree(kaddrs);
>  
>  	return err;
>  }
> @@ -1329,7 +1329,7 @@ static int __sctp_setsockopt_connectx(struct sock *sk,
>  	if (unlikely(addrs_size <= 0))
>  		return -EINVAL;
>  
> -	kaddrs = vmemdup_user(addrs, addrs_size);
> +	kaddrs = memdup_user(addrs, addrs_size);
>  	if (unlikely(IS_ERR(kaddrs)))
>  		return PTR_ERR(kaddrs);
>  
> @@ -1349,7 +1349,7 @@ static int __sctp_setsockopt_connectx(struct sock *sk,
>  	err = __sctp_connect(sk, kaddrs, addrs_size, flags, assoc_id);
>  
>  out_free:
> -	kvfree(kaddrs);
> +	kfree(kaddrs);
>  
>  	return err;
>  }
> -- 
> 2.1.0
> 
> 
Acked-by: Neil Horman <nhorman@tuxdriver.com>

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH net] sctp: use memdup_user instead of vmemdup_user
  2019-03-20  6:49 ` Xin Long
@ 2019-03-20 18:10   ` David Miller
  -1 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2019-03-20 18:10 UTC (permalink / raw)
  To: lucien.xin; +Cc: netdev, linux-sctp, marcelo.leitner, nhorman, syzkaller

From: Xin Long <lucien.xin@gmail.com>
Date: Wed, 20 Mar 2019 14:49:38 +0800

> In sctp_setsockopt_bindx()/__sctp_setsockopt_connectx(), it allocates
> memory with addrs_size which is passed from userspace. We used flag
> GFP_USER to put some more restrictions on it in Commit cacc06215271
> ("sctp: use GFP_USER for user-controlled kmalloc").
> 
> However, since Commit c981f254cc82 ("sctp: use vmemdup_user() rather
> than badly open-coding memdup_user()"), vmemdup_user() has been used,
> which doesn't check GFP_USER flag when goes to vmalloc_*(). So when
> addrs_size is a huge value, it could exhaust memory and even trigger
> oom killer.
> 
> This patch is to use memdup_user() instead, in which GFP_USER would
> work to limit the memory allocation with a huge addrs_size.
> 
> Note we can't fix it by limiting 'addrs_size', as there's no demand
> for it from RFC.
> 
> Reported-by: syzbot+ec1b7575afef85a0e5ca@syzkaller.appspotmail.com
> Fixes: c981f254cc82 ("sctp: use vmemdup_user() rather than badly open-coding memdup_user()")
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

Applied and queued up for -stable, thanks Xin.

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH net] sctp: use memdup_user instead of vmemdup_user
@ 2019-03-20 18:10   ` David Miller
  0 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2019-03-20 18:10 UTC (permalink / raw)
  To: lucien.xin; +Cc: netdev, linux-sctp, marcelo.leitner, nhorman, syzkaller

From: Xin Long <lucien.xin@gmail.com>
Date: Wed, 20 Mar 2019 14:49:38 +0800

> In sctp_setsockopt_bindx()/__sctp_setsockopt_connectx(), it allocates
> memory with addrs_size which is passed from userspace. We used flag
> GFP_USER to put some more restrictions on it in Commit cacc06215271
> ("sctp: use GFP_USER for user-controlled kmalloc").
> 
> However, since Commit c981f254cc82 ("sctp: use vmemdup_user() rather
> than badly open-coding memdup_user()"), vmemdup_user() has been used,
> which doesn't check GFP_USER flag when goes to vmalloc_*(). So when
> addrs_size is a huge value, it could exhaust memory and even trigger
> oom killer.
> 
> This patch is to use memdup_user() instead, in which GFP_USER would
> work to limit the memory allocation with a huge addrs_size.
> 
> Note we can't fix it by limiting 'addrs_size', as there's no demand
> for it from RFC.
> 
> Reported-by: syzbot+ec1b7575afef85a0e5ca@syzkaller.appspotmail.com
> Fixes: c981f254cc82 ("sctp: use vmemdup_user() rather than badly open-coding memdup_user()")
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

Applied and queued up for -stable, thanks Xin.

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-03-20 18:10 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-20  6:49 [PATCH net] sctp: use memdup_user instead of vmemdup_user Xin Long
2019-03-20  6:49 ` Xin Long
2019-03-20 11:24 ` Neil Horman
2019-03-20 11:24   ` Neil Horman
2019-03-20 18:10 ` David Miller
2019-03-20 18:10   ` 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.