Re: [PATCH] ifconfig: keep track of allowed ips pointer correctly

2020-11-04 Thread Klemens Nanni
On Tue, Oct 27, 2020 at 06:16:08PM +0100, Jason A. Donenfeld wrote:
> Somebody on IRC mentioned that using ifconfig to set wgallowedips wasn't
> working on macppc. I don't have a macppc to test this on, but it seems
> like the code is assuming that the two values printed out by this test
> program must always be the same:
> 
>   struct s {
>   int i;
>   };
> 
>   struct p {
>   long l;
>   char c;
>   struct s a[];
>   };
> 
>   int main(int argc, char *argv[])
>   {
>   printf("%zu %zu\n", sizeof(struct p), (size_t)&((struct p *)0)->a[0]);
>   return 0;
>   }
> 
> But actually, on my amd64 system, that little test prints out "16 12".
> This patch fixes up ifconfig.c to do the right thing, so that it
> corresponds with how the kernel handles iteration.
> 
> I don't have a macppc in order to test this, but it works on amd64.
Using the wg(4) EXAMPLES section as test, this fixes wg(4) on macppc.
amd64 continues to work for me as well.

FWIW, both platforms produced the same ifconfig.o regardless of using a
void pointer or char pointer cast as guenther suggested.  I don't know
if `char *' might be beneficial on other platform/compiler combinations.

OK?
Otherwise I'll commit this on friday unless I hear objections or further
feedback.

Jason's diff (with `char *' as per guenther) reattached below.


Index: ifconfig.c
===
RCS file: /cvs/src/sbin/ifconfig/ifconfig.c,v
retrieving revision 1.429
diff -u -p -r1.429 ifconfig.c
--- ifconfig.c  7 Oct 2020 14:38:54 -   1.429
+++ ifconfig.c  4 Nov 2020 19:35:20 -
@@ -5696,11 +5696,10 @@ ensurewginterface(void)
err(1, "calloc");
 }
 
-void *
+void
 growwgdata(size_t by)
 {
ptrdiff_t peer_offset, aip_offset;
-   void *ret;
 
if (wg_interface == NULL)
wgdata.wgd_size = sizeof(*wg_interface);
@@ -5721,16 +5720,18 @@ growwgdata(size_t by)
if (wg_aip != NULL)
wg_aip = (void *)wg_interface + aip_offset;
 
-   ret = (void *)wg_interface + wgdata.wgd_size - by;
-   bzero(ret, by);
-
-   return ret;
+   bzero((char *)wg_interface + wgdata.wgd_size - by, by);
 }
 
 void
 setwgpeer(const char *peerkey_b64, int param)
 {
-   wg_peer = growwgdata(sizeof(*wg_peer));
+   growwgdata(sizeof(*wg_peer));
+   if (wg_aip)
+   wg_peer = (struct wg_peer_io *)wg_aip;
+   else
+   wg_peer = _interface->i_peers[0];
+   wg_aip = _peer->p_aips[0];
wg_peer->p_flags |= WG_PEER_HAS_PUBLIC;
WG_LOAD_KEY(wg_peer->p_public, peerkey_b64, "wgpeer");
wg_interface->i_peers_count++;
@@ -5743,7 +5744,7 @@ setwgpeeraip(const char *aip, int param)
if (wg_peer == NULL)
errx(1, "wgaip: wgpeer not set");
 
-   wg_aip = growwgdata(sizeof(*wg_aip));
+   growwgdata(sizeof(*wg_aip));
 
if ((res = inet_net_pton(AF_INET, aip, _aip->a_ipv4,
sizeof(wg_aip->a_ipv4))) != -1) {
@@ -5759,6 +5760,8 @@ setwgpeeraip(const char *aip, int param)
 
wg_peer->p_flags |= WG_PEER_REPLACE_AIPS;
wg_peer->p_aips_count++;
+
+   wg_aip++;
 }
 
 void



Re: [PATCH] ifconfig: keep track of allowed ips pointer correctly

2020-10-30 Thread wictory
Hi!

On Tue, Oct 27, 2020 at 06:16:08PM +0100, Jason A. Donenfeld wrote:
> Somebody on IRC mentioned that using ifconfig to set wgallowedips wasn't
> working on macppc.

It isn't working on armv7 either. I posted on bugs@ earlier, but it seems
the problem I found might be the same as is being discussed here.

So, on "vanilla" 6.8-RELEASE I get the following execution:

+ ifconfig wg0 create
+ ifconfig wg0 wgkey eEcguPq8JcnbS5CwTP1WsGcKL8/aSD3eD+CSDwy3sLA=
+ ifconfig wg0 wgpeer s+zREKf1nsr9TV8Fbogv4c9Xkn0c305KF+6hOLx81Ho= wgaip 
192.168.123.1/24
ifconfig: SIOCSWG: Address family not supported by protocol family
+ ifconfig wg0 
wg0: flags=8082 mtu 1420
index 46 priority 0 llprio 3
wgpubkey g/V4mqfXffmIEm3ME7RKKvYE3As53mIfsRbYCNY9yTM=
wgpeer s+zREKf1nsr9TV8Fbogv4c9Xkn0c305KF+6hOLx81Ho=
tx: 0, rx: 0
groups: wg
+ ifconfig wg0 wgpeer s+zREKf1nsr9TV8Fbogv4c9Xkn0c305KF+6hOLx81Ho= wgaip 
fd2b:a33a:77b0:450d::/64
ifconfig: SIOCSWG: Address family not supported by protocol family
+ ifconfig wg0
wg0: flags=8082 mtu 1420
index 46 priority 0 llprio 3
wgpubkey g/V4mqfXffmIEm3ME7RKKvYE3As53mIfsRbYCNY9yTM=
wgpeer s+zREKf1nsr9TV8Fbogv4c9Xkn0c305KF+6hOLx81Ho=
tx: 0, rx: 0
groups: wg

When I apply the patch you posted here, I get the following execution:

+ /usr/src/sbin/ifconfig/ifconfig wg0 create
 
+ /usr/src/sbin/ifconfig/ifconfig wg0 wgkey 
wIcMGMrjswYhhVi4SQfLzMuhmPArJuY0avwKyEr1QF8= 
+ /usr/src/sbin/ifconfig/ifconfig wg0 wgpeer 
WDck6FQW6XBnSbJoYe6P6aRj3W2w37x2NQrFa+1zz5Y= wgaip 192.168.123.1/24 

  
+ /usr/src/sbin/ifconfig/ifconfig wg0   
 
wg0: flags=8082 mtu 1420 
 
index 50 priority 0 llprio 3
 
wgpubkey aYVRr9XrQ09yWzKEHEShPq1472QL+A9pckPJv2HgOEA=
wgpeer WDck6FQW6XBnSbJoYe6P6aRj3W2w37x2NQrFa+1zz5Y= 
 
tx: 0, rx: 0
 
wgaip /24   
 
groups: wg
+ /usr/src/sbin/ifconfig/ifconfig wg0 wgpeer 
WDck6FQW6XBnSbJoYe6P6aRj3W2w37x2NQrFa+1zz5Y= wgaip fd2b:a33a:77b0:450d::/64 

  
+ /usr/src/sbin/ifconfig/ifconfig wg0   
 
wg0: flags=8082 mtu 1420
index 50 priority 0 llprio 3
wgpubkey aYVRr9XrQ09yWzKEHEShPq1472QL+A9pckPJv2HgOEA=
wgpeer WDck6FQW6XBnSbJoYe6P6aRj3W2w37x2NQrFa+1zz5Y=
tx: 0, rx: 0
wgaip /64
groups: wg

so, the patch "improves" the situation in the sense that I don't get the
error message, but I still have the problem that wgallowedips are not set,
at least properly. The output fails like that since inet_ntop(3) fails with
EAFNOSUPPORT.

> I don't have a macppc to test this on, but it seems like the code is
> assuming that the two values printed out by this test program must always
> be the same:
> 
>   struct s {
>   int i;
>   };
> 
>   struct p {
>   long l;
>   char c;
>   struct s a[];
>   };
> 
>   int main(int argc, char *argv[])
>   {
>   printf("%zu %zu\n", sizeof(struct p), (size_t)&((struct p *)0)->a[0]);
>   return 0;
>   }
> 
> But actually, on my amd64 system, that little test prints out "16 12".
> This patch fixes up ifconfig.c to do the right thing, so that it
> corresponds with how the kernel handles iteration.
> 
> I don't have a macppc in order to test this, but it works on amd64.

The good news is that it also doesn't work in armv7 :P but I hope my report
helps somehow.

> ---
>  sbin/ifconfig/ifconfig.c | 19 +++
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git sbin/ifconfig/ifconfig.c sbin/ifconfig/ifconfig.c
> index 2ccbac6f4ce..ade6e4943b7 100644
> --- sbin/ifconfig/ifconfig.c
> +++ sbin/ifconfig/ifconfig.c
> @@ -5696,11 +5696,10 @@ ensurewginterface(void)
>   err(1, "calloc");
>  }
>  
> -void *
> +void
>  growwgdata(size_t by)
>  {
>   ptrdiff_t peer_offset, aip_offset;
> - void *ret;
>  
>   if (wg_interface == NULL)
>   wgdata.wgd_size = sizeof(*wg_interface);
> @@ -5721,16 +5720,18 @@ growwgdata(size_t by)
>   if (wg_aip != NULL)
>   wg_aip = (void *)wg_interface + aip_offset;
>  
> -  

Re: [PATCH] ifconfig: keep track of allowed ips pointer correctly

2020-10-28 Thread Klemens Nanni
On Tue, Oct 27, 2020 at 06:16:08PM +0100, Jason A. Donenfeld wrote:
> I don't have a macppc in order to test this, but it works on amd64.
Using the EXAMPLES code from wg(4) as is, this makes things work on
macppc.



Re: [PATCH] ifconfig: keep track of allowed ips pointer correctly

2020-10-27 Thread Philip Guenther
On Tue, Oct 27, 2020 at 9:18 AM Jason A. Donenfeld  wrote:
...

> @@ -5721,16 +5720,18 @@ growwgdata(size_t by)
> if (wg_aip != NULL)
> wg_aip = (void *)wg_interface + aip_offset;
>
> -   ret = (void *)wg_interface + wgdata.wgd_size - by;
> -   bzero(ret, by);
> -
> -   return ret;
> +   bzero((void *)wg_interface + wgdata.wgd_size - by, by);
>  }
>

Total side note, but this caught my eye as relying on the gcc extension to
do pointer arithmetic on "void *" pointers.  At least historically we've
avoided relying on that, which is easy in this case by just casting to
"char *" instead.

Philip Guenther


[PATCH] ifconfig: keep track of allowed ips pointer correctly

2020-10-27 Thread Jason A. Donenfeld
Somebody on IRC mentioned that using ifconfig to set wgallowedips wasn't
working on macppc. I don't have a macppc to test this on, but it seems
like the code is assuming that the two values printed out by this test
program must always be the same:

  struct s {
  int i;
  };

  struct p {
  long l;
  char c;
  struct s a[];
  };

  int main(int argc, char *argv[])
  {
  printf("%zu %zu\n", sizeof(struct p), (size_t)&((struct p *)0)->a[0]);
  return 0;
  }

But actually, on my amd64 system, that little test prints out "16 12".
This patch fixes up ifconfig.c to do the right thing, so that it
corresponds with how the kernel handles iteration.

I don't have a macppc in order to test this, but it works on amd64.
---
 sbin/ifconfig/ifconfig.c | 19 +++
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git sbin/ifconfig/ifconfig.c sbin/ifconfig/ifconfig.c
index 2ccbac6f4ce..ade6e4943b7 100644
--- sbin/ifconfig/ifconfig.c
+++ sbin/ifconfig/ifconfig.c
@@ -5696,11 +5696,10 @@ ensurewginterface(void)
err(1, "calloc");
 }
 
-void *
+void
 growwgdata(size_t by)
 {
ptrdiff_t peer_offset, aip_offset;
-   void *ret;
 
if (wg_interface == NULL)
wgdata.wgd_size = sizeof(*wg_interface);
@@ -5721,16 +5720,18 @@ growwgdata(size_t by)
if (wg_aip != NULL)
wg_aip = (void *)wg_interface + aip_offset;
 
-   ret = (void *)wg_interface + wgdata.wgd_size - by;
-   bzero(ret, by);
-
-   return ret;
+   bzero((void *)wg_interface + wgdata.wgd_size - by, by);
 }
 
 void
 setwgpeer(const char *peerkey_b64, int param)
 {
-   wg_peer = growwgdata(sizeof(*wg_peer));
+   growwgdata(sizeof(*wg_peer));
+   if (wg_aip)
+   wg_peer = (struct wg_peer_io *)wg_aip;
+   else
+   wg_peer = _interface->i_peers[0];
+   wg_aip = _peer->p_aips[0];
wg_peer->p_flags |= WG_PEER_HAS_PUBLIC;
WG_LOAD_KEY(wg_peer->p_public, peerkey_b64, "wgpeer");
wg_interface->i_peers_count++;
@@ -5743,7 +5744,7 @@ setwgpeeraip(const char *aip, int param)
if (wg_peer == NULL)
errx(1, "wgaip: wgpeer not set");
 
-   wg_aip = growwgdata(sizeof(*wg_aip));
+   growwgdata(sizeof(*wg_aip));
 
if ((res = inet_net_pton(AF_INET, aip, _aip->a_ipv4,
sizeof(wg_aip->a_ipv4))) != -1) {
@@ -5759,6 +5760,8 @@ setwgpeeraip(const char *aip, int param)
 
wg_peer->p_flags |= WG_PEER_REPLACE_AIPS;
wg_peer->p_aips_count++;
+
+   wg_aip++;
 }
 
 void
-- 
2.29.1