* [PATCH v1 net] net: Allow larger buffer than peer address for SO_PEERNAME.
@ 2023-08-19 0:55 Kuniyuki Iwashima
2023-08-22 2:11 ` Jakub Kicinski
0 siblings, 1 reply; 5+ messages in thread
From: Kuniyuki Iwashima @ 2023-08-19 0:55 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: Kuniyuki Iwashima, Kuniyuki Iwashima, netdev
When we call getsockopt(SO_PEERNAME), the buffer size must be smaller
than or equal to the length of the peer name.
It works with protocols whose address size is fixed. However, the
restriction does not make sense for socket families with an arbitrary
length address.
For example, we usually do not know the peer name if we get an AF_UNIX
socket by accept(), FD passing, or pidfd_getfd(). Then we get -EINVAL
if we pass sizeof(struct sockaddr_un) to getsockopt(SO_PEERNAME). So,
we need to do binary search to get the exact peer name.
addrlen = sizeof(struct sockaddr_un);
getsockopt(fd, SOL_SOCKET, SO_PEERNAME,
(struct sockaddr *)&addr, &addrlen); <-- -EINVAL
The error handling is to avoid copying garbage after the copied peer
address in the temporal buffer.
Let's update copy size by the peer name size if it is larger.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon•com>
---
net/core/sock.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/core/sock.c b/net/core/sock.c
index c9cffb7acbea..f6ee2998a109 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1829,7 +1829,7 @@ int sk_getsockopt(struct sock *sk, int level, int optname,
if (lv < 0)
return -ENOTCONN;
if (lv < len)
- return -EINVAL;
+ len = lv;
if (copy_to_sockptr(optval, address, len))
return -EFAULT;
goto lenout;
--
2.30.2
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH v1 net] net: Allow larger buffer than peer address for SO_PEERNAME.
2023-08-19 0:55 [PATCH v1 net] net: Allow larger buffer than peer address for SO_PEERNAME Kuniyuki Iwashima
@ 2023-08-22 2:11 ` Jakub Kicinski
2023-08-22 2:40 ` Kuniyuki Iwashima
0 siblings, 1 reply; 5+ messages in thread
From: Jakub Kicinski @ 2023-08-22 2:11 UTC (permalink / raw)
To: Kuniyuki Iwashima
Cc: David S. Miller, Eric Dumazet, Paolo Abeni, Kuniyuki Iwashima,
netdev
On Fri, 18 Aug 2023 17:55:52 -0700 Kuniyuki Iwashima wrote:
> For example, we usually do not know the peer name if we get an AF_UNIX
> socket by accept(), FD passing, or pidfd_getfd(). Then we get -EINVAL
> if we pass sizeof(struct sockaddr_un) to getsockopt(SO_PEERNAME). So,
> we need to do binary search to get the exact peer name.
Sounds annoying indeed, but is it really a fix?
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v1 net] net: Allow larger buffer than peer address for SO_PEERNAME.
2023-08-22 2:11 ` Jakub Kicinski
@ 2023-08-22 2:40 ` Kuniyuki Iwashima
2023-08-22 8:43 ` Paolo Abeni
0 siblings, 1 reply; 5+ messages in thread
From: Kuniyuki Iwashima @ 2023-08-22 2:40 UTC (permalink / raw)
To: kuba; +Cc: davem, edumazet, kuni1840, kuniyu, netdev, pabeni
From: Jakub Kicinski <kuba@kernel•org>
Date: Mon, 21 Aug 2023 19:11:13 -0700
> On Fri, 18 Aug 2023 17:55:52 -0700 Kuniyuki Iwashima wrote:
> > For example, we usually do not know the peer name if we get an AF_UNIX
> > socket by accept(), FD passing, or pidfd_getfd(). Then we get -EINVAL
> > if we pass sizeof(struct sockaddr_un) to getsockopt(SO_PEERNAME). So,
> > we need to do binary search to get the exact peer name.
>
> Sounds annoying indeed, but is it really a fix?
So, is net-next preferable ?
I don't have a strong opinion, but I thought "Before knowing the peer
name, you have to know the length" is a bug in the logic, at least for
AF_UNIX.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v1 net] net: Allow larger buffer than peer address for SO_PEERNAME.
2023-08-22 2:40 ` Kuniyuki Iwashima
@ 2023-08-22 8:43 ` Paolo Abeni
2023-08-22 16:24 ` Kuniyuki Iwashima
0 siblings, 1 reply; 5+ messages in thread
From: Paolo Abeni @ 2023-08-22 8:43 UTC (permalink / raw)
To: Kuniyuki Iwashima, kuba; +Cc: davem, edumazet, kuni1840, netdev
On Mon, 2023-08-21 at 19:40 -0700, Kuniyuki Iwashima wrote:
> From: Jakub Kicinski <kuba@kernel•org>
> Date: Mon, 21 Aug 2023 19:11:13 -0700
> > On Fri, 18 Aug 2023 17:55:52 -0700 Kuniyuki Iwashima wrote:
> > > For example, we usually do not know the peer name if we get an AF_UNIX
> > > socket by accept(), FD passing, or pidfd_getfd(). Then we get -EINVAL
> > > if we pass sizeof(struct sockaddr_un) to getsockopt(SO_PEERNAME). So,
> > > we need to do binary search to get the exact peer name.
> >
> > Sounds annoying indeed, but is it really a fix?
>
> So, is net-next preferable ?
>
> I don't have a strong opinion, but I thought "Before knowing the peer
> name, you have to know the length" is a bug in the logic, at least for
> AF_UNIX.
I'm unsure we can accept this change: AFAICS currently
getsockopt(SO_PEERNAME,... len) never change the user-provided len on
success. Applications could relay on that and avoid re-reading len on
successful completion. With this patch such application could read
uninitialized data and/or ever mis-interpret the peer name len.
If the user-space application want to avoid the binary search, it can
already call getpeername().
Cheers,
Paolo
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v1 net] net: Allow larger buffer than peer address for SO_PEERNAME.
2023-08-22 8:43 ` Paolo Abeni
@ 2023-08-22 16:24 ` Kuniyuki Iwashima
0 siblings, 0 replies; 5+ messages in thread
From: Kuniyuki Iwashima @ 2023-08-22 16:24 UTC (permalink / raw)
To: pabeni; +Cc: davem, edumazet, kuba, kuni1840, kuniyu, netdev
From: Paolo Abeni <pabeni@redhat•com>
Date: Tue, 22 Aug 2023 10:43:06 +0200
> On Mon, 2023-08-21 at 19:40 -0700, Kuniyuki Iwashima wrote:
> > From: Jakub Kicinski <kuba@kernel•org>
> > Date: Mon, 21 Aug 2023 19:11:13 -0700
> > > On Fri, 18 Aug 2023 17:55:52 -0700 Kuniyuki Iwashima wrote:
> > > > For example, we usually do not know the peer name if we get an AF_UNIX
> > > > socket by accept(), FD passing, or pidfd_getfd(). Then we get -EINVAL
> > > > if we pass sizeof(struct sockaddr_un) to getsockopt(SO_PEERNAME). So,
> > > > we need to do binary search to get the exact peer name.
> > >
> > > Sounds annoying indeed, but is it really a fix?
> >
> > So, is net-next preferable ?
> >
> > I don't have a strong opinion, but I thought "Before knowing the peer
> > name, you have to know the length" is a bug in the logic, at least for
> > AF_UNIX.
>
> I'm unsure we can accept this change: AFAICS currently
> getsockopt(SO_PEERNAME,... len) never change the user-provided len on
> success. Applications could relay on that and avoid re-reading len on
> successful completion. With this patch such application could read
> uninitialized data and/or ever mis-interpret the peer name len.
>
> If the user-space application want to avoid the binary search, it can
> already call getpeername().
Ah exactly, getppername() has the same behaviour with this patch
in move_addr_to_user().
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-08-22 16:25 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-08-19 0:55 [PATCH v1 net] net: Allow larger buffer than peer address for SO_PEERNAME Kuniyuki Iwashima
2023-08-22 2:11 ` Jakub Kicinski
2023-08-22 2:40 ` Kuniyuki Iwashima
2023-08-22 8:43 ` Paolo Abeni
2023-08-22 16:24 ` Kuniyuki Iwashima
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox