| Message ID | 20230719084415.1378696-1-leitao@debian.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: Use _K_SS_MAXSIZE instead of absolute value | expand |
From: Breno Leitao <leitao@debian.org> Date: Wed, 19 Jul 2023 01:44:12 -0700 > Looking at sk_getsockopt function, it is unclear why 128 is a magical > number. > > Use the proper macro, so it becomes clear to understand what the value > mean, and get a reference where it is coming from (user-exported API). > > Signed-off-by: Breno Leitao <leitao@debian.org> > --- > 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 9370fd50aa2c..58b6f00197d6 100644 > --- a/net/core/sock.c > +++ b/net/core/sock.c > @@ -1815,7 +1815,7 @@ int sk_getsockopt(struct sock *sk, int level, int optname, > > case SO_PEERNAME: > { > - char address[128]; > + char address[_K_SS_MAXSIZE]; I guess you saw a bug caught by the fortified memcpy(), but this doesn't fix it properly. I'll post a series soon that fix the issue and another realted one. Thanks! > > lv = sock->ops->getname(sock, (struct sockaddr *)address, 2); > if (lv < 0) > -- > 2.34.1
On Wed, Jul 19, 2023 at 10:04:45AM -0700, Kuniyuki Iwashima wrote: > From: Breno Leitao <leitao@debian.org> > Date: Wed, 19 Jul 2023 01:44:12 -0700 > > Looking at sk_getsockopt function, it is unclear why 128 is a magical > > number. > > > > Use the proper macro, so it becomes clear to understand what the value > > mean, and get a reference where it is coming from (user-exported API). > > > > Signed-off-by: Breno Leitao <leitao@debian.org> > > --- > > 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 9370fd50aa2c..58b6f00197d6 100644 > > --- a/net/core/sock.c > > +++ b/net/core/sock.c > > @@ -1815,7 +1815,7 @@ int sk_getsockopt(struct sock *sk, int level, int optname, > > > > case SO_PEERNAME: > > { > > - char address[128]; > > + char address[_K_SS_MAXSIZE]; > > I guess you saw a bug caught by the fortified memcpy(), but this > doesn't fix it properly. Not really, in fact. I was reading this code, and I found this discussion a while ago, where I got the idea: https://lore.kernel.org/lkml/20140930.005925.995989898229686123.davem@davemloft.net/
From: Breno Leitao <leitao@debian.org> Date: Wed, 19 Jul 2023 10:18:49 -0700 > On Wed, Jul 19, 2023 at 10:04:45AM -0700, Kuniyuki Iwashima wrote: > > From: Breno Leitao <leitao@debian.org> > > Date: Wed, 19 Jul 2023 01:44:12 -0700 > > > Looking at sk_getsockopt function, it is unclear why 128 is a magical > > > number. > > > > > > Use the proper macro, so it becomes clear to understand what the value > > > mean, and get a reference where it is coming from (user-exported API). > > > > > > Signed-off-by: Breno Leitao <leitao@debian.org> > > > --- > > > 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 9370fd50aa2c..58b6f00197d6 100644 > > > --- a/net/core/sock.c > > > +++ b/net/core/sock.c > > > @@ -1815,7 +1815,7 @@ int sk_getsockopt(struct sock *sk, int level, int optname, > > > > > > case SO_PEERNAME: > > > { > > > - char address[128]; > > > + char address[_K_SS_MAXSIZE]; > > > > I guess you saw a bug caught by the fortified memcpy(), but this > > doesn't fix it properly. > > Not really, in fact. I was reading this code, and I found this > discussion a while ago, where I got the idea: > > https://lore.kernel.org/lkml/20140930.005925.995989898229686123.davem@davemloft.net/ I got it, but I prefer using struct sockaddr_storage as done in other places. $ grep -rn sockaddr_storage net/ Also, there would be some situations where we must cast each family-specific address back to sockaddr_storage for fortified library. Then, it makes more sense to use sockaddr_storage rather than _K_SS_MAXSIZE.
On Wed, Jul 19, 2023 at 10:30:17AM -0700, Kuniyuki Iwashima wrote: > From: Breno Leitao <leitao@debian.org> > Date: Wed, 19 Jul 2023 10:18:49 -0700 > > On Wed, Jul 19, 2023 at 10:04:45AM -0700, Kuniyuki Iwashima wrote: > > > From: Breno Leitao <leitao@debian.org> > > > Date: Wed, 19 Jul 2023 01:44:12 -0700 > > > > Looking at sk_getsockopt function, it is unclear why 128 is a magical > > > > number. > > > > > > > > Use the proper macro, so it becomes clear to understand what the value > > > > mean, and get a reference where it is coming from (user-exported API). > > > > > > > > Signed-off-by: Breno Leitao <leitao@debian.org> > > > > --- > > > > 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 9370fd50aa2c..58b6f00197d6 100644 > > > > --- a/net/core/sock.c > > > > +++ b/net/core/sock.c > > > > @@ -1815,7 +1815,7 @@ int sk_getsockopt(struct sock *sk, int level, int optname, > > > > > > > > case SO_PEERNAME: > > > > { > > > > - char address[128]; > > > > + char address[_K_SS_MAXSIZE]; > > > > > > I guess you saw a bug caught by the fortified memcpy(), but this > > > doesn't fix it properly. > > > > Not really, in fact. I was reading this code, and I found this > > discussion a while ago, where I got the idea: > > > > https://lore.kernel.org/lkml/20140930.005925.995989898229686123.davem@davemloft.net/ > > I got it, but I prefer using struct sockaddr_storage as done in > other places. > > $ grep -rn sockaddr_storage net/ > > Also, there would be some situations where we must cast each > family-specific address back to sockaddr_storage for fortified > library. > > Then, it makes more sense to use sockaddr_storage rather than > _K_SS_MAXSIZE. Agree, that is a better fix. Thanks for working on it!
diff --git a/net/core/sock.c b/net/core/sock.c index 9370fd50aa2c..58b6f00197d6 100644 --- a/net/core/sock.c +++ b/net/core/sock.c @@ -1815,7 +1815,7 @@ int sk_getsockopt(struct sock *sk, int level, int optname, case SO_PEERNAME: { - char address[128]; + char address[_K_SS_MAXSIZE]; lv = sock->ops->getname(sock, (struct sockaddr *)address, 2); if (lv < 0)
Looking at sk_getsockopt function, it is unclear why 128 is a magical number. Use the proper macro, so it becomes clear to understand what the value mean, and get a reference where it is coming from (user-exported API). Signed-off-by: Breno Leitao <leitao@debian.org> --- net/core/sock.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)