[PATCH v1 net] net: Allow larger buffer than peer address for SO_PEERNAME.

[PATCH v1 net] net: Allow larger buffer than peer address for SO_PEERNAME.

[Kuniyuki Iwashima]

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

Re: [PATCH v1 net] net: Allow larger buffer than peer address for SO_PEERNAME.

[Jakub Kicinski]

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?

Re: [PATCH v1 net] net: Allow larger buffer than peer address for SO_PEERNAME.

[Kuniyuki Iwashima]

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.

Re: [PATCH v1 net] net: Allow larger buffer than peer address for SO_PEERNAME.

[Paolo Abeni]

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

Re: [PATCH v1 net] net: Allow larger buffer than peer address for SO_PEERNAME.

[Kuniyuki Iwashima]

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().