Re: [PATCH 1/3] selinux: Check address length before reading address family

From: Paul Moore <paul@paul-moore.com>
To: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Cc: linux-security-module@vger.kernel.org, selinux@vger.kernel.org
Subject: Re: [PATCH 1/3] selinux: Check address length before reading address family
Date: Mon, 15 Apr 2019 12:46:51 -0400	[thread overview]
Message-ID: <CAHC9VhSCNOANdB=xQnQ6=ieWWPjUpcynYF_8kDOnaAK321cacQ@mail.gmail.com> (raw)
In-Reply-To: <1555066776-9758-1-git-send-email-penguin-kernel@I-love.SAKURA.ne.jp>

On Fri, Apr 12, 2019 at 7:00 AM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
>
> KMSAN will complain if valid address length passed to bind()/connect() is
> shorter than sizeof("struct sockaddr"->sa_family) bytes.
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
>  security/selinux/hooks.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)

A reminder to please CC the SELinux mailing list on patches such as these.

This looks good to me, and since it's specific to SELinux, I've gone
ahead and merged it into the selinux/next branch.  Thanks for your
help on this.

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index d5fdcb0d26fe..c61787b15f27 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -4512,7 +4512,7 @@ static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in
>                 struct lsm_network_audit net = {0,};
>                 struct sockaddr_in *addr4 = NULL;
>                 struct sockaddr_in6 *addr6 = NULL;
> -               u16 family_sa = address->sa_family;
> +               u16 family_sa;
>                 unsigned short snum;
>                 u32 sid, node_perm;
>
> @@ -4522,6 +4522,9 @@ static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in
>                  * need to check address->sa_family as it is possible to have
>                  * sk->sk_family = PF_INET6 with addr->sa_family = AF_INET.
>                  */
> +               if (addrlen < offsetofend(struct sockaddr, sa_family))
> +                       return -EINVAL;
> +               family_sa = address->sa_family;
>                 switch (family_sa) {
>                 case AF_UNSPEC:
>                 case AF_INET:
> @@ -4654,6 +4657,8 @@ static int selinux_socket_connect_helper(struct socket *sock,
>                  * need to check address->sa_family as it is possible to have
>                  * sk->sk_family = PF_INET6 with addr->sa_family = AF_INET.
>                  */
> +               if (addrlen < offsetofend(struct sockaddr, sa_family))
> +                       return -EINVAL;
>                 switch (address->sa_family) {
>                 case AF_INET:
>                         addr4 = (struct sockaddr_in *)address;
> --
> 2.16.5

-- 
paul moore
www.paul-moore.com