netdev - Re: [PATCH net 1/5] ppp: lock ppp structure before modifying mru in ppp

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20160224.153202.880408949435562278.davem@davemloft.net>
Date:	Wed, 24 Feb 2016 15:32:02 -0500 (EST)
From:	David Miller <davem@...emloft.net>
To:	g.nault@...halink.fr
Cc:	netdev@...r.kernel.org, paulus@...ba.org
Subject: Re: [PATCH net 1/5] ppp: lock ppp structure before modifying mru
 in ppp_ioctl()

From: Guillaume Nault <g.nault@...halink.fr>
Date: Mon, 22 Feb 2016 20:47:13 +0100

> PPP's Tx and Rx paths read ppp->mru under protection of ppp_xmit_lock()
> and ppp_recv_lock() respectively.
> Therefore ppp_ioctl() must hold the xmit and recv locks before
> concurrently updating ppp->mru.
> 
> Signed-off-by: Guillaume Nault <g.nault@...halink.fr>
 ...
> diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> index fc8ad00..4d342ae 100644
> --- a/drivers/net/ppp/ppp_generic.c
> +++ b/drivers/net/ppp/ppp_generic.c
> @@ -654,7 +654,10 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  	case PPPIOCSMRU:
>  		if (get_user(val, p))
>  			break;
> +		ppp_lock(ppp);
>  		ppp->mru = val;
> +		ppp_unlock(ppp);
> +

I see no bug here at all.

The store here is atomic, and all of those mentioned code paths only
read the MRU once and then use that value for the duration of the
rest of the processing of that PPP frame.

No possible corruptions or misbehavior can occur and I therefore think
the lack of locking here is completely legitimate.

You absolutely must demonstrate a case of corruption or misbehavior
when you want to add supposedly "missing locking".  Otherwise I'll have
a hard time accepting your changes.  This is especially for a subsystem
that as been around as long as PPP.