All of lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH] sunrpc: safely reallow resvport min/max inversion
       [not found] ` <20180917152834.GD18600@fieldses.org>
@ 2018-09-17 15:33   ` J. Bruce Fields
  2018-10-18 19:24     ` J. Bruce Fields
  0 siblings, 1 reply; 3+ messages in thread
From: J. Bruce Fields @ 2018-09-17 15:33 UTC (permalink / raw)
  To: Trond Myklebust, Anna Schumaker
  Cc: Frank Sorenson, steved, linux-nfs, David Howells

From: "J. Bruce Fields" <bfields@redhat.com>

Commits ffb6ca33b04b and e08ea3a96fc7 prevent setting xprt_min_resvport
greater than xprt_max_resvport, but may also break simple code that sets
one parameter then the other, if the new range does not overlap the old.

Also it looks racy to me, unless there's some serialization I'm not
seeing.  Granted it would probably require malicious privileged processes
(unless there's a chance these might eventually be settable in unprivileged
containers), but still it seems better not to let userspace panic the
kernel.

Simpler seems to be to allow setting the parameters to whatever you want
but interpret xprt_min_resvport > xprt_max_resvport as the empty range.

Untested.

Fixes: ffb6ca33b04b "sunrpc: Prevent resvport min/max inversion..."
Fixes: e08ea3a96fc7 "sunrpc: Prevent rexvport min/max inversion..."
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 net/sunrpc/xprtsock.c | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

Whoops, I sent this before but just noticed that I failed to cc the
list, then that I mispelled the address.  Argh.

diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 9e1c5024aba9..5d59de92b5a1 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -129,7 +129,7 @@ static struct ctl_table xs_tunables_table[] = {
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax,
 		.extra1		= &xprt_min_resvport_limit,
-		.extra2		= &xprt_max_resvport
+		.extra2		= &xprt_max_resvport_limit
 	},
 	{
 		.procname	= "max_resvport",
@@ -137,7 +137,7 @@ static struct ctl_table xs_tunables_table[] = {
 		.maxlen		= sizeof(unsigned int),
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax,
-		.extra1		= &xprt_min_resvport,
+		.extra1		= &xprt_min_resvport_limit,
 		.extra2		= &xprt_max_resvport_limit
 	},
 	{
@@ -1773,11 +1773,17 @@ static void xs_udp_timer(struct rpc_xprt *xprt, struct rpc_task *task)
 	spin_unlock_bh(&xprt->transport_lock);
 }
 
-static unsigned short xs_get_random_port(void)
+static int xs_get_random_port(void)
 {
-	unsigned short range = xprt_max_resvport - xprt_min_resvport + 1;
-	unsigned short rand = (unsigned short) prandom_u32() % range;
-	return rand + xprt_min_resvport;
+	unsigned short min = xprt_min_resvport, max = xprt_max_resvport;
+	unsigned short range;
+	unsigned short rand;
+
+	if (max < min)
+		return -EADDRINUSE;
+	range = max - min + 1;
+	rand = (unsigned short) prandom_u32() % range;
+	return rand + min;
 }
 
 /**
@@ -1833,9 +1839,9 @@ static void xs_set_srcport(struct sock_xprt *transport, struct socket *sock)
 		transport->srcport = xs_sock_getport(sock);
 }
 
-static unsigned short xs_get_srcport(struct sock_xprt *transport)
+static int xs_get_srcport(struct sock_xprt *transport)
 {
-	unsigned short port = transport->srcport;
+	int port = transport->srcport;
 
 	if (port == 0 && transport->xprt.resvport)
 		port = xs_get_random_port();
@@ -1856,7 +1862,7 @@ static int xs_bind(struct sock_xprt *transport, struct socket *sock)
 {
 	struct sockaddr_storage myaddr;
 	int err, nloop = 0;
-	unsigned short port = xs_get_srcport(transport);
+	int port = xs_get_srcport(transport);
 	unsigned short last;
 
 	/*
@@ -1874,8 +1880,8 @@ static int xs_bind(struct sock_xprt *transport, struct socket *sock)
 	 * transport->xprt.resvport == 1) xs_get_srcport above will
 	 * ensure that port is non-zero and we will bind as needed.
 	 */
-	if (port == 0)
-		return 0;
+	if (port <= 0)
+		return port;
 
 	memcpy(&myaddr, &transport->srcaddr, transport->xprt.addrlen);
 	do {
@@ -3317,12 +3323,8 @@ static int param_set_uint_minmax(const char *val,
 
 static int param_set_portnr(const char *val, const struct kernel_param *kp)
 {
-	if (kp->arg == &xprt_min_resvport)
-		return param_set_uint_minmax(val, kp,
-			RPC_MIN_RESVPORT,
-			xprt_max_resvport);
 	return param_set_uint_minmax(val, kp,
-			xprt_min_resvport,
+			RPC_MIN_RESVPORT,
 			RPC_MAX_RESVPORT);
 }
 
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH] sunrpc: safely reallow resvport min/max inversion
  2018-09-17 15:33   ` [PATCH] sunrpc: safely reallow resvport min/max inversion J. Bruce Fields
@ 2018-10-18 19:24     ` J. Bruce Fields
  2018-10-18 19:27       ` J. Bruce Fields
  0 siblings, 1 reply; 3+ messages in thread
From: J. Bruce Fields @ 2018-10-18 19:24 UTC (permalink / raw)
  To: Trond Myklebust, Anna Schumaker
  Cc: Frank Sorenson, steved, linux-nfs, David Howells

I didn't get any feedback on this patch.  Maybe the problem was this:

On Mon, Sep 17, 2018 at 11:33:06AM -0400, J. Bruce Fields wrote:
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> Commits ffb6ca33b04b and e08ea3a96fc7 prevent setting xprt_min_resvport
> greater than xprt_max_resvport, but may also break simple code that sets
> one parameter then the other, if the new range does not overlap the old.
> 
> Also it looks racy to me, unless there's some serialization I'm not
> seeing.  Granted it would probably require malicious privileged processes
> (unless there's a chance these might eventually be settable in unprivileged
> containers), but still it seems better not to let userspace panic the
> kernel.
> 
> Simpler seems to be to allow setting the parameters to whatever you want
> but interpret xprt_min_resvport > xprt_max_resvport as the empty range.
> 
> Untested.

So, I've booted to it and verified that I can set min_resvport and
max_resvport, and that mount fails when min_resvport > max_resvport.

I'll resend.

--b.

> 
> Fixes: ffb6ca33b04b "sunrpc: Prevent resvport min/max inversion..."
> Fixes: e08ea3a96fc7 "sunrpc: Prevent rexvport min/max inversion..."
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> ---
>  net/sunrpc/xprtsock.c | 34 ++++++++++++++++++----------------
>  1 file changed, 18 insertions(+), 16 deletions(-)
> 
> Whoops, I sent this before but just noticed that I failed to cc the
> list, then that I mispelled the address.  Argh.
> 
> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> index 9e1c5024aba9..5d59de92b5a1 100644
> --- a/net/sunrpc/xprtsock.c
> +++ b/net/sunrpc/xprtsock.c
> @@ -129,7 +129,7 @@ static struct ctl_table xs_tunables_table[] = {
>  		.mode		= 0644,
>  		.proc_handler	= proc_dointvec_minmax,
>  		.extra1		= &xprt_min_resvport_limit,
> -		.extra2		= &xprt_max_resvport
> +		.extra2		= &xprt_max_resvport_limit
>  	},
>  	{
>  		.procname	= "max_resvport",
> @@ -137,7 +137,7 @@ static struct ctl_table xs_tunables_table[] = {
>  		.maxlen		= sizeof(unsigned int),
>  		.mode		= 0644,
>  		.proc_handler	= proc_dointvec_minmax,
> -		.extra1		= &xprt_min_resvport,
> +		.extra1		= &xprt_min_resvport_limit,
>  		.extra2		= &xprt_max_resvport_limit
>  	},
>  	{
> @@ -1773,11 +1773,17 @@ static void xs_udp_timer(struct rpc_xprt *xprt, struct rpc_task *task)
>  	spin_unlock_bh(&xprt->transport_lock);
>  }
>  
> -static unsigned short xs_get_random_port(void)
> +static int xs_get_random_port(void)
>  {
> -	unsigned short range = xprt_max_resvport - xprt_min_resvport + 1;
> -	unsigned short rand = (unsigned short) prandom_u32() % range;
> -	return rand + xprt_min_resvport;
> +	unsigned short min = xprt_min_resvport, max = xprt_max_resvport;
> +	unsigned short range;
> +	unsigned short rand;
> +
> +	if (max < min)
> +		return -EADDRINUSE;
> +	range = max - min + 1;
> +	rand = (unsigned short) prandom_u32() % range;
> +	return rand + min;
>  }
>  
>  /**
> @@ -1833,9 +1839,9 @@ static void xs_set_srcport(struct sock_xprt *transport, struct socket *sock)
>  		transport->srcport = xs_sock_getport(sock);
>  }
>  
> -static unsigned short xs_get_srcport(struct sock_xprt *transport)
> +static int xs_get_srcport(struct sock_xprt *transport)
>  {
> -	unsigned short port = transport->srcport;
> +	int port = transport->srcport;
>  
>  	if (port == 0 && transport->xprt.resvport)
>  		port = xs_get_random_port();
> @@ -1856,7 +1862,7 @@ static int xs_bind(struct sock_xprt *transport, struct socket *sock)
>  {
>  	struct sockaddr_storage myaddr;
>  	int err, nloop = 0;
> -	unsigned short port = xs_get_srcport(transport);
> +	int port = xs_get_srcport(transport);
>  	unsigned short last;
>  
>  	/*
> @@ -1874,8 +1880,8 @@ static int xs_bind(struct sock_xprt *transport, struct socket *sock)
>  	 * transport->xprt.resvport == 1) xs_get_srcport above will
>  	 * ensure that port is non-zero and we will bind as needed.
>  	 */
> -	if (port == 0)
> -		return 0;
> +	if (port <= 0)
> +		return port;
>  
>  	memcpy(&myaddr, &transport->srcaddr, transport->xprt.addrlen);
>  	do {
> @@ -3317,12 +3323,8 @@ static int param_set_uint_minmax(const char *val,
>  
>  static int param_set_portnr(const char *val, const struct kernel_param *kp)
>  {
> -	if (kp->arg == &xprt_min_resvport)
> -		return param_set_uint_minmax(val, kp,
> -			RPC_MIN_RESVPORT,
> -			xprt_max_resvport);
>  	return param_set_uint_minmax(val, kp,
> -			xprt_min_resvport,
> +			RPC_MIN_RESVPORT,
>  			RPC_MAX_RESVPORT);
>  }
>  
> -- 
> 2.17.1
> 

^ permalink raw reply	[flat|nested] 3+ messages in thread
* [PATCH] sunrpc: safely reallow resvport min/max inversion
  2018-10-18 19:24     ` J. Bruce Fields
@ 2018-10-18 19:27       ` J. Bruce Fields
  0 siblings, 0 replies; 3+ messages in thread
From: J. Bruce Fields @ 2018-10-18 19:27 UTC (permalink / raw)
  To: Trond Myklebust, Anna Schumaker
  Cc: Frank Sorenson, steved, linux-nfs, David Howells

From: "J. Bruce Fields" <bfields@redhat.com>

Commits ffb6ca33b04b and e08ea3a96fc7 prevent setting xprt_min_resvport
greater than xprt_max_resvport, but may also break simple code that sets
one parameter then the other, if the new range does not overlap the old.

Also it looks racy to me, unless there's some serialization I'm not
seeing.  Granted it would probably require malicious privileged processes
(unless there's a chance these might eventually be settable in unprivileged
containers), but still it seems better not to let userspace panic the
kernel.

Simpler seems to be to allow setting the parameters to whatever you want
but interpret xprt_min_resvport > xprt_max_resvport as the empty range.

Fixes: ffb6ca33b04b "sunrpc: Prevent resvport min/max inversion..."
Fixes: e08ea3a96fc7 "sunrpc: Prevent rexvport min/max inversion..."
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 net/sunrpc/xprtsock.c | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 6b7539c0466e..58219d7de91f 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -129,7 +129,7 @@ static struct ctl_table xs_tunables_table[] = {
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax,
 		.extra1		= &xprt_min_resvport_limit,
-		.extra2		= &xprt_max_resvport
+		.extra2		= &xprt_max_resvport_limit
 	},
 	{
 		.procname	= "max_resvport",
@@ -137,7 +137,7 @@ static struct ctl_table xs_tunables_table[] = {
 		.maxlen		= sizeof(unsigned int),
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax,
-		.extra1		= &xprt_min_resvport,
+		.extra1		= &xprt_min_resvport_limit,
 		.extra2		= &xprt_max_resvport_limit
 	},
 	{
@@ -1773,11 +1773,17 @@ static void xs_udp_timer(struct rpc_xprt *xprt, struct rpc_task *task)
 	spin_unlock_bh(&xprt->transport_lock);
 }
 
-static unsigned short xs_get_random_port(void)
+static int xs_get_random_port(void)
 {
-	unsigned short range = xprt_max_resvport - xprt_min_resvport + 1;
-	unsigned short rand = (unsigned short) prandom_u32() % range;
-	return rand + xprt_min_resvport;
+	unsigned short min = xprt_min_resvport, max = xprt_max_resvport;
+	unsigned short range;
+	unsigned short rand;
+
+	if (max < min)
+		return -EADDRINUSE;
+	range = max - min + 1;
+	rand = (unsigned short) prandom_u32() % range;
+	return rand + min;
 }
 
 /**
@@ -1833,9 +1839,9 @@ static void xs_set_srcport(struct sock_xprt *transport, struct socket *sock)
 		transport->srcport = xs_sock_getport(sock);
 }
 
-static unsigned short xs_get_srcport(struct sock_xprt *transport)
+static int xs_get_srcport(struct sock_xprt *transport)
 {
-	unsigned short port = transport->srcport;
+	int port = transport->srcport;
 
 	if (port == 0 && transport->xprt.resvport)
 		port = xs_get_random_port();
@@ -1856,7 +1862,7 @@ static int xs_bind(struct sock_xprt *transport, struct socket *sock)
 {
 	struct sockaddr_storage myaddr;
 	int err, nloop = 0;
-	unsigned short port = xs_get_srcport(transport);
+	int port = xs_get_srcport(transport);
 	unsigned short last;
 
 	/*
@@ -1874,8 +1880,8 @@ static int xs_bind(struct sock_xprt *transport, struct socket *sock)
 	 * transport->xprt.resvport == 1) xs_get_srcport above will
 	 * ensure that port is non-zero and we will bind as needed.
 	 */
-	if (port == 0)
-		return 0;
+	if (port <= 0)
+		return port;
 
 	memcpy(&myaddr, &transport->srcaddr, transport->xprt.addrlen);
 	do {
@@ -3317,12 +3323,8 @@ static int param_set_uint_minmax(const char *val,
 
 static int param_set_portnr(const char *val, const struct kernel_param *kp)
 {
-	if (kp->arg == &xprt_min_resvport)
-		return param_set_uint_minmax(val, kp,
-			RPC_MIN_RESVPORT,
-			xprt_max_resvport);
 	return param_set_uint_minmax(val, kp,
-			xprt_min_resvport,
+			RPC_MIN_RESVPORT,
 			RPC_MAX_RESVPORT);
 }
 
-- 
2.17.2

^ permalink raw reply related	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-10-19  3:29 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20180904190632.GA22395@fieldses.org>
     [not found] ` <20180917152834.GD18600@fieldses.org>
2018-09-17 15:33   ` [PATCH] sunrpc: safely reallow resvport min/max inversion J. Bruce Fields
2018-10-18 19:24     ` J. Bruce Fields
2018-10-18 19:27       ` J. Bruce Fields

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.