From: Chuck Lever <[email protected]>
The only reason svcsock.c looks at a sockaddr's port is to check whether
the remote peer is connecting from a privileged port. Refactor this check
to hide processing that is specific to address format.
Signed-off-by: Chuck Lever <[email protected]>
Cc: Aurelien Charbon <[email protected]>
Signed-off-by: Neil Brown <[email protected]>
### Diffstat output
./net/sunrpc/svcsock.c | 20 +++++++++++++++++---
1 file changed, 17 insertions(+), 3 deletions(-)
diff .prev/net/sunrpc/svcsock.c ./net/sunrpc/svcsock.c
--- .prev/net/sunrpc/svcsock.c 2006-12-13 10:32:15.000000000 +1100
+++ ./net/sunrpc/svcsock.c 2006-12-13 10:32:17.000000000 +1100
@@ -926,6 +926,20 @@ svc_tcp_data_ready(struct sock *sk, int
wake_up_interruptible(sk->sk_sleep);
}
+static inline int svc_port_is_privileged(struct sockaddr *sin)
+{
+ switch (sin->sa_family) {
+ case AF_INET:
+ return ntohs(((struct sockaddr_in *)sin)->sin_port) < 1024;
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
+ case AF_INET6:
+ return ntohs(((struct sockaddr_in6 *)sin)->sin6_port) < 1024;
+#endif
+ default:
+ return 0;
+ }
+}
+
/*
* Accept a TCP connection
*/
@@ -972,7 +986,7 @@ svc_tcp_accept(struct svc_sock *svsk)
* hosts here, but when we get encryption, the IP of the host won't
* tell us anything. For now just warn about unpriv connections.
*/
- if (ntohs(sin.sin_port) >= 1024) {
+ if (!svc_port_is_privileged((struct sockaddr *) &sin)) {
dprintk(KERN_WARNING
"%s: connect from unprivileged port: %s\n",
serv->sv_name,
@@ -1313,7 +1327,6 @@ int
svc_recv(struct svc_rqst *rqstp, long timeout)
{
struct svc_sock *svsk = NULL;
- struct sockaddr_in *sin = svc_addr_in(rqstp);
struct svc_serv *serv = rqstp->rq_server;
struct svc_pool *pool = rqstp->rq_pool;
int len, i;
@@ -1408,7 +1421,8 @@ svc_recv(struct svc_rqst *rqstp, long ti
svsk->sk_lastrecv = get_seconds();
clear_bit(SK_OLD, &svsk->sk_flags);
- rqstp->rq_secure = ntohs(sin->sin_port) < 1024;
+ rqstp->rq_secure =
+ svc_port_is_privileged(svc_addr(rqstp));
rqstp->rq_chandle.defer = svc_defer;
if (serv->sv_stats)
On Wed, 13 Dec 2006 10:59:27 +1100
NeilBrown <[email protected]> wrote:
> From: Chuck Lever <[email protected]>
> The only reason svcsock.c looks at a sockaddr's port is to check whether
> the remote peer is connecting from a privileged port. Refactor this check
> to hide processing that is specific to address format.
>
> Signed-off-by: Chuck Lever <[email protected]>
> Cc: Aurelien Charbon <[email protected]>
> Signed-off-by: Neil Brown <[email protected]>
>
> ### Diffstat output
> ./net/sunrpc/svcsock.c | 20 +++++++++++++++++---
> 1 file changed, 17 insertions(+), 3 deletions(-)
>
> diff .prev/net/sunrpc/svcsock.c ./net/sunrpc/svcsock.c
> --- .prev/net/sunrpc/svcsock.c 2006-12-13 10:32:15.000000000 +1100
> +++ ./net/sunrpc/svcsock.c 2006-12-13 10:32:17.000000000 +1100
> @@ -926,6 +926,20 @@ svc_tcp_data_ready(struct sock *sk, int
> wake_up_interruptible(sk->sk_sleep);
> }
>
> +static inline int svc_port_is_privileged(struct sockaddr *sin)
> +{
> + switch (sin->sa_family) {
> + case AF_INET:
> + return ntohs(((struct sockaddr_in *)sin)->sin_port) < 1024;
> +#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
> + case AF_INET6:
> + return ntohs(((struct sockaddr_in6 *)sin)->sin6_port) < 1024;
> +#endif
> + default:
> + return 0;
> + }
> +}
I'm a bit surprised to see this test implemented in sunrpc - it's the sort
of thing which core networking should implement?
And should that "1024" be PROT_SOCK?
On 12/12/06, Andrew Morton <[email protected]> wrote:
> On Wed, 13 Dec 2006 10:59:27 +1100
> NeilBrown <[email protected]> wrote:
>
> > From: Chuck Lever <[email protected]>
> > The only reason svcsock.c looks at a sockaddr's port is to check whether
> > the remote peer is connecting from a privileged port. Refactor this check
> > to hide processing that is specific to address format.
> >
> > Signed-off-by: Chuck Lever <[email protected]>
> > Cc: Aurelien Charbon <[email protected]>
> > Signed-off-by: Neil Brown <[email protected]>
> >
> > ### Diffstat output
> > ./net/sunrpc/svcsock.c | 20 +++++++++++++++++---
> > 1 file changed, 17 insertions(+), 3 deletions(-)
> >
> > diff .prev/net/sunrpc/svcsock.c ./net/sunrpc/svcsock.c
> > --- .prev/net/sunrpc/svcsock.c 2006-12-13 10:32:15.000000000 +1100
> > +++ ./net/sunrpc/svcsock.c 2006-12-13 10:32:17.000000000 +1100
> > @@ -926,6 +926,20 @@ svc_tcp_data_ready(struct sock *sk, int
> > wake_up_interruptible(sk->sk_sleep);
> > }
> >
> > +static inline int svc_port_is_privileged(struct sockaddr *sin)
> > +{
> > + switch (sin->sa_family) {
> > + case AF_INET:
> > + return ntohs(((struct sockaddr_in *)sin)->sin_port) < 1024;
> > +#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
> > + case AF_INET6:
> > + return ntohs(((struct sockaddr_in6 *)sin)->sin6_port) < 1024;
> > +#endif
> > + default:
> > + return 0;
> > + }
> > +}
>
> I'm a bit surprised to see this test implemented in sunrpc - it's the sort
> of thing which core networking should implement?
The check is open-coded in each socket type's bind callout, and
includes a capability check which I believe the NFS server doesn't
require.
> And should that "1024" be PROT_SOCK?
All I can say is.... "Doh!" I'll send Neil a replacement with this fixed.
--
"We who cut mere stones must always be envisioning cathedrals"
-- Quarry worker's creed