Re: [PATCH net-next v2 1/7] af_iucv: take socket lock around SO_MSGSIZE getsockopt

[Stanislav Fomichev <sdf.kernel@gmail.com>]

On 05/15, Breno Leitao wrote:
> Mirror the locking used by the SO_MSGLIMIT case directly above: take
> lock_sock() before reading iucv->hs_dev and dereferencing hs_dev->mtu,
> and release it afterwards. This keeps the two adjacent getsockopt arms
> consistent and matches the lock held by iucv_sock_close() when it
> clears hs_dev.
> 
> This is not an exploitable bug. iucv_sock_close() is the only writer
> of iucv->hs_dev and only runs from the protocol release callback,
> which the socket layer invokes after the last file reference drops.
> The getsockopt() syscall holds an fd reference for its entire
> duration via fdget()/fdput(), so iucv_sock_close() cannot run
> concurrently with the SO_MSGSIZE read on the same socket. There is
> no other writer of hs_dev, and the aligned pointer load cannot tear
> on any architecture Linux supports, so the existing code cannot
> observe a NULL dereference or use-after-free in practice.
> 
> The change is purely defensive: making the locking pattern uniform
> across the function avoids surprising the next reader and removes a
> foot-gun should the close path ever grow a new caller that does not
> hold the fd reference.
> 
> Note: For the reason above, it doesn't contain a "Fixes" tag, and is
> aiming at net-next instead of net.
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>  net/iucv/af_iucv.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> index 72dfccd4e3d58..3dd11d7a967c8 100644
> --- a/net/iucv/af_iucv.c
> +++ b/net/iucv/af_iucv.c
> @@ -1566,9 +1566,11 @@ static int iucv_sock_getsockopt(struct socket *sock, int level, int optname,
>  	case SO_MSGSIZE:
>  		if (sk->sk_state == IUCV_OPEN)
>  			return -EBADFD;
> +		lock_sock(sk);
>  		val = (iucv->hs_dev) ? iucv->hs_dev->mtu -
>  				sizeof(struct af_iucv_trans_hdr) - ETH_HLEN :
>  				0x7fffffff;
> +		release_sock(sk);
>  		break;
>  	default:
>  		return -ENOPROTOOPT;
> 

SO_IPRMDATA_MSG also seems to be only reading the value set via setsockopt,
so maybe it's ok to just cover the whole switch with lock/unlock? (will
mirror what setsockopt does)