netdev - Re: [PATCH v2 net 2/2] af_unix: Don't peek OOB data without MSG

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: <3e4ba1b4-ded8-4dd9-9112-a4fb354e1f55@oracle.com>
Date: Tue, 16 Apr 2024 13:11:09 -0700
From: Rao Shoaib <rao.shoaib@...cle.com>
To: Kuniyuki Iwashima <kuniyu@...zon.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>
Cc: Kuniyuki Iwashima <kuni1840@...il.com>, netdev@...r.kernel.org
Subject: Re: [PATCH v2 net 2/2] af_unix: Don't peek OOB data without MSG_OOB.

The proposed fix is not the correct fix as among other things it does 
not allow going pass the OOB if data is present. TCP allows that.

I have attached a patch that fixes this issue and other issues that I 
encountered in my testing. I compared TCP.

Shoaib

On 4/10/24 10:10, Kuniyuki Iwashima wrote:
> Currently, we can read OOB data without MSG_OOB by using MSG_PEEK
> when OOB data is sitting on the front row, which is apparently
> wrong.
> 
>    >>> from socket import *
>    >>> c1, c2 = socketpair(AF_UNIX, SOCK_STREAM)
>    >>> c1.send(b'a', MSG_OOB)
>    1
>    >>> c2.recv(1, MSG_PEEK | MSG_DONTWAIT)
>    b'a'
> 
> If manage_oob() is called when no data has been copied, we only
> check if the socket enables SO_OOBINLINE or MSG_PEEK is not used.
> Otherwise, the skb is returned as is.
> 
> However, here we should return NULL if MSG_PEEK is set and no data
> has been copied.
> 
> Also, in such a case, we should not jump to the redo label because
> we will be caught in the loop and hog the CPU until normal data
> comes in.
> 
> Then, we need to handle skb == NULL case with the if-clause below
> the manage_oob() block.
> 
> With this patch:
> 
>    >>> from socket import *
>    >>> c1, c2 = socketpair(AF_UNIX, SOCK_STREAM)
>    >>> c1.send(b'a', MSG_OOB)
>    1
>    >>> c2.recv(1, MSG_PEEK | MSG_DONTWAIT)
>    Traceback (most recent call last):
>      File "<stdin>", line 1, in <module>
>    BlockingIOError: [Errno 11] Resource temporarily unavailable
> 
> Fixes: 314001f0bf92 ("af_unix: Add OOB support")
> Signed-off-by: Kuniyuki Iwashima <kuniyu@...zon.com>
> ---
>   net/unix/af_unix.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index f297320438bf..9a6ad5974dff 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -2663,7 +2663,9 @@ static struct sk_buff *manage_oob(struct sk_buff *skb, struct sock *sk,
>   					WRITE_ONCE(u->oob_skb, NULL);
>   					consume_skb(skb);
>   				}
> -			} else if (!(flags & MSG_PEEK)) {
> +			} else if (flags & MSG_PEEK) {
> +				skb = NULL;
> +			} else {
>   				skb_unlink(skb, &sk->sk_receive_queue);
>   				WRITE_ONCE(u->oob_skb, NULL);
>   				if (!WARN_ON_ONCE(skb_unref(skb)))
> @@ -2745,11 +2747,9 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state,
>   #if IS_ENABLED(CONFIG_AF_UNIX_OOB)
>   		if (skb) {
>   			skb = manage_oob(skb, sk, flags, copied);
> -			if (!skb) {
> +			if (!skb && copied) {
>   				unix_state_unlock(sk);
> -				if (copied)
> -					break;
> -				goto redo;
> +				break;
>   			}
>   		}
>   #endif
View attachment "0001-AF_UNIX-Fix-handling-of-OOB-Data.patch" of type "text/x-patch" (2772 bytes)