Re: [PATCH net] pppoe: check sockaddr length in pppoe_connect()

[Kevin Easton <kevin@guarana.org>]

On Mon, Apr 23, 2018 at 04:38:27PM +0200, Guillaume Nault wrote:
> We must validate sockaddr_len, otherwise userspace can pass fewer data
> than we expect and we end up accessing invalid data.
> 
> Fixes: 224cf5ad14c0 ("ppp: Move the PPP drivers")
> Reported-by: syzbot+4f03bdf92fdf9ef5ddab@syzkaller.appspotmail.com
> Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
> ---
>  drivers/net/ppp/pppoe.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
> index 1483bc7b01e1..7df07337d69c 100644
> --- a/drivers/net/ppp/pppoe.c
> +++ b/drivers/net/ppp/pppoe.c
> @@ -620,6 +620,10 @@ static int pppoe_connect(struct socket *sock, struct sockaddr *uservaddr,
>  	lock_sock(sk);
>  
>  	error = -EINVAL;
> +
> +	if (sockaddr_len != sizeof(struct sockaddr_pppox))
> +		goto end;
> +
>  	if (sp->sa_protocol != PX_PROTO_OE)
>  		goto end;

There's another bug here - pppoe_connect() should also be validating
sp->sa_family.  My suggested patch was going to be:

diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
index 1483bc7..90eb3fd 100644
--- a/drivers/net/ppp/pppoe.c
+++ b/drivers/net/ppp/pppoe.c
@@ -620,6 +620,14 @@ static int pppoe_connect(struct socket *sock, struct sockaddr *uservaddr,
        lock_sock(sk);
 
        error = -EINVAL;
+       if (sockaddr_len < sizeof(struct sockaddr_pppox))
+               goto end;
+
+       error = -EAFNOSUPPORT;
+       if (sp->sa_family != AF_PPPOX)
+               goto end;
+
+       error = -EINVAL;
        if (sp->sa_protocol != PX_PROTO_OE)
                goto end;
 
Should I rework this on top of net.git HEAD?

(The same applies to pppol2tp_connect()).

    - Kevin