netdev - Re: [PATCH bpf-next v2] Only run BPF cgroup unix sockaddr recvmsg() hooks on named sockets

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: <eb4bd204-17b1-f0cb-93dd-d74999ddb265@linux.dev>
Date: Mon, 16 Oct 2023 11:33:36 -0700
From: Martin KaFai Lau <martin.lau@...ux.dev>
To: Kuniyuki Iwashima <kuniyu@...zon.com>, daan.j.demeyer@...il.com
Cc: bpf@...r.kernel.org, kernel-team@...a.com, netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next v2] Only run BPF cgroup unix sockaddr recvmsg()
 hooks on named sockets

On 10/12/23 11:11 AM, Kuniyuki Iwashima wrote:
> From: Daan De Meyer <daan.j.demeyer@...il.com>
> Date: Thu, 12 Oct 2023 10:52:13 +0200
>> Changes since v1:
>>
>> * Added missing Signed-off-by tag
> 
> You can put these after --- so that it will disappear when merged.
> 
> 
>>
>> We should not run the recvmsg() hooks on unnamed sockets as we do
>> not run them on unnamed sockets in the other hooks either. We may
>> look into relaxing this later but for now let's make sure we are
>> consistent and not run the hooks on unnamed sockets anywhere.
>>
>> Signed-off-by: Daan De Meyer <daan.j.demeyer@...il.com>
>> ---
>>   net/unix/af_unix.c | 14 ++++++++------
>>   1 file changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
>> index e10d07c76044..81fb8bddaff9 100644
>> --- a/net/unix/af_unix.c
>> +++ b/net/unix/af_unix.c
>> @@ -2416,9 +2416,10 @@ int __unix_dgram_recvmsg(struct sock *sk, struct msghdr *msg, size_t size,
>>   	if (msg->msg_name) {
>>   		unix_copy_addr(msg, skb->sk);
> 
> How is an unnamed socket set to skb->sk ?

I had a similar question. Most likely socketpair? Please add an explanation in 
the commit message in v3. Please also help to add a selftest for this case.

> 
> 
>>
>> -		BPF_CGROUP_RUN_PROG_UNIX_RECVMSG_LOCK(sk,
>> -						      msg->msg_name,
>> -						      &msg->msg_namelen);
>> +		if (msg->msg_namelen > 0)
>> +			BPF_CGROUP_RUN_PROG_UNIX_RECVMSG_LOCK(sk,
>> +							      msg->msg_name,
>> +							      &msg->msg_namelen);
>>   	}
>>
>>   	if (size > skb->len - skip)
>> @@ -2773,9 +2774,10 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state,
>>   					 state->msg->msg_name);
>>   			unix_copy_addr(state->msg, skb->sk);
>>
>> -			BPF_CGROUP_RUN_PROG_UNIX_RECVMSG_LOCK(sk,
>> -							      state->msg->msg_name,
>> -							      &state->msg->msg_namelen);
>> +			if (state->msg->msg_namelen > 0)
>> +				BPF_CGROUP_RUN_PROG_UNIX_RECVMSG_LOCK(sk,
>> +								      state->msg->msg_name,
>> +								      &state->msg->msg_namelen);
>>
>>   			sunaddr = NULL;
>>   		}
>> --
>> 2.41.0
>>