[Devel] Re: [PATCH 3/5] Add common socket helpers to unify the security hooks

[Serge E. Hallyn]

Quoting Dan Smith (danms at us.ibm.com):
> This moves the meat out of the bind(), getsockname(), and getpeername() syscalls
> into helper functions that performs security_socket_bind() and then the
> sock->ops->call().  This allows a unification of this behavior between the
> syscalls and the pending socket restart logic.
> 
> Signed-off-by: Dan Smith <danms at us.ibm.com>
> Cc: netdev at vger.kernel.org
> ---
>  include/net/sock.h |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  net/socket.c       |   29 ++++++-----------------------
>  2 files changed, 54 insertions(+), 23 deletions(-)
> 
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 2c0da92..43b9599 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -1572,6 +1572,54 @@ extern void sock_enable_timestamp(struct sock *sk, int flag);
>  extern int sock_get_timestamp(struct sock *, struct timeval __user *);
>  extern int sock_get_timestampns(struct sock *, struct timespec __user *);
> 
> +/* bind() helper shared between any callers needing to perform a bind on
> + * behalf of userspace (syscall and restart) with the security hooks.
> + */
> +static inline int sock_bind(struct socket *sock,
> +			    struct sockaddr *addr,
> +			    int addr_len)
> +{
> +	int err;
> +
> +	err = security_socket_bind(sock, addr, addr_len);
> +	if (err)
> +		return err;
> +	else
> +		return sock->ops->bind(sock, addr, addr_len);
> +}
> +
> +/* getname() helper shared between any callers needing to perform a getname on
> + * behalf of userspace (syscall and restart) with the security hooks.
> + */
> +static inline int sock_getname(struct socket *sock,
> +			       struct sockaddr *addr,
> +			       int *addr_len)
> +{
> +	int err;
> +
> +	err = security_socket_getsockname(sock);
> +	if (err)
> +		return err;
> +	else
> +		return sock->ops->getname(sock, addr, addr_len, 0);

My only concern here is whether 0 should be passed in by the caller?
(Not sure, just wondering)

Otherwise this looks good, thanks.

Acked-by: Serge Hallyn <serue at us.ibm.com>

> +}
> +
> +/* getpeer() helper shared between any callers needing to perform a getpeer on
> + * behalf of userspace (syscall and restart) with the security hooks.
> + */
> +static inline int sock_getpeer(struct socket *sock,
> +			       struct sockaddr *addr,
> +			       int *addr_len)
> +{
> +	int err;
> +
> +	err = security_socket_getpeername(sock);
> +	if (err)
> +		return err;
> +	else
> +		return sock->ops->getname(sock, addr, addr_len, 1);
> +}
> +
>  /* 
>   *	Enable debug/info messages 
>   */
> diff --git a/net/socket.c b/net/socket.c
> index 791d71a..65e7698 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -1414,15 +1414,10 @@ SYSCALL_DEFINE3(bind, int, fd, struct sockaddr __user *, umyaddr, int, addrlen)
>  	sock = sockfd_lookup_light(fd, &err, &fput_needed);
>  	if (sock) {
>  		err = move_addr_to_kernel(umyaddr, addrlen, (struct sockaddr *)&address);
> -		if (err >= 0) {
> -			err = security_socket_bind(sock,
> -						   (struct sockaddr *)&address,
> -						   addrlen);
> -			if (!err)
> -				err = sock->ops->bind(sock,
> -						      (struct sockaddr *)
> -						      &address, addrlen);
> -		}
> +		if (err >= 0)
> +			err = sock_bind(sock,
> +					(struct sockaddr *)&address,
> +					addrlen);
>  		fput_light(sock->file, fput_needed);
>  	}
>  	return err;
> @@ -1610,11 +1605,7 @@ SYSCALL_DEFINE3(getsockname, int, fd, struct sockaddr __user *, usockaddr,
>  	if (!sock)
>  		goto out;
> 
> -	err = security_socket_getsockname(sock);
> -	if (err)
> -		goto out_put;
> -
> -	err = sock->ops->getname(sock, (struct sockaddr *)&address, &len, 0);
> +	err = sock_getname(sock, (struct sockaddr *)&address, &len);
>  	if (err)
>  		goto out_put;
>  	err = move_addr_to_user((struct sockaddr *)&address, len, usockaddr, usockaddr_len);
> @@ -1639,15 +1630,7 @@ SYSCALL_DEFINE3(getpeername, int, fd, struct sockaddr __user *, usockaddr,
> 
>  	sock = sockfd_lookup_light(fd, &err, &fput_needed);
>  	if (sock != NULL) {
> -		err = security_socket_getpeername(sock);
> -		if (err) {
> -			fput_light(sock->file, fput_needed);
> -			return err;
> -		}
> -
> -		err =
> -		    sock->ops->getname(sock, (struct sockaddr *)&address, &len,
> -				       1);
> +		err = sock_getpeer(sock, (struct sockaddr *)&address, &len);
>  		if (!err)
>  			err = move_addr_to_user((struct sockaddr *)&address, len, usockaddr,
>  						usockaddr_len);
> -- 
> 1.6.2.5
> 
> _______________________________________________
> Containers mailing list
> Containers at lists.linux-foundation.org
> https://lists.linux-foundation.org/mailman/listinfo/containers
_______________________________________________
Containers mailing list
Containers at lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers