Re: libc: Wrap __warn_references occurences in #if defined(APIWARN)

2021-10-24 Thread Theo de Raadt
But why?

So I thought APIWARN was to be naunced, so that there are two kinds: API
we always bitch about, and ones we only bitch about if APIWARN is set
(which allows a linker later on to decide if it wants to honour the
.section)

But you are throwing them all behind the #ifdef, which means the warning
section references won't be created except with APIWARN.

Why?  Was this split pointless, or is there a specific goal?

Frederic Cambus  wrote:

> Hi tech@,
> 
> Most (but not all) __warn_references occurences in libc are wrapped in
> #if defined(APIWARN) blocks. This diff wraps the remaining occurences,
> so building libc without the APIWARN definition does not produce any
> strain .gnu.warning.* sections.
> 
> Comments? OK?
> 
> Index: lib/libc/compat-43/getwd.c
> ===
> RCS file: /cvs/src/lib/libc/compat-43/getwd.c,v
> retrieving revision 1.12
> diff -u -p -r1.12 getwd.c
> --- lib/libc/compat-43/getwd.c28 Nov 2017 06:55:49 -  1.12
> +++ lib/libc/compat-43/getwd.c24 Oct 2021 22:07:40 -
> @@ -45,5 +45,7 @@ getwd(char *buf)
>   return(NULL);
>  }
>  
> +#if defined(APIWARN)
>  __warn_references(getwd,
>  "getwd() possibly used unsafely; consider using getcwd()");
> +#endif
> Index: lib/libc/stdio/mktemp.c
> ===
> RCS file: /cvs/src/lib/libc/stdio/mktemp.c,v
> retrieving revision 1.39
> diff -u -p -r1.39 mktemp.c
> --- lib/libc/stdio/mktemp.c   28 Nov 2017 06:55:49 -  1.39
> +++ lib/libc/stdio/mktemp.c   24 Oct 2021 22:07:41 -
> @@ -118,8 +118,10 @@ _mktemp(char *path)
>   return(path);
>  }
>  
> +#if defined(APIWARN)
>  __warn_references(mktemp,
>  "mktemp() possibly used unsafely; consider using mkstemp()");
> +#endif
>  
>  char *
>  mktemp(char *path)
> Index: lib/libc/stdio/tempnam.c
> ===
> RCS file: /cvs/src/lib/libc/stdio/tempnam.c,v
> retrieving revision 1.20
> diff -u -p -r1.20 tempnam.c
> --- lib/libc/stdio/tempnam.c  28 Nov 2017 06:55:49 -  1.20
> +++ lib/libc/stdio/tempnam.c  24 Oct 2021 22:07:41 -
> @@ -36,8 +36,10 @@
>  #include 
>  #include 
>  
> +#if defined(APIWARN)
>  __warn_references(tempnam,
>  "tempnam() possibly used unsafely; consider using mkstemp()");
> +#endif
>  
>  char *
>  tempnam(const char *dir, const char *pfx)
> Index: lib/libc/stdio/tmpnam.c
> ===
> RCS file: /cvs/src/lib/libc/stdio/tmpnam.c,v
> retrieving revision 1.12
> diff -u -p -r1.12 tmpnam.c
> --- lib/libc/stdio/tmpnam.c   28 Nov 2017 06:55:49 -  1.12
> +++ lib/libc/stdio/tmpnam.c   24 Oct 2021 22:07:41 -
> @@ -36,8 +36,10 @@
>  #include 
>  #include 
>  
> +#if defined(APIWARN)
>  __warn_references(tmpnam,
>  "tmpnam() possibly used unsafely; consider using mkstemp()");
> +#endif
>  
>  char *
>  tmpnam(char *s)
> 



Faster unhibernate by skipping devices

2021-10-24 Thread Theo de Raadt
At the last hackathon I observed high elapsed time when the unhibernate
bsd.booted kernel attaches unneccessary drivers, and then detatches a
vast number of them when suspending to bounce to the old image.

This diff skips attaching those devices.  The current list is all tape
and network devices (based upon DV_TAPE and DV_NET), plus vmm, azalia,
and usb sub-devices (based upon a new CD_SKIPHIBERNATE flag).  The usb
sub-devices in particular make a big difference.  Adding additional
devices to the list only requires putting CD_SKIPHIBERNATE into struct
cfdriver.

To test, upgrade the bootblocks -- which must pass a new boothowto flag
indicating the unhibernate operation -- previously the bsd.booted kernel
was unaware of the circumstances until inspecting swap for a unhib signature,
but this is too late (obviously it is after device configure):

   cd /usr/src/sys/arch/amd64/stand
   make && make install
   installboot -v sd0 (or whatever)

Then install a new kernel, and perform an unhibernate cycle.

Surface go2 has a pretty fast BIOS and many iic + usb devices, so this
cuts unhibernate elapsed time by roughly a third.  The other laptop I
trialed this on, x1nano, has fewer devices to skip so the speedup is
less dramatic.

I had some weird experiences developing this.  Consider reverting the
sys/dev/usb subdirectory flagging before reporting a problem, because
I suspect my weird problems came from those drivers.

Index: kern/subr_autoconf.c
===
RCS file: /cvs/src/sys/kern/subr_autoconf.c,v
retrieving revision 1.94
diff -u -p -u -r1.94 subr_autoconf.c
--- kern/subr_autoconf.c30 Dec 2019 23:56:26 -  1.94
+++ kern/subr_autoconf.c23 Oct 2021 20:42:16 -
@@ -51,6 +51,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "hotplug.h"
 #include "mpath.h"
@@ -188,7 +189,7 @@ config_search(cfmatch_t fn, struct devic
m.parent = parent;
m.match = NULL;
m.aux = aux;
-   m.indirect = parent && parent->dv_cfdata->cf_driver->cd_indirect;
+   m.indirect = parent && (parent->dv_cfdata->cf_driver->cd_mode & 
CD_INDIRECT);
m.pri = 0;
 
for (cf = cfdata; cf->cf_driver; cf++) {
@@ -202,6 +203,14 @@ config_search(cfmatch_t fn, struct devic
if (cf->cf_fstate == FSTATE_DNOTFOUND ||
cf->cf_fstate == FSTATE_DSTAR)
continue;
+   if (boothowto & RB_UNHIBERNATE) {
+   if (cf->cf_driver->cd_mode & CD_SKIPHIBERNATE)
+   continue;
+   if (cf->cf_driver->cd_class == DV_IFNET)
+   continue;
+   if (cf->cf_driver->cd_class == DV_TAPE)
+   continue;
+   }
for (p = cf->cf_parents; *p >= 0; p++)
if (parent->dv_cfdata == [*p])
mapply(, cf);
@@ -237,7 +246,7 @@ config_scan(cfscan_t fn, struct device *
void *match;
int indirect;
 
-   indirect = parent && parent->dv_cfdata->cf_driver->cd_indirect;
+   indirect = parent && (parent->dv_cfdata->cf_driver->cd_mode & 
CD_INDIRECT);
 
for (cf = cfdata; cf->cf_driver; cf++) {
/*
@@ -348,7 +357,7 @@ config_attach(struct device *parent, voi
autoconf_attdet++;
mtx_leave(_attdet_mtx);
 
-   if (parent && parent->dv_cfdata->cf_driver->cd_indirect) {
+   if (parent && (parent->dv_cfdata->cf_driver->cd_mode & CD_INDIRECT)) {
dev = match;
cf = dev->dv_cfdata;
} else {
Index: sys/device.h
===
RCS file: /cvs/src/sys/sys/device.h,v
retrieving revision 1.55
diff -u -p -u -r1.55 device.h
--- sys/device.h10 Sep 2018 16:18:34 -  1.55
+++ sys/device.h23 Oct 2021 20:42:16 -
@@ -136,11 +136,15 @@ struct cfattach {
 #defineDETACH_FORCE0x01/* force detachment; hardware 
gone */
 #defineDETACH_QUIET0x02/* don't print a notice */
 
+/* For cd_mode, below */
+#define CD_INDIRECT1
+#define CD_SKIPHIBERNATE   2
+
 struct cfdriver {
void**cd_devs;  /* devices found */
char*cd_name;   /* device name */
enumdevclass cd_class;  /* device classification */
-   int cd_indirect;/* indirectly configure subdevices */
+   int cd_mode;/* device type subclassification */
int cd_ndevs;   /* size of cd_devs array */
 };
 
Index: sys/reboot.h
===
RCS file: /cvs/src/sys/sys/reboot.h,v
retrieving revision 1.19
diff -u -p -u -r1.19 reboot.h
--- sys/reboot.h23 May 2020 00:40:53 -  1.19
+++ sys/reboot.h23 Oct 2021 20:42:16 -
@@ 

Re: installer/wifi drivers: use join by default

2021-10-24 Thread Theo de Raadt
yes, we are ready.

Klemens Nanni  wrote:

> I reckon the adoption of `join' was fairly successful and lots of people
> use it exclusively, so upon install I suggest to provide hostname.if(5)
> using it instead of `nwid'.
> 
> Likewise, reflect that trend in our driver manuals.
> 
> Feedback? Objection? OK?
> 
> 
> Index: distrib/miniroot/install.sub
> ===
> RCS file: /cvs/src/distrib/miniroot/install.sub,v
> retrieving revision 1.1182
> diff -u -p -r1.1182 install.sub
> --- distrib/miniroot/install.sub  24 Oct 2021 10:11:24 -  1.1182
> +++ distrib/miniroot/install.sub  24 Oct 2021 11:17:29 -
> @@ -1241,23 +1241,23 @@ ieee80211_config() {
>   ask_until "$_prompt" "O"
>   case "$_haswpa-$resp" in
>   ?-[Oo]) # No further questions
> - ifconfig $_if nwid "$_nwid"
> - quote nwid "$_nwid" >>$_hn
> + ifconfig $_if join "$_nwid"
> + quote join "$_nwid" >>$_hn
>   break
>   ;;
>   ?-[Ww]) ask_until "WEP key? (will echo)"
>   # Make sure ifconfig accepts the key.
> - if _err=$(ifconfig $_if nwid "$_nwid" nwkey 
> "$resp" 2>&1) &&
> + if _err=$(ifconfig $_if join "$_nwid" nwkey 
> "$resp" 2>&1) &&
>   [[ -z $_err ]]; then
> - quote nwid "$_nwid" nwkey "$resp" >>$_hn
> + quote join "$_nwid" nwkey "$resp" >>$_hn
>   break
>   fi
>   echo "$_err"
>   ;;
>   1-[Pp]) ask_until "WPA passphrase? (will echo)"
>   # Make sure ifconfig accepts the key.
> - if ifconfig $_if nwid "$_nwid" wpakey "$resp"; 
> then
> - quote nwid "$_nwid" wpakey "$resp" 
> >>$_hn
> + if ifconfig $_if join "$_nwid" wpakey "$resp"; 
> then
> + quote join "$_nwid" wpakey "$resp" 
> >>$_hn
>   break
>   fi
>   ;;
> Index: share/man/man4/acx.4
> ===
> RCS file: /cvs/src/share/man/man4/acx.4,v
> retrieving revision 1.49
> diff -u -p -r1.49 acx.4
> --- share/man/man4/acx.4  15 Oct 2021 08:10:44 -  1.49
> +++ share/man/man4/acx.4  24 Oct 2021 10:17:23 -
> @@ -165,7 +165,7 @@ using WEP key
>  .Dq mywepkey ,
>  obtaining an IP address using DHCP:
>  .Bd -literal -offset indent
> -nwid mynwid nwkey mywepkey
> +join mynwid nwkey mywepkey
>  inet autoconf
>  .Ed
>  .Pp
> @@ -174,7 +174,7 @@ The following
>  example creates a host-based access point on boot:
>  .Bd -literal -offset indent
>  mediaopt hostap
> -nwid mynwid nwkey mywepkey
> +join mynwid nwkey mywepkey
>  inet 192.168.1.1 255.255.255.0
>  .Ed
>  .Sh SEE ALSO
> Index: share/man/man4/an.4
> ===
> RCS file: /cvs/src/share/man/man4/an.4,v
> retrieving revision 1.45
> diff -u -p -r1.45 an.4
> --- share/man/man4/an.4   15 Oct 2021 08:10:44 -  1.45
> +++ share/man/man4/an.4   24 Oct 2021 10:17:23 -
> @@ -134,7 +134,7 @@ using WEP key
>  .Dq mywepkey ,
>  obtaining an IP address using DHCP:
>  .Bd -literal -offset indent
> -nwid mynwid nwkey mywepkey
> +join mynwid nwkey mywepkey
>  inet autoconf
>  .Ed
>  .Sh DIAGNOSTICS
> Index: share/man/man4/ath.4
> ===
> RCS file: /cvs/src/share/man/man4/ath.4,v
> retrieving revision 1.68
> diff -u -p -r1.68 ath.4
> --- share/man/man4/ath.4  15 Oct 2021 08:10:44 -  1.68
> +++ share/man/man4/ath.4  24 Oct 2021 10:17:23 -
> @@ -213,7 +213,7 @@ using WPA key
>  .Dq mywpakey ,
>  obtaining an IP address using DHCP:
>  .Bd -literal -offset indent
> -nwid mynwid wpakey mywpakey
> +join mynwid wpakey mywpakey
>  inet autoconf
>  .Ed
>  .Pp
> @@ -222,7 +222,7 @@ The following
>  example creates a host-based access point on boot:
>  .Bd -literal -offset indent
>  mediaopt hostap
> -nwid mynwid wpakey mywpakey
> +join mynwid wpakey mywpakey
>  inet 192.168.1.1 255.255.255.0
>  .Ed
>  .Sh DIAGNOSTICS
> Index: share/man/man4/athn.4
> ===
> RCS file: /cvs/src/share/man/man4/athn.4,v
> retrieving revision 1.42
> diff -u -p -r1.42 athn.4
> --- share/man/man4/athn.4 15 Oct 2021 08:10:44 -  1.42
> +++ share/man/man4/athn.4 24 Oct 2021 10:17:23 -
> @@ -206,7 

Re: unveil(2) stty(1)

2021-10-23 Thread Theo de Raadt
Ah, right.  OK deraadt


Ricardo Mestre  wrote:

> the semantics should be to call unveil on something before the NULL,NULL
> otherwise nothing will get unveiled, maybe bob disagrees? :)
> 
> regarding the weird behaviour well there are other programs with the same
> issue, see pfctl, it accepts several configs but only stops loading if one
> of them is bogus.
> 
> On 10:21 Sat 23 Oct , Theo de Raadt wrote:
> > But the -f file is opened above your proposed unveil() addition.  So I think
> > you only need unveil(NULL,NULL).
> > 
> > While here, I see a different weird problem:
> > 
> > stty -f file -f dsaf -f dsaf -f sadf -f asdf -f sadf 
> > 
> > You can pass lots of -f options, and stty will leak them the fd's.  I
> > suspect it can hit the fd limit before it hits the argv limit.  Anyways
> > just a strange behaviour.
> > 
> > 
> > Ricardo Mestre  wrote:
> > 
> > > stty(1) can't be pledged for all modes, but it can be unveiled. the only 
> > > file to
> > > be opened is on stty -f `file', so call unveil(2) afterwards to restrict 
> > > all fs
> > > access. tested with all arguments through ktrace/kdump.
> > > 
> > > ok?
> > > 
> > > Index: stty.c
> > > ===
> > > RCS file: /cvs/src/bin/stty/stty.c,v
> > > retrieving revision 1.21
> > > diff -u -p -u -r1.21 stty.c
> > > --- stty.c28 Jun 2019 13:35:00 -  1.21
> > > +++ stty.c23 Oct 2021 15:52:46 -
> > > @@ -82,6 +82,11 @@ main(int argc, char *argv[])
> > >  args:argc -= optind;
> > >   argv += optind;
> > >  
> > > + if (unveil("/", "") == -1)
> > > + err(1, "unveil /");
> > > + if (unveil(NULL, NULL) == -1)
> > > + err(1, "unveil");
> > > +
> > >   if (ioctl(i.fd, TIOCGETD, ) == -1)
> > >   err(1, "TIOCGETD");
> > >  
> > > 
> > 
> 



Re: unveil(2) stty(1)

2021-10-23 Thread Theo de Raadt
But the -f file is opened above your proposed unveil() addition.  So I think
you only need unveil(NULL,NULL).

While here, I see a different weird problem:

stty -f file -f dsaf -f dsaf -f sadf -f asdf -f sadf 

You can pass lots of -f options, and stty will leak them the fd's.  I
suspect it can hit the fd limit before it hits the argv limit.  Anyways
just a strange behaviour.


Ricardo Mestre  wrote:

> stty(1) can't be pledged for all modes, but it can be unveiled. the only file 
> to
> be opened is on stty -f `file', so call unveil(2) afterwards to restrict all 
> fs
> access. tested with all arguments through ktrace/kdump.
> 
> ok?
> 
> Index: stty.c
> ===
> RCS file: /cvs/src/bin/stty/stty.c,v
> retrieving revision 1.21
> diff -u -p -u -r1.21 stty.c
> --- stty.c28 Jun 2019 13:35:00 -  1.21
> +++ stty.c23 Oct 2021 15:52:46 -
> @@ -82,6 +82,11 @@ main(int argc, char *argv[])
>  args:argc -= optind;
>   argv += optind;
>  
> + if (unveil("/", "") == -1)
> + err(1, "unveil /");
> + if (unveil(NULL, NULL) == -1)
> + err(1, "unveil");
> +
>   if (ioctl(i.fd, TIOCGETD, ) == -1)
>   err(1, "TIOCGETD");
>  
> 



Re: ixl(4): add checksum receive offloading

2021-10-22 Thread Theo de Raadt
Claudio Jeker  wrote:

> For ospfd tests you want to make sure that some of the ospf packets need
> fragmenting. So this needs a sizeable network to hit this.

Yes, as I remember, the problems only related to fragments of large
packets.

These tests are too narrow and the results inconclusive.




Re: ixl(4): add checksum receive offloading

2021-10-22 Thread Theo de Raadt
the nfs issues related to fragments being incorrectly labelled as
checksummed.  I am vague on whether this was hardware incorrectly indicating
checksum valid in damaged packets.  of course, that was nothing compared
to the astoundingly nasty v6 checksum hardware bug in tht(4)...

Hrvoje Popovski  wrote:

> On 22.10.2021. 16:22, Sebastian Benoit wrote:
> > Stuart Henderson(s...@spacehopper.org) on 2021.10.22 12:55:20 +0100:
> >> On 2021/10/22 11:25, Jan Klemkow wrote:
> >>> this diff add hardware checksum offloading for the receive path of
> >>> ixl(4) interfaces.
> >>
> >> Would be good to have this tested with NFS if anyone has a way to do so.
> >> nics are probably better now but I'm pretty sure we have had problems
> >> with NFS and offloading in the past.
> > 
> > And ospf(6)d... mikeb hit that i believe?
> >  
> 
> and ospf6d seems to work
> 
> smc24# ospf6ctl show nei de
> 
> Neighbor 10.1.1.1, interface address fe80::eef4:bbff:feda:f7f8
>   Area 0.0.0.5, interface ixl0
>   Neighbor priority is 5, State is FULL, 7 state changes
>   DR is 161.53.253.247, BDR is 10.1.1.1
>   Options *|*|-|R|-|*|E|V6
>   Dead timer due in 00:00:17
>   Uptime 00:02:28
>   Database Summary List 0
>   Link State Request List 0
>   Link State Retransmission List 0
> 
> 
> Destination  Nexthop   Path TypeType  CostUptime
> ::10.1.1.1   fe80::eef4:bbff:feda:f7f8%ixl0  Inter-Area   Router
>10  00:01:54
> 2001:db8:15::/64 ::  C Intra-Area   Network   10
> 00:01:59
> ::/96fe80::eef4:bbff:feda:f7f8%ixl0  Type 1 ext
> Network   110 00:01:54
> 2002::/24fe80::eef4:bbff:feda:f7f8%ixl0  Type 1 ext
> Network   110 00:01:54
> 2002:db8:15::/64 fe80::eef4:bbff:feda:f7f8%ixl0  Type 1 ext
> Network   110 00:00:26
> 2002:7f00::/24   fe80::eef4:bbff:feda:f7f8%ixl0  Type 1 ext
> Network   110 00:01:54
> 2002:e000::/20   fe80::eef4:bbff:feda:f7f8%ixl0  Type 1 ext
> Network   110 00:01:54
> 2002:ff00::/24   fe80::eef4:bbff:feda:f7f8%ixl0  Type 1 ext
> Network   110 00:01:54
> 2003:db8:15::/64 fe80::eef4:bbff:feda:f7f8%ixl0  Type 1 ext
> Network   110 00:00:18
> 2004:db8:15::/64 fe80::eef4:bbff:feda:f7f8%ixl0  Type 1 ext
> Network   110 00:00:13
> 2005:db8:15::/64 fe80::eef4:bbff:feda:f7f8%ixl0  Type 1 ext
> Network   110 00:00:04
> 



Re: base64.c: u_char -> unsigned char

2021-10-22 Thread Theo de Raadt
Yes, this is better

Theo Buehler  wrote:

> LibreSSL portable uses base64.c. Not everyone has u_char, so this is a
> mild portability annoyance. This means that we can get rid of
> sys/types.h. I also removed stdio.h since that seems unused.
> 
> As far as I can see, base64.c directly needs
> 
> ctype.h for isspace()
> resolv.h for the function prototypes
> stdlib.h for size_t
> string.h for strchr()
> 
> The remaining includes seem to be needed for resolv.h, although
> sys/socket.h probably isn't needed either.
> 
> Index: net/base64.c
> ===
> RCS file: /cvs/src/lib/libc/net/base64.c,v
> retrieving revision 1.9
> diff -u -p -r1.9 base64.c
> --- net/base64.c  11 Oct 2021 14:32:26 -  1.9
> +++ net/base64.c  21 Oct 2021 17:18:29 -
> @@ -42,14 +42,12 @@
>   * IF IBM IS APPRISED OF THE POSSIBILITY OF SUCH DAMAGES.
>   */
>  
> -#include 
>  #include 
>  #include 
>  #include 
>  
>  #include 
>  #include 
> -#include 
>  
>  #include 
>  #include 
> @@ -123,14 +121,14 @@ static const char Pad64 = '=';
>  
>  int
>  b64_ntop(src, srclength, target, targsize)
> - u_char const *src;
> + unsigned char const *src;
>   size_t srclength;
>   char *target;
>   size_t targsize;
>  {
>   size_t datalength = 0;
> - u_char input[3];
> - u_char output[4];
> + unsigned char input[3];
> + unsigned char output[4];
>   int i;
>  
>   while (2 < srclength) {
> @@ -188,11 +186,11 @@ b64_ntop(src, srclength, target, targsiz
>  int
>  b64_pton(src, target, targsize)
>   char const *src;
> - u_char *target;
> + unsigned char *target;
>   size_t targsize;
>  {
>   int tarindex, state, ch;
> - u_char nextbyte;
> + unsigned char nextbyte;
>   char *pos;
>  
>   state = 0;
> 



Re: retire hifn safe ubsec

2021-10-21 Thread Theo de Raadt
Stuart Henderson  wrote:

> On 2021/10/21 16:30, Alexander Bluhm wrote:
> > Hi,
> > 
> > Goal is to retire the async crypto API.  It is slow and adds
> > complexity which hinders MP progress in IPsec.  It is used by the
> > old PCI devices hifn(4), safe(4), and ubsec(4).
> > 
> > These devices are not common anymore.  Using the CPU for crypto is
> > faster than offloading via the PCI bus.  By having special requirements
> > for the crypto API, those devices slow down modern machines.  They
> > only support crypto algorithms that are insecure nowadays.
> > 
> > ok to remove hifn(4) safe(4) ubsec(4) ?
> 
> OK. The main useful feature as far as I'm concerned is the rng but
> I don't think it's useful/common enough to be worth doing anything other
> than just deleting the drivers.

Perhaps that function can be left intact in a few drivers.

But honestly I think the software rng stack we have now does sufficient
amounts of churn, and I cannot see runtime operations which exhaust it.
Send or receive packets?  You are generating interrupts at unpredictable
interrupt time deltas, those get folded in.  This isn't a basic fold like
in other systems, it is powerful & sophisticated and I see no way it can
fall behind.



Re: head(1): fully support the legacy -count syntax

2021-10-10 Thread Theo de Raadt
Stuart Henderson  wrote:

x1> On 2021/10/10 14:26, Scott Cheloha wrote:
> > On Sun, Oct 10, 2021 at 12:31:22PM -0600, Theo de Raadt wrote:
> > > Bryan Steele  wrote:
> > > 
> > > > On Sun, Oct 10, 2021 at 12:18:55PM -0500, Scott Cheloha wrote:
> > > > > On Sun, Oct 10, 2021 at 10:51:29AM -0600, Theo de Raadt wrote:
> > > > > > did anyone ever use it this way, or are you getting ahead of 
> > > > > > yourself.
> > > > > 
> > > > > I don't understand the question.
> > > > 
> > > > I've only ever seen it used with -count as the first argument, can't
> > > > say it's every occoured to me to type "head file -10".
> > 
> > That is not what I proposed.  Reread my first message:
> > 
> > https://marc.info/?l=openbsd-tech=163388435528203=2
> 
> i.e. "head -2 -3 somefile" is taken as -3.
> 
> This is unportable syntax, GNU head doesn't support it, current OpenBSD head
> doesn't support it, and it doesn't seem to be really meaningful.
> Additionally I don't think we've ever had a problem with this in ports.
> I think we would be better served to keep things as-is and not support it.
> Seems that FreeBSD is the odd one out here?

Indeed, the problem is our code supports this

but noone else supports it

well, someone might accidentally use it in a script they write on OpenBSD

and... it is unportable, the behaviour is either different, or an error
condition

So who benefits?  Noone, the way I see it.



Re: head(1): fully support the legacy -count syntax

2021-10-10 Thread Theo de Raadt
Bryan Steele  wrote:

> On Sun, Oct 10, 2021 at 12:18:55PM -0500, Scott Cheloha wrote:
> > On Sun, Oct 10, 2021 at 10:51:29AM -0600, Theo de Raadt wrote:
> > > did anyone ever use it this way, or are you getting ahead of yourself.
> > 
> > I don't understand the question.
> 
> I've only ever seen it used with -count as the first argument, can't
> say it's every occoured to me to type "head file -10".

POSIX says options before arguments.  That is what we support.  We don't
support options after arguments.



Re: head(1): fully support the legacy -count syntax

2021-10-10 Thread Theo de Raadt
did anyone ever use it this way, or are you getting ahead of yourself.

Scott Cheloha  wrote:

> Hi,
> 
> head(1) takes line count arguments in two ways.  The legacy (1977)
> syntax is "-count" [1].  The "new" (1992) syntax is "-n count" [2].
> In either case, "count" must be a positive decimal value.
> 
> Somewhere along the way, support for the legacy syntax was neutered.
> At present it only works as expected if -count is the first option
> argument to head(1), i.e. this works:
> 
>   $ head -20 file
> 
> but this is an error:
> 
>   $ head -20 -25 file
> 
> I'm not a fan of this.  If we're going to support the legacy syntax I
> think it should behave like option arguments for other utilities.  You
> should be able to specify -count multiple times.  We do this for
> tail(1), which has a similar legacy syntax problem.
> 
> The easiest way to transparently support such invocations is to use
> the GNU double-colon extension for getopt(3).  We then rebuild the
> number string with asprintf(3), parse it with strtonum(3), and free
> the string.
> 
> Before:
> 
> $ jot 10 | head -5 -6
> head: unknown option -- 6
> usage: head [-count | -n count] [file ...]
> 
> After:
> 
> $ jot 10 | head -5 -6
> 1
> 2
> 3
> 4
> 5
> 6
> 
> --
> 
> ok?
> 
> [1] 
> https://svnweb.freebsd.org/csrg/usr.bin/head/head.c?revision=1026=markup
> 
> [2] 
> https://svnweb.freebsd.org/csrg/usr.bin/head/head.c?revision=52824=markup
> 
> Index: head.c
> ===
> RCS file: /cvs/src/usr.bin/head/head.c,v
> retrieving revision 1.22
> diff -u -p -r1.22 head.c
> --- head.c10 Oct 2021 15:57:25 -  1.22
> +++ head.c10 Oct 2021 16:44:01 -
> @@ -49,27 +49,30 @@ int
>  main(int argc, char *argv[])
>  {
>   const char *errstr;
> + char *str;
>   FILE*fp;
>   longcnt;
>   int ch, firsttime;
>   longlinecnt = 10;
> - int status = 0;
> + int error, status = 0;
>  
>   if (pledge("stdio rpath", NULL) == -1)
>   err(1, "pledge");
>  
> - /* handle obsolete -number syntax */
> - if (argc > 1 && argv[1][0] == '-' &&
> - isdigit((unsigned char)argv[1][1])) {
> - linecnt = strtonum(argv[1] + 1, 1, LONG_MAX, );
> - if (errstr != NULL)
> - errx(1, "count is %s: %s", errstr, argv[1] + 1);
> - argc--;
> - argv++;
> - }
> -
> - while ((ch = getopt(argc, argv, "n:")) != -1) {
> +#define OPTSTR "0::1::2::3::4::5::6::7::8::9::n:"
> + while ((ch = getopt(argc, argv, OPTSTR)) != -1) {
>   switch (ch) {
> + case '0': case '1': case '2': case '3': case '4':
> + case '5': case '6': case '7': case '8': case '9':
> + error = asprintf(, "%c%s",
> + ch, (optarg == NULL) ? "" : optarg);
> + if (error == -1)
> + err(1, "asprintf");
> + linecnt = strtonum(str, 1, LONG_MAX, );
> + if (errstr != NULL)
> + errx(1, "count is %s: %s", errstr, str);
> + free(str);
> + break;
>   case 'n':
>   linecnt = strtonum(optarg, 1, LONG_MAX, );
>   if (errstr != NULL)
> 



Re: [PATCH] /etc/netstart: unquoted command substitution inside arithmetic expression

2021-10-08 Thread Theo de Raadt
Philip Guenther  wrote:

> On Fri, Oct 8, 2021 at 8:57 AM Theo de Raadt  wrote:
> 
>  Philip Guenther  wrote:
> 
>  > On Thu, Oct 7, 2021 at 5:57 PM bm1les  wrote:
>  > 
>  > > --- netstart2 Sep 2021 19:38:20 -   1.216
>  > > +++ netstart8 Oct 2021 02:43:30 -
>  > > @@ -365,7 +365,7 @@ ifmstart "tun tap gif etherip gre egre p
>  > >  if [[ $ip6kernel == YES ]]; then
>  > > # Ensure IPv6 Duplicate Address Detection (DAD) is completed.
>  > > count=0
>  > > -   while ((count++ < 10 && $(sysctl -n net.inet6.ip6.dad_pending) !=
>  > > 0)); do
>  > > +   while ((count++ < 10 && "$(sysctl -n net.inet6.ip6.dad_pending)"
>  > > != 0)); do
>  > > sleep 1
>  > > done
>  > >  fi
>  > >
>  > 
>  > I can't figure out what problem you think this could solve.  Can you
>  > explain the circumstances under which those quotes could make a difference?
> 
>  Not the OP's issue, but I think a kernels compiled without option INET6
>  will return an errno, and I cannot tell if sysctl prints out an error message
>  or converts to "", the empty string, which would conceivably mis-parse.
> 
> AFAICT, an empty quoted string there results in the exact same error.  As I 
> wrote
> off-list to the original submitter:
> 
>  Can you be clearer about how the quoting makes the result any better when run
>  under bsd.rd?  Doesn't it fail in the same way?  Testing with 'echo' instead 
> would
>  seem to indicate so:
>  : bleys; (( 1 < 10 && $(echo) != 0 )); echo $?  
>  /bin/ksh:  1 < 10 &&  != 0 : unexpected `!='
>  2
>  : bleys; (( 1 < 10 && $(echo -n) != 0 )); echo $?  
>  /bin/ksh:  1 < 10 &&  != 0 : unexpected `!='
>  2
>  : bleys; (( 1 < 10 && "$(echo)" != 0 )); echo $?
>  /bin/ksh:  1 < 10 &&  != 0 : unexpected `!='
>  2
>  : bleys; (( 1 < 10 && "$(echo -n)" != 0 )); echo $?
>  /bin/ksh:  1 < 10 &&  != 0 : unexpected `!='
>  2
>  : bleys;

Well, netstart can do better, and should not emit low-level parsing errors



Re: [PATCH] /etc/netstart: unquoted command substitution inside arithmetic expression

2021-10-08 Thread Theo de Raadt
Philip Guenther  wrote:

> On Thu, Oct 7, 2021 at 5:57 PM bm1les  wrote:
> 
> > --- netstart2 Sep 2021 19:38:20 -   1.216
> > +++ netstart8 Oct 2021 02:43:30 -
> > @@ -365,7 +365,7 @@ ifmstart "tun tap gif etherip gre egre p
> >  if [[ $ip6kernel == YES ]]; then
> > # Ensure IPv6 Duplicate Address Detection (DAD) is completed.
> > count=0
> > -   while ((count++ < 10 && $(sysctl -n net.inet6.ip6.dad_pending) !=
> > 0)); do
> > +   while ((count++ < 10 && "$(sysctl -n net.inet6.ip6.dad_pending)"
> > != 0)); do
> > sleep 1
> > done
> >  fi
> >
> 
> I can't figure out what problem you think this could solve.  Can you
> explain the circumstances under which those quotes could make a difference?

Not the OP's issue, but I think a kernels compiled without option INET6
will return an errno, and I cannot tell if sysctl prints out an error message
or converts to "", the empty string, which would conceivably mis-parse.





Re: Remove deprecated variables in sysctl(2)

2021-10-05 Thread Theo de Raadt
Solene Rapenne  wrote:

> Variables HW_PHYSMEM and HW_USERMEM were deprecated 13 years ago,
> maybe we can remove them from sysctl(2)?
> 
> It was originally in sysctl(3) if someone want to do some history
> research
> https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/lib/libc/gen/Attic/sysctl.3#rev1.178

In a better world, HW_PHYSMEM and HW_USERMEM should become 64-bit by default,
aliases for the 64-bit hackery that was introduced.  Then everyone is happy.



Re: Remove deprecated variables in sysctl(2)

2021-10-05 Thread Theo de Raadt
Stuart Henderson  wrote:

> On 2021/10/05 18:11, Solene Rapenne wrote:
> > Variables HW_PHYSMEM and HW_USERMEM were deprecated 13 years ago,
> > maybe we can remove them from sysctl(2)?
> 
> "deprecated" doesn't mean removed or disabled, it just means that one
> shouldn't use them. These are still in system headers and supported by
> the kernel, and could possibly be used by ports.

Yep.

Please do not remove.



Re: Unref/free amap w/o KERNEL_LOCK()

2021-10-01 Thread Theo de Raadt
> but please wait with putting in further diffs until Theo has started
> building snaps

That is very important: continual community testing of snapshot kernels
is incredibly important, otherwise layers of related commits can happen,
and it becomes harder to diagnose introduced problems.





Re: CVE-2021-26333 Low privileged malicious users may be able to access and leak data through the AMD Chipset Driver

2021-10-01 Thread Theo de Raadt
Dear Chicken Little -

We would appreciate if you learn to actually read before yelling about
the sky falling.

The alert is not about spectre, meltdown, or foreshadow.

The report is not about hurricanes, forest fires, salmonena in the spinach,
or cancer-causing chemicals in the chicken-feed either.

It is about bug in a *WINDOWS DEVICE DRIVER*

It is in the FIRST SENTENCE.

Those of us on the list have mitigated against this problem by not running
Windows.


Jerome KASPER  wrote:

> Hello tech@,
> After spectre, meltdown and foreshadow, sounds like speculative attacks
> are to come in a next future. Today i just came through that bulletin from
> AMD:
> https://www.amd.com/en/corporate/product-security/bulletin/amd-sb-1009
> Details of the Attack there :
> https://zeroperil.co.uk/wp-content/uploads/2021/09/AMD_PSP_Vulnerability_Report.pdf
> Just wanted to let you know and was wondering if some mitigation was
> on-going?
> Best regards,
> Jerome Kasper



Re: Variable type fix in parse.y (all of them)

2021-09-29 Thread Theo de Raadt
OK deraadt



Re: riscv makedev wd0

2021-09-28 Thread Theo de Raadt
Alexander Bluhm  wrote:

> in regress/etc/MAKEDEV I see this "wd0: unknown device" error in
> riscv64 ramdisk.
> 
>  run-riscv64-ramdisk 
> rm -rf -- riscv64-ramdisk.dir
> mkdir -m 700 riscv64-ramdisk.dir
> cp /usr/src/regress/etc/MAKEDEV/../../../etc/etc.riscv64/MAKEDEV 
> riscv64-ramdisk.dir/
> chown root:wheel riscv64-ramdisk.dir
> time sh -c 'cd riscv64-ramdisk.dir && sh ./MAKEDEV ramdisk'
> wd0: unknown device
> 0m00.27s real 0m00.06s user 0m00.21s system
> ls -ln riscv64-ramdisk.dir/ |  awk '/^[bcps]/ {printf "%s %x.%x 
> %x,%x%s\n",$1,$3,$4,$5,$6,$10}  /^l/  {printf "%s 
> %s.%s%s>%s\n",$1,$3,$4,$9,$11}' |  sort +5 -n |  sed -e 
> 's/rwx/7/g;s/rw-/6/g;s/r-x/5/g;s/r--/4/g'  -e 
> 's/-wx/3/g;s/-w-/2/g;s/--x/1/g;s/---/0/g'  -e 's/^\([bcpsl]\)\([0-9][0-9]*\) 
> /\2\1/'  >riscv64-ramdisk.out
> 
> If we have no wd(4) on riscv64, we should also remove it from ramdisk.

there are a whole bunch of things wrong in conf.c, because the riscv64 codebase
was built upon code from an architecture which had wd support ripped out.
I'll work on putting the pieces back in place, and send a diff later.



Re: Missing include in sys/device.h

2021-09-27 Thread Theo de Raadt
Oh, like how about you try compiling a kernel after your proposed diff?

in userland -
   size_t comes from sys/types.h
   or a header file which pulls in sys/types.h, and there should be
   no further new iteration added

in kernel -
size_t comes from either sys/param.h or sys/types.h, depending
on which one a file includes

What you are suggesting is completely crazy.  It's akin to "all
header files should include all other dependent header files, and
hey maybe one day we can have a #include_all directive, eh?

Absolutely no way.

Rafael Sadowski  wrote:

> device.h misses a definition of size_t
> 
> /usr/include/sys/device.h:128:2: error: unknown type name 'size_t'
> 
> Index: device.h
> ===
> RCS file: /cvs/src/sys/sys/device.h,v
> retrieving revision 1.55
> diff -u -p -r1.55 device.h
> --- device.h  10 Sep 2018 16:18:34 -  1.55
> +++ device.h  28 Sep 2021 05:04:57 -
> @@ -44,6 +44,8 @@
>  #ifndef _SYS_DEVICE_H_
>  #define  _SYS_DEVICE_H_
>  
> +#include 
> +
>  #include 
>  
>  /*
> 



Re: [patch] tcpdump: Sync DNS types with IANA

2021-09-23 Thread Theo de Raadt
Matthew Martin  wrote:

> On Tue, May 18, 2021 at 10:24:00PM -0500, Matthew Martin wrote:
> > Sync the DNS types with IANA[1] and upstream[2]. With this the Type65
> > queries show up as HTTPS.
> > 
> > Removed the UNSPECA type parsing as IANA has that query type number
> > assigned to NID now.
> > 
> > Also added a const on ns_class2str.
> > 
> > 1: https://www.iana.org/assignments/dns-parameters/dns-parameters.xhtml
> > 2: https://github.com/the-tcpdump-group/tcpdump/pull/912

I would be happier if /usr/include/arpa/nameser.h was updated properly
FIRST.

This would involved merging facts from both sides to make it correct,
then the tcpdump file can get deleted.

There are some almost scary differences:

+#define MAXDNAME   256 /* maximum domain name */

This is not used in tcpdump.  But it is scary because if someone used
it, and attempted to interface to some other library function, they
would be operating on definition different from the system.  That is a
nono.

I'll take a shot at the merge.



Re: Should 80MB of RAM be enough for kernel relinking on i386?

2021-09-23 Thread Theo de Raadt
You've made a pretty big assumption there.

When we see something we don't think is important, we delete the
email and carry on with more important things.

You think hand-tuning this one paragraph is going to reduce the amount
of mail we delete because it doesn't matter?  It won't. 

A large number of inaccurate documentation problems in the world can be
solved by deleting the innacurate documentation rather than trying to fine-tune
it to be more accurate for the moment, until it becomes inaccurate again.
In truth, noone cares about this paragraph.   It has been perceveived as
a promise.  We don't need to make statements people perceive as promises.

Patrick Harper  wrote:

> Because you might not wish to deal with people like me posting dmesg's 
> of absurdly deficient hardware on the lists. Putting minimum values up 
> means you can cast off everything you don't want to bother trying to 
> support as 'not worth your time', without anyone else having to 
> discover that it wasn't worth their time either.
> 
> On Thu, 23 Sep 2021, at 21:25, Theo de Raadt wrote:
> > We don't list memory sizing for other architectures.  Why list it for i386?
> >
> > Why not delete the text?
> >
> > Because someone wants the truth?  You can't handle 



Re: Should 80MB of RAM be enough for kernel relinking on i386?

2021-09-23 Thread Theo de Raadt
Stuart Henderson  wrote:

> Nobody should really by using i386 for new systems. The advantages
> of running amd64-compatible hardware are too big to ignore. Lower power
> consumption, much faster in most cases (the extra register used by PIE
> hurts i386 mode a lot more than amd64 mode with its extra registers),
> more address space for ASLR, some other security mitigations don't
> work on i386, more compatible with software in ports.
> 
> If it's an old system then you already have it so you can just see
> for yourself how much space it needs (and make your own decisions on
> swap space vs RAM, and whether to disable relinking at boot [just using
> it for upgrades/syspatch], and whether to bodge things to use ld.bfd to
> save RAM). So much depends on how you're going to use the system that
> I don't think it's really useful to publish the numbers.

We don't list memory sizing for other architectures.  Why list it for i386?

Why not delete the text?

Because someone wants the truth?  You can't handle 



Re: Possible bug in lib/libc/gen/exec.c

2021-09-16 Thread Theo de Raadt
It always returns -1 until the world changes in some subtle way,
then the code is wrong.

The logic is supposed to return what execve returns, not reinvent
the value.

Over decades this kind of assumption can turn into a bug, so I
prefer to do it right.

Alejandro Colomar (man-pages)  wrote:

> Hello Theo,
> 
> On 9/16/21 10:53 PM, Theo de Raadt wrote:
> > @@ -45,25 +46,31 @@ execl(const char *name, const char *arg,
> >   {
> > va_list ap;
> > char **argv;
> > -   int n;
> > +   size_t maplen;
> > +   int save_errno, n, error;
> 
> See below.
> 
> > va_start(ap, arg);
> > n = 1;
> > while (va_arg(ap, char *) != NULL)
> > n++;
> > va_end(ap);
> > -   argv = alloca((n + 1) * sizeof(*argv));
> > -   if (argv == NULL) {
> > -   errno = ENOMEM;
> > +   maplen = (n + 1) * sizeof(*argv);
> > +   argv = mmap(NULL, maplen, PROT_WRITE|PROT_READ,
> > +   MAP_ANON|MAP_PRIVATE, -1, 0);
> > +   if (argv == MAP_FAILED)
> > return (-1);
> > -   }
> > va_start(ap, arg);
> > n = 1;
> > argv[0] = (char *)arg;
> > while ((argv[n] = va_arg(ap, char *)) != NULL)
> > n++;
> > va_end(ap);
> > -   return (execve(name, argv, environ));
> > +
> > +   error = execve(name, argv, environ);
> > +   save_errno = errno;
> > +   munmap(argv, maplen);
> > +   errno = save_errno;
> > +   return (error);
> 
> This part could be simplified to:
> 
>   (void) execve(name, argv, environ);
>   save_errno = errno;
>   munmap(argv, maplen);
>   errno = save_errno;
>   return -1;
> 
> Allowing to remove the error variable.  execve(2) always returns -1.
> 
> >   }
> >   DEF_WEAK(execl);
> >   @@ -72,18 +79,21 @@ execle(const char *name, const char *arg
> >   {
> > va_list ap;
> > char **argv, **envp;
> > -   int n;
> > +   size_t maplen;
> > +   int save_errno, n, error;
> 
> Same here.
> 
> > @@ -91,7 +101,12 @@ execle(const char *name, const char *arg
> > n++;
> > envp = va_arg(ap, char **);
> > va_end(ap);
> > -   return (execve(name, argv, envp));
> > +
> > +   error = execve(name, argv, envp);
> > +   save_errno = errno;
> > +   munmap(argv, maplen);
> > +   errno = save_errno;
> > +   return error;
> 
> Same here
> 
> > @@ -99,25 +114,32 @@ execlp(const char *name, const char *arg
> >   {
> > va_list ap;
> > char **argv;
> > -   int n;
> > +   size_t maplen;
> > +   int save_errno, n, error;
> 
> Same here.
> 
> 
> 
> -- 
> Alejandro Colomar
> Linux man-pages comaintainer; https://www.kernel.org/doc/man-pages/
> http://www.alejandro-colomar.es/



Re: tls_pending(3)

2021-09-16 Thread Theo de Raadt
Interestingly, there was a long discussion over beer at a hackathon
recently, about non-blocking and poll/select/kqueue/etc, in particular
when mixed with TLS codepaths.

In my view, mixing poll()-style event loops with non-blocking
descriptors will -- 99% of the time -- result in people writing broken
event processing loops, and when they cannot figure out the breakage,
they will double down repeatedly adding more and more hacks, and never
solve the problem.  We have seen poll loops with nested poll loops.

I believe we should not encourage use of non-blocking mode.

Actually, we should discourage non-blocking loops, by not providing
APIs which encourage busy-loop polling.  

Non-blocking mode is poisonous.

Kristaps Dzonsons  wrote:

> Hi,
> 
> I'm porting a nonblocking, polling OpenSSL system to libtls.  However,
> I'm not sure how this is non-hackily possible without SSL_pending(3)
> to detect if less data is read with tls_read() than is buffered.
> 
>  writer:
>   tls_write(40)
> 
>  reader:
>   poll(POLLIN, INFTIM) -> POLLIN /* descriptor has a read */
>   tls_read(20) /* 20 bytes read, 20 bytes remain */
>   poll(POLLIN, INFTIM) -> 0 /* data was buffered */
> 
> This introduces tls_pending(3), which calls through to SSL_pending(3),
> and includes a simple example in tls_read(3).
> 
> Beck's tutorial says that "libtls is designed to do the common things
> you do for making TLS connections easy", and I'm not sure my use case 
> meets that standard.  If it looks reasonable, however, I can also add
> the regression tests.
> 
> Thank you,
> 
> Kristaps



Re: Possible bug in lib/libc/gen/exec.c

2021-09-16 Thread Theo de Raadt
enh  wrote:

> we had the same issue in bionic when we removed all our alloca()s, modulo the 
> fact
> that ours is a VLA rather than alloca(), but same thing:
> https://android.googlesource.com/platform/bionic/+/master/libc/bionic/exec.cpp#61

that cargo culting doesn't fix anything...

> we argued that it doesn't matter in this case though because we'll touch all 
> the
> entries, in order, so there's no possibility of jumping off the end of the 
> stack onto
> another VMA.

we've always had a sufficient gap beyond the end of our stack, but a few
years ago many people freaked out because they didn't.  nowadays, that is
enforced on a system level, so that door is largely closed, dependent on
linear access.

> (we have used mmap() instead in more complicated cases though.)

yes, that is the solution, but rollback and errno must be handled carefully.

If this mmap() fix goes in, the only alloca() remain in our tree are:

   clang - seems to be in test code only
   binutils - shocking
   1 in gcc configure
   1 in perl - non-openbsd cases
   1 small one in libcrypto - someone should go fix that



Re: Possible bug in lib/libc/gen/exec.c

2021-09-16 Thread Theo de Raadt
Theo de Raadt  wrote:

> Maybe we should investigate using mmap.  Of the 4 cases, 3 are not
> too difficult, but the 4th case will be very messy, including unwind
> for the 3rd case.

Here is a version that uses mmap instead of alloca, including rollback
of resource allocations in case of failure.  This is similar to the mmap
use in vfprintf which I added many years ago to support positional
arguments without alloca or malloc, so that snprintf family can be
remain maximally reentrant.  So I've been here before...

I have done a little testing, but it may still have bugs.

Index: lib/libc/gen/exec.c
===
RCS file: /cvs/src/lib/libc/gen/exec.c,v
retrieving revision 1.23
diff -u -p -u -r1.23 exec.c
--- lib/libc/gen/exec.c 13 Mar 2016 18:34:20 -  1.23
+++ lib/libc/gen/exec.c 16 Sep 2021 20:48:25 -
@@ -30,6 +30,7 @@
 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -45,25 +46,31 @@ execl(const char *name, const char *arg,
 {
va_list ap;
char **argv;
-   int n;
+   size_t maplen;
+   int save_errno, n, error;
 
va_start(ap, arg);
n = 1;
while (va_arg(ap, char *) != NULL)
n++;
va_end(ap);
-   argv = alloca((n + 1) * sizeof(*argv));
-   if (argv == NULL) {
-   errno = ENOMEM;
+   maplen = (n + 1) * sizeof(*argv);
+   argv = mmap(NULL, maplen, PROT_WRITE|PROT_READ,
+   MAP_ANON|MAP_PRIVATE, -1, 0);
+   if (argv == MAP_FAILED)
return (-1);
-   }
va_start(ap, arg);
n = 1;
argv[0] = (char *)arg;
while ((argv[n] = va_arg(ap, char *)) != NULL)
n++;
va_end(ap);
-   return (execve(name, argv, environ));
+
+   error = execve(name, argv, environ);
+   save_errno = errno;
+   munmap(argv, maplen);
+   errno = save_errno;
+   return (error);
 }
 DEF_WEAK(execl);
 
@@ -72,18 +79,21 @@ execle(const char *name, const char *arg
 {
va_list ap;
char **argv, **envp;
-   int n;
+   size_t maplen;
+   int save_errno, n, error;
 
va_start(ap, arg);
n = 1;
while (va_arg(ap, char *) != NULL)
n++;
va_end(ap);
-   argv = alloca((n + 1) * sizeof(*argv));
-   if (argv == NULL) {
-   errno = ENOMEM;
+
+   maplen = (n + 1) * sizeof(*argv);
+   argv = mmap(NULL, maplen, PROT_WRITE|PROT_READ,
+   MAP_ANON|MAP_PRIVATE, -1, 0);
+   if (argv == MAP_FAILED)
return (-1);
-   }
+
va_start(ap, arg);
n = 1;
argv[0] = (char *)arg;
@@ -91,7 +101,12 @@ execle(const char *name, const char *arg
n++;
envp = va_arg(ap, char **);
va_end(ap);
-   return (execve(name, argv, envp));
+
+   error = execve(name, argv, envp);
+   save_errno = errno;
+   munmap(argv, maplen);
+   errno = save_errno;
+   return error;
 }
 
 int
@@ -99,25 +114,32 @@ execlp(const char *name, const char *arg
 {
va_list ap;
char **argv;
-   int n;
+   size_t maplen;
+   int save_errno, n, error;
 
va_start(ap, arg);
n = 1;
while (va_arg(ap, char *) != NULL)
n++;
va_end(ap);
-   argv = alloca((n + 1) * sizeof(*argv));
-   if (argv == NULL) {
-   errno = ENOMEM;
+
+   maplen = (n + 1) * sizeof(*argv);
+   argv = mmap(NULL, maplen, PROT_WRITE|PROT_READ,
+   MAP_ANON|MAP_PRIVATE, -1, 0);
+   if (argv == MAP_FAILED)
return (-1);
-   }
+
va_start(ap, arg);
n = 1;
argv[0] = (char *)arg;
while ((argv[n] = va_arg(ap, char *)) != NULL)
n++;
va_end(ap);
-   return (execvp(name, argv));
+   error = execvp(name, argv);
+   save_errno = errno;
+   munmap(argv, maplen);
+   errno = save_errno;
+   return error;
 }
 
 int
@@ -132,10 +154,12 @@ execvpe(const char *name, char *const *a
 {
char **memp;
int cnt;
-   size_t lp, ln, len;
+   size_t lp, ln, curlen;
char *p;
int eacces = 0;
char *bp, *cur, *path, buf[PATH_MAX];
+   size_t maplen;
+   int save_errno, n;
 
/*
 * Do not allow null name
@@ -156,13 +180,14 @@ execvpe(const char *name, char *const *a
/* Get the path we're searching. */
if (!(path = getenv("PATH")))
path = _PATH_DEFPATH;
-   len = strlen(path) + 1;
-   cur = alloca(len);
-   if (cur == NULL) {
-   errno = ENOMEM;
+
+   curlen = strlen(path) + 1;
+   cur = mmap(NULL, curlen, PROT_WRITE|PROT_READ,
+   MAP_ANON|MAP_PRIVATE, -1, 0);
+   if (cur == MAP_FAILED)
return (-1);
-   }
-   strlcpy(cur, path, len);
+
+   strlcpy(cur, path, curlen);
path = cur;
   

Re: Possible bug in lib/libc/gen/exec.c

2021-09-16 Thread Theo de Raadt
Alejandro Colomar (man-pages)  wrote:

> Hi,
> 
> I don't know if OpenBSD has a different implementation of alloca(3)
> than Linux.  In Linux, alloca(3) (a.k.a. __builtin_alloca()) can't
> return NULL, as it can't detect errors.

There are no alloca can return NULL.

> The only way to detect an
> error is to add a handler for SIGSEGV, AFAIK.

Actually, it is worse than that.  Massive alloca have been encountered
which reach off the stack.  For over 20 years we've been on a mission
to delete alloca in our tree, and these are the last ones that remain,
because other allocators are very difficult to use in this place.

Maybe we should investigate using mmap.  Of the 4 cases, 3 are not
too difficult, but the 4th case will be very messy, including unwind
for the 3rd case.

> I found the following code in :
> 
>   argv = alloca((n + 1) * sizeof(*argv));
>   if (argv == NULL) {
>   errno = ENOMEM;
>   return (-1);
>   }
> 
> But of course, that NULL should never happen, right?

> As a side note, I think you could encapsulate the code calling
> alloca(3) into a helper macro, to avoid repetition.

I don't see how that would help anything.
 
> I can prepare a patch if you confirm the bug.

What do you propose to do in your patch?




Re: xdpyinfo: can't load library 'libdmx.so.2.0'

2021-09-15 Thread Theo de Raadt
the build system should not be discovering things!


Sebastien Marie  wrote:

> On Wed, Sep 15, 2021 at 05:11:12AM +, Renato Aguiar wrote:
> > I noticed this after sysupgrading to latest snapshot:
> > 
> > $ ldd /usr/X11R6/bin/xdpyinfo
> > /usr/X11R6/bin/xdpyinfo:
> > ld.so: xdpyinfo: can't load library 'libdmx.so.2.0'
> > /usr/X11R6/bin/xdpyinfo: signal 9
> > 
> > It works fine after manually building and installing xdpyinfo from
> > latest xenocara code.
> > 
> 
> libdmx.so has been removed recently. From
> xenocara/app/xdpyinfo/configure, dmx library could be detected as
> build time.
> 
> I suppose the host used to build the snapshots still has old dmx
> library in path, and the build of xdpyinfo detected it and link to it,
> whereas the library isn't in sets anymore.
> 
> Thanks for the report.
> -- 
> Sebastien Marie
> 



Re: riscv64: icache flush using sysarch(2)

2021-09-13 Thread Theo de Raadt
Mark Kettenis  wrote:

> Really depends on how the future riscv64 instructions behave.  A valid
> concern so the XXX is appropriate.  But I would pass the address and
> size to pmap_proc_iflush().

Yes, let the pmap decide what it wants to do.



Re: Change vm_dsize to vsize_t

2021-09-10 Thread Theo de Raadt
Sebastien Marie  wrote:

> On Fri, Sep 10, 2021 at 08:12:58AM -0600, Theo de Raadt wrote:
> > > for Rust programs. It should even be free as Rust libc crate doesn't
> > > have `struct kinfo_proc` definition. I can't comment on Go side.
> > 
> > The structure has nothing to do with libc!
> > 
> > So why would it occur there?
> 
> Because Rust libc is where Rust developers puts all the OS stuff.
> 
> The `struct kinfo_vmentry' or `struct kinfo_proc` on FreeBSD is
> defined in Rust libc crate for example.
> 
> https://github.com/rust-lang/libc/blob/master/src/unix/bsd/freebsdlike/freebsd/mod.rs#L173


They have lost the plot.

Decisions will be made as follows -- does rust get to define change
policy, or do systems get to define change policy, when they encounter
a mistake they made.

Systems contain mistakes.  Not all mistakes can be fixed without ABI
or API change.

What is being proposed is a fix that requires ABI change.

Rust policy is irrelevant, they are out of scope, and the 'policy' /
'implimentation' you describe is going to bear them consequences
eventually on *ALL SYSTEMS INTERFACES IN ALL SYSTEMS*, because their
requirement is completely unreasonable.  Not even Linux is as stable
as they want, over a decade.


What I find amazing here is that rust is not a source-code focused language.
What you have described is making binary data authoritative rather than source.





Re: Change vm_dsize to vsize_t

2021-09-10 Thread Theo de Raadt
> for Rust programs. It should even be free as Rust libc crate doesn't
> have `struct kinfo_proc` definition. I can't comment on Go side.

The structure has nothing to do with libc!

So why would it occur there?



Re: Unhibernation broken with boot.conf

2021-09-09 Thread Theo de Raadt
The hibernate check does:

bootdev_has_hibernate(void)
{
return ((bootdev_dip->bios_info.flags & BDI_HIBVALID)? 1 : 0);


Someone could have a boot.conf that changes their default disk because
they want to boot from a non-standard disk.  Hibernate store will occur
to swap on the non-standard disk.  They want to resume from that same
non-standard disk.  I have not tested this configuration, but it is
plausible.

However by checking for BDI_HIBVALID against the wrong disk, the hibernate
will fail for them.

So you've fixed something you don't like, and broken a different
configuration.



Theo de Raadt  wrote:

> that is a demo.
> 
> It breaks hibernate.  Lots of things in boot.conf could break hibernate.
> Why is that so surprising?  You haven't explained why you need to use
> boot.conf
> 
> But furthermore, you want to skip boot.conf parsing entirely, in the
> hibernate case.  I believe you have ignored the potential consequences
> of that upon others.
> 
> ha...@sdf.org wrote:
> 
> > My justification is documented in the boot.conf manual:
> > 
> >  Remove the 5 second pause at boot-time permanently, causing
> >  boot to load the kernel immediately without prompting:
> > 
> ># echo "boot" > /etc/boot.conf
> > 
> > Is the proposed behavior incorrect?
> > 
> > > Disagree.
> > > 
> > > There is no justification for what you have in /etc/boot.conf
> > > 
> > > ha...@sdf.org wrote:
> > > 
> > > > Hello tech@,
> > > > 
> > > > Currently unhibernating doesn't work with this configuration:
> > > > 
> > > > # echo "boot" > /etc/boot.conf
> > > > 
> > > > The original kernel is booted instead of the hibernated one.
> > > > This was fixed a few years ago for sysupgrade's /bsd.upgrade kernel but
> > > > unhibernation was overlooked at the time.
> > > > 
> > > > Before:
> > > > 
> > > > unhibernate detected: switching to /bsd.booted
> > > > booting sr0a:/bsd:...
> > > > 
> > > > After:
> > > > 
> > > > unhibernate detected: switching to /bsd.booted
> > > > booting sr0a:/bsd.booted:...
> > > > 
> > > > Index: boot.c
> > > > ===
> > > > RCS file: /cvs/src/sys/stand/boot/boot.c,v
> > > > retrieving revision 1.54
> > > > diff -u -p -r1.54 boot.c
> > > > --- boot.c  15 Jun 2020 14:43:57 -  1.54
> > > > +++ boot.c  8 Sep 2021 14:45:09 -
> > > > @@ -84,8 +84,6 @@ boot(dev_t bootdev)
> > > > isupgrade = 1;
> > > > }
> > > >  
> > > > -   st = read_conf();
> > > > -
> > > >  #ifdef HIBERNATE
> > > > int bootdev_has_hibernate(void);
> > > >  
> > > > @@ -94,6 +92,8 @@ boot(dev_t bootdev)
> > > > printf("unhibernate detected: switching to %s\n", 
> > > > cmd.image);
> > > > }
> > > >  #endif
> > > > +
> > > > +   st = read_conf();
> > > >  
> > > > if (!bootprompt)
> > > > snprintf(cmd.path, sizeof cmd.path, "%s:%s",
> > > >
> > 
> 



Re: Unhibernation broken with boot.conf

2021-09-09 Thread Theo de Raadt
that is a demo.

It breaks hibernate.  Lots of things in boot.conf could break hibernate.
Why is that so surprising?  You haven't explained why you need to use
boot.conf

But furthermore, you want to skip boot.conf parsing entirely, in the
hibernate case.  I believe you have ignored the potential consequences
of that upon others.

ha...@sdf.org wrote:

> My justification is documented in the boot.conf manual:
> 
>  Remove the 5 second pause at boot-time permanently, causing
>  boot to load the kernel immediately without prompting:
> 
># echo "boot" > /etc/boot.conf
> 
> Is the proposed behavior incorrect?
> 
> > Disagree.
> > 
> > There is no justification for what you have in /etc/boot.conf
> > 
> > ha...@sdf.org wrote:
> > 
> > > Hello tech@,
> > > 
> > > Currently unhibernating doesn't work with this configuration:
> > > 
> > > # echo "boot" > /etc/boot.conf
> > > 
> > > The original kernel is booted instead of the hibernated one.
> > > This was fixed a few years ago for sysupgrade's /bsd.upgrade kernel but
> > > unhibernation was overlooked at the time.
> > > 
> > > Before:
> > > 
> > >   unhibernate detected: switching to /bsd.booted
> > >   booting sr0a:/bsd:...
> > > 
> > > After:
> > > 
> > >   unhibernate detected: switching to /bsd.booted
> > >   booting sr0a:/bsd.booted:...
> > > 
> > > Index: boot.c
> > > ===
> > > RCS file: /cvs/src/sys/stand/boot/boot.c,v
> > > retrieving revision 1.54
> > > diff -u -p -r1.54 boot.c
> > > --- boot.c15 Jun 2020 14:43:57 -  1.54
> > > +++ boot.c8 Sep 2021 14:45:09 -
> > > @@ -84,8 +84,6 @@ boot(dev_t bootdev)
> > >   isupgrade = 1;
> > >   }
> > >  
> > > - st = read_conf();
> > > -
> > >  #ifdef HIBERNATE
> > >   int bootdev_has_hibernate(void);
> > >  
> > > @@ -94,6 +92,8 @@ boot(dev_t bootdev)
> > >   printf("unhibernate detected: switching to %s\n", cmd.image);
> > >   }
> > >  #endif
> > > +
> > > + st = read_conf();
> > >  
> > >   if (!bootprompt)
> > >   snprintf(cmd.path, sizeof cmd.path, "%s:%s",
> > >
> 



Re: Unhibernation broken with boot.conf

2021-09-09 Thread Theo de Raadt
Disagree.

There is no justification for what you have in /etc/boot.conf

ha...@sdf.org wrote:

> Hello tech@,
> 
> Currently unhibernating doesn't work with this configuration:
> 
> # echo "boot" > /etc/boot.conf
> 
> The original kernel is booted instead of the hibernated one.
> This was fixed a few years ago for sysupgrade's /bsd.upgrade kernel but
> unhibernation was overlooked at the time.
> 
> Before:
> 
>   unhibernate detected: switching to /bsd.booted
>   booting sr0a:/bsd:...
> 
> After:
> 
>   unhibernate detected: switching to /bsd.booted
>   booting sr0a:/bsd.booted:...
> 
> Index: boot.c
> ===
> RCS file: /cvs/src/sys/stand/boot/boot.c,v
> retrieving revision 1.54
> diff -u -p -r1.54 boot.c
> --- boot.c15 Jun 2020 14:43:57 -  1.54
> +++ boot.c8 Sep 2021 14:45:09 -
> @@ -84,8 +84,6 @@ boot(dev_t bootdev)
>   isupgrade = 1;
>   }
>  
> - st = read_conf();
> -
>  #ifdef HIBERNATE
>   int bootdev_has_hibernate(void);
>  
> @@ -94,6 +92,8 @@ boot(dev_t bootdev)
>   printf("unhibernate detected: switching to %s\n", cmd.image);
>   }
>  #endif
> +
> + st = read_conf();
>  
>   if (!bootprompt)
>   snprintf(cmd.path, sizeof cmd.path, "%s:%s",
> 



Re: Change vm_dsize to vsize_t

2021-09-09 Thread Theo de Raadt
Stuart Henderson  wrote:

> On 2021/09/09 06:47, Greg Steuck wrote:
> > Mark Kettenis  writes:
> > 
> > >> From: "Theo de Raadt" 
> > >> Date: Tue, 07 Sep 2021 07:08:19 -0600
> > >> 
> > >> Or we could coordinate the Greg approach as a sysctl ABI change near a
> > >> libc major bump.  On the other side of such a bump, all kernel + base +
> > >> packages are updated to use the new storage ABI.  We get orderly .h
> > >> files without a confusing glitch, and kern_sysctl.c doesn't need to
> > >> store the value into two fields (32bit and 64bit) for the forseeable
> > >> future.
> > >> 
> > >> Over the years I've arrived at the conclusion that maintaining binary
> > >> compatibility at all costs collects too much confusing damage.  Instead,
> > >> we've built an software ecosystem where ABI changes are expected and
> > >> carry minimal consequence.
> 
> Sadly rust and especially go made a different decision about that.

I don't understand why this matters.

3 libraries are cranking tomorrow.  For binary compatibility, people must
upgrade their packages.

As to the binaries they built by hand?  Shrug!



Re: OpenSSH: RSA/SHA1 disabled by default

2021-09-07 Thread Theo de Raadt
Stuart Henderson  wrote:

> On 2021/09/08 09:03, Damien Miller wrote:
> > This is a case of the host key algorithm not matching, so you
> > should use HostKeyAlgorithms=+ssh-rsa - I'll make sure to mention
> > this in the release notes.
> 
> People seem to really be having a hard time grasping what's being
> disabled by default. And it doesn't help with the confusion that a large
> well-known site doing a lot of SSH traffic for many users are handling
> ssh-rsa deprecation as "old user RSA keys will still work with SHA-1 but
> new ones will need SHA-2" (creating an artificial link between user keys
> and host key algorithm that doesn't exist in the protocol).

Does it matter?  It is going to happen.  Whether people cope now, or
then, does it matter?  Not really.



Re: ksh: add support for bracketed paste mode

2021-09-07 Thread Theo de Raadt
bracketed paste mode is complete garbage which should never have been
invented.  It creates as many problems as it solves.

It is way too easy for a tty to get into the mode, and get stuck.  Yes,
getting stuck happens, especially when programs crash while in the mode.

At that point, the user experience is terrible, and no user knows how to
fix it.

I despise the thing, and will raise objection everytime.

Sören Tempel  wrote:

> Ping.
> 
> I've been using the patched the last ~6 months and didn't encounter any
> problems with it. If there is no interested in bracketed paste mode or
> if the design needs to be revised in general, please let me know. Below
> is a slightly updated version of this patch which should fix the build
> without -DEMACS.
> 
> Greetings,
> Sören
> 
> diff --git edit.c edit.c
> index 3089d195d20..cef7949b88d 100644
> --- edit.c
> +++ edit.c
> @@ -150,12 +150,28 @@ x_puts(const char *s)
>   shf_putc(*s++, shl_out);
>  }
>  
> +#ifdef EMACS
> +static void
> +x_paste_mode(bool onoff)
> +{
> + if (!Flag(FBBRACKETPASTE))
> + return;
> +
> + printf((onoff) ? BRPASTE_INT : BRPASTE_DEINT);
> + fflush(stdout);
> +}
> +#endif
> +
>  bool
>  x_mode(bool onoff)
>  {
>   static bool x_cur_mode;
>   boolprev;
>  
> +#ifdef EMACS
> + x_paste_mode(onoff);
> +#endif
> +
>   if (x_cur_mode == onoff)
>   return x_cur_mode;
>   prev = x_cur_mode;
> diff --git edit.h edit.h
> index 0b604cd64fb..8cc774f01dd 100644
> --- edit.h
> +++ edit.h
> @@ -34,6 +34,12 @@ extern X_chars edchars;
>  #define XCF_FULLPATH BIT(2)  /* command completion: store full path */
>  #define XCF_COMMAND_FILE (XCF_COMMAND|XCF_FILE)
>  
> +/* https://www.xfree86.org/4.7.0/ctlseqs.html#Bracketed%20Paste%20Mode */
> +#define BRPASTE_INT  "\033[?2004h"
> +#define BRPASTE_DEINT"\033[?2004l"
> +#define BRPASTE_PRE  kb_encode("^[[200~")
> +#define BRPASTE_POST kb_encode("^[[201~")
> +
>  /* edit.c */
>  int  x_getc(void);
>  void x_flush(void);
> diff --git emacs.c emacs.c
> index 1a5ff6e9927..f4d369fbc38 100644
> --- emacs.c
> +++ emacs.c
> @@ -118,6 +118,7 @@ staticchar*xmp;   /* mark pointer */
>  static   char*killstack[KILLSIZE];
>  static   int killsp, killtp;
>  static   int x_literal_set;
> +static   int x_brack_paste;
>  static   int x_arg_set;
>  static   char*macro_args;
>  static   int prompt_skip;
> @@ -203,6 +204,8 @@ static intx_fold_lower(int);
>  static int   x_fold_upper(int);
>  static int   x_set_arg(int);
>  static int   x_comment(int);
> +static int   x_brack_paste_start(int);
> +static int   x_brack_paste_end(int);
>  #ifdef DEBUG
>  static int   x_debug_info(int);
>  #endif
> @@ -260,6 +263,8 @@ static const struct x_ftab x_ftab[] = {
>   { x_fold_upper, "upcase-word",  XF_ARG },
>   { x_set_arg,"set-arg",  XF_NOBIND },
>   { x_comment,"comment",  0 },
> + { x_brack_paste_start,  "bracketed-paste-start",0 },
> + { x_brack_paste_end,"bracketed-paste-end",  0 },
>   { 0, 0, 0 },
>  #ifdef DEBUG
>   { x_debug_info, "debug-info",   0 },
> @@ -316,6 +321,8 @@ x_emacs(char *buf, size_t len)
>   }
>  
>   x_literal_set = 0;
> + x_brack_paste = 0;
> +
>   x_arg = -1;
>   x_last_command = NULL;
>   while (1) {
> @@ -353,6 +360,13 @@ x_emacs(char *buf, size_t len)
>   }
>   }
>  
> + /* In bracketed paste mode only allow x_brack_paste_end,
> +  * to quit this mode, for all other commands insert a literal. 
> */
> + if (x_brack_paste && (submatch == 1 && kmatch)) {
> + if (kmatch->ftab->xf_func != x_brack_paste_end)
> + submatch = 0;
> + }
> +
>   if (submatch == 1 && kmatch) {
>   if (kmatch->ftab->xf_func == x_ins_string &&
>   kmatch->args && !macro_args) {
> @@ -1479,6 +1493,10 @@ x_init_emacs(void)
>  
>   TAILQ_INIT();
>  
> + /* bracketed paste mode */
> + kb_add_string(x_brack_paste_start,  NULL, BRPASTE_PRE);
> + kb_add_string(x_brack_paste_end,NULL, BRPASTE_POST);
> +
>   /* man page order */
>   kb_add(x_abort, CTRL('G'), 0);
>   kb_add(x_mv_back,   CTRL('B'), 0);
> @@ -1991,6 +2009,21 @@ x_comment(int c)
>   return KSTD;
>  }
>  
> +int
> +x_brack_paste_start(int c)
> +{
> + if (Flag(FBBRACKETPASTE))
> + x_brack_paste = 1;
> + return KSTD;
> +}
> +
> +int
> +x_brack_paste_end(int c)
> +{
> + if (Flag(FBBRACKETPASTE))
> + x_brack_paste = 0;
> + return KSTD;
> +}
>  
>  /* NAME:
>   *  x_prev_histword - recover word from prev command
> diff --git misc.c misc.c
> index 

Re: Change vm_dsize to vsize_t

2021-09-07 Thread Theo de Raadt
Claudio Jeker  wrote:

> > @@ -443,7 +443,7 @@ struct kinfo_proc {
> >  
> > int32_t p_vm_rssize;/* SEGSZ_T: current resident set size 
> > in pages */
> > int32_t p_vm_tsize; /* SEGSZ_T: text size (pages) */
> > -   int32_t p_vm_dsize; /* SEGSZ_T: data size (pages) */
> > +   u_int64_t   p_vm_dsize; /* VSIZE_T: data size (pages) */
> > int32_t p_vm_ssize; /* SEGSZ_T: stack size (pages) */
> >  
> > int64_t p_uvalid;   /* CHAR: following p_u* members from 
> > struct user are valid */
> 
> From my understanding this is not how struct kinfo_proc should be modified.
> Instead the code should add the u_int64_t version at the end and leave the
> old in place. This way old userland still works with new kernel.

If this is done as a size-change inline as Greg suggested, then it is a
large sysctl ABI bump, with temporary breakage until binaries catch up.

If the 32-bit value is kept as-is, and a 64-bit one is appended, this is
still a small sysctl ABI bump depending on what uses the field.  Existing
binaries will use the 32-bit field for a time.  Such as ps(1).

Obviously we cannot add a new 64-bit field at the end, and mark the
32-bit field "unused", it breaks many utilities in a similar fashion.
A truncated value must be put into the 32-bit field, or it is just as
akward as a large sysctl ABI bump.

Or we could coordinate the Greg approach as a sysctl ABI change near a
libc major bump.  On the other side of such a bump, all kernel + base +
packages are updated to use the new storage ABI.  We get orderly .h
files without a confusing glitch, and kern_sysctl.c doesn't need to
store the value into two fields (32bit and 64bit) for the forseeable
future.

Over the years I've arrived at the conclusion that maintaining binary
compatibility at all costs collects too much confusing damage.  Instead,
we've built an software ecosystem where ABI changes are expected and
carry minimal consequence.



Re: Update to pcap-filter.5/tcpdump.8 (was: update to tcpdump(8))

2021-09-06 Thread Theo de Raadt
I think this is a good enough start, as-is.  A big improvement.

Further iterations can refine the few funny sentences where tcpdump and
pcap-filter diverge.  I'm not worried about them, they just come off
as strange wording, or irrelevancies.

Jason McIntyre  wrote:

> On Sun, Sep 05, 2021 at 04:43:34PM +0200, Denis Fondras wrote:
> > Le Sat, Sep 04, 2021 at 09:57:10PM +0100, Jason McIntyre a ?crit :
> > > the diff looks ok to me. but run any doc changes through "mandoc
> > > -Tlint", and look at any issues your diff may have introduced. in this
> > > case it's just trailing whitespace, but it's super helpful to check your
> > > work.
> > > 
> > 
> > Thank you Jason. There is still a warning in tcpdump.8.
> > 
> > Here is a new version including changes to pcap-filter.5 and tcpdump.8
> > I did not change the examples though as tcpdump examples are broader than
> > filters.
> > 
> 
> hi.
> 
> the warning in tcpdump is fine.
> 
> the diff reads ok to me, but let's wait for a technical ok ;)
> 
> jmc
> 
> > Index: lib/libpcap/pcap-filter.5
> > ===
> > RCS file: /cvs/src/lib/libpcap/pcap-filter.5,v
> > retrieving revision 1.9
> > diff -u -p -r1.9 pcap-filter.5
> > --- lib/libpcap/pcap-filter.5   2 Sep 2021 10:59:13 -   1.9
> > +++ lib/libpcap/pcap-filter.5   5 Sep 2021 13:35:41 -
> > @@ -40,27 +40,31 @@ or
> >  .Pp
> >  The filter expression consists of one or more
> >  .Em primitives .
> > -Primitives usually consist of an ID (name or number)
> > +Primitives usually consist of an
> > +.Ar id
> > +.Pq name or number
> >  preceded by one or more qualifiers.
> >  There are three different kinds of qualifier:
> >  .Bl -tag -width "proto"
> > -.It type
> > -Type qualifiers say what kind of thing the ID name or number refers to.
> > +.It Ar type
> > +Specify which kind of address component the
> > +.Ar id
> > +name or number refers to.
> >  Possible types are
> >  .Cm host ,
> > -.Cm net ,
> > +.Cm net
> >  and
> >  .Cm port .
> > -For example,
> > +E.g.,
> >  .Dq host foo ,
> >  .Dq net 128.3 ,
> > -and
> >  .Dq port 20 .
> >  If there is no type qualifier,
> >  .Cm host
> >  is assumed.
> > -.It dir
> > -Dir qualifiers specify a particular transfer direction to and/or from an 
> > ID.
> > +.It Ar dir
> > +Specify a particular transfer direction to and/or from
> > +.Ar id .
> >  Possible directions are
> >  .Cm src ,
> >  .Cm dst ,
> > @@ -73,11 +77,13 @@ Possible directions are
> >  .Cm addr3 ,
> >  and
> >  .Cm addr4 .
> > -For example,
> > -.Cm src foo ,
> > -.Cm dst net 128.3 ,
> > -.Cm src or dst port ftp-data .
> > -If there is no dir qualifier,
> > +E.g.,
> > +.Dq src foo ,
> > +.Dq dst net 128.3 ,
> > +.Dq src or dst port ftp-data .
> > +If there is no
> > +.Ar dir
> > +qualifier,
> >  .Cm src or dst
> >  is assumed.
> >  The
> > @@ -89,57 +95,85 @@ The
> >  and
> >  .Cm addr4
> >  qualifiers are only valid for IEEE 802.11 Wireless LAN link layers.
> > -For some link layers, such as SLIP and the "cooked" Linux capture mode
> > -used for the "any" device and for some other device types, the
> > +For null link layers (i.e., point-to-point protocols such as SLIP
> > +.Pq Serial Line Internet Protocol
> > +or the
> > +.Xr pflog 4
> > +header), the
> >  .Cm inbound
> >  and
> >  .Cm outbound
> >  qualifiers can be used to specify a desired direction.
> > -.It proto
> > -Proto qualifiers restrict the match to a particular protocol.
> > -Possible
> > -protos are:
> > +.It Ar proto
> > +Restrict the match to a particular protocol.
> > +Possible protocols are:
> > +.Cm ah ,
> > +.Cm arp ,
> > +.Cm atalk ,
> > +.Cm decnet ,
> > +.Cm esp ,
> >  .Cm ether ,
> >  .Cm fddi ,
> > -.Cm tr ,
> > -.Cm wlan ,
> > +.Cm icmp ,
> > +.Cm icmp6 ,
> > +.Cm igmp ,
> > +.Cm igrp ,
> >  .Cm ip ,
> >  .Cm ip6 ,
> > -.Cm arp ,
> > +.Cm lat ,
> > +.Cm mopdl ,
> > +.Cm moprc ,
> > +.Cm pim ,
> >  .Cm rarp ,
> > -.Cm decnet ,
> > +.Cm sca ,
> > +.Cm stp ,
> >  .Cm tcp ,
> > +.Cm udp ,
> >  and
> > -.Cm udp .
> > -For example,
> > +.Cm wlan .
> > +E.g.,
> >  .Dq ether src foo ,
> >  .Dq arp net 128.3 ,
> >  .Dq tcp port 21 ,
> >  and
> >  .Dq wlan addr2 0:2:3:4:5:6 .
> > -If there is no proto qualifier,
> > +If there is no protocol qualifier,
> >  all protocols consistent with the type are assumed.
> > -For example,
> > +E.g.,
> >  .Dq src foo
> >  means
> > -.Dq (ip or arp or rarp) src foo
> > -(except the latter is not legal syntax);
> > +.Do
> > +.Pq ip or arp or rarp
> > +src foo
> > +.Dc
> > +.Pq except the latter is not legal syntax ;
> >  .Dq net bar
> >  means
> > -.Dq (ip or arp or rarp) net bar ;
> > +.Do
> > +.Pq ip or arp or rarp
> > +net bar
> > +.Dc ;
> >  and
> >  .Dq port 53
> >  means
> > -.Dq (tcp or udp) port 53 .
> > +.Do
> > +.Pq TCP or UDP
> > +port 53
> > +.Dc .
> >  .Pp
> >  .Cm fddi
> >  is actually an alias for
> >  .Cm ether ;
> >  the parser treats them identically as meaning
> > -"the data link level used on the specified 

Re: rm.1: remove extraneous word

2021-09-03 Thread Theo de Raadt
Unix has these things called hard links.

As such, rm deletes a directory entry pointing to an inode which stores
a file, but other directory entries could point at the same file.

Introducing people to this vaguely is nice, so I thikn this should
keep saying 'directory entries'.


>On Thu, Sep 02, 2021 at 11:10:54PM +0100, Jason McIntyre wrote:
>> On Thu, Sep 02, 2021 at 02:28:54PM -0700, Evan Silberman wrote:
>> > Speaking of the first sentence of rm(1):
>> > 
>> > Remove extraneous word from command description
>> > 
>> > "non-directory files" reads more naturally and means the same thing as
>> > "non-directory type files".
>> > 
>> 
>> true.
>> 
>> i wonder if it was originally an attempt to not quote posix
>> (or posix attempting to not quote bsd). posix refers to removing
>> "directory entries", which seems more natural.
>> 
>> regardless, rm can remove both directory entries/non-directory type
>> files as well as directories. although by default it does not remove
>> directories, i wonder if we could just say:
>> 
>>  The
>>  .Nm
>>  utility
>>  attempts to remove any files specified on the command line.
>> 
>> and NAME could be:
>> 
>>  - rm - remove directory entries
>>  + rm - remove files
>> 
>> but maybe that is unixical heresy?
>> 
>> jmc
>> 
>
>i cannot really make up my mind here. posix and other bsds all use
>"remove directory entries" for NAME. i worry that my proposal would be
>needless change, and a lessening of valid terminology. so i probably
>reject my own proposal.
>
>on the other hand, the phrase "non-directory type files" is pretty
>awful. posix is clearer i think, sticking to "directory entries
>specified by each file argument".we could also use this: "directory
>entries specified on the command line". but that would feel like
>deliberately avoiding the term "file", which is clear and simple.
>
>just using "non-directory files" is also weird. i mean, you can very
>much remove directory files.
>
>jmc
>
>> > diff --git a/bin/rm/rm.1 b/bin/rm/rm.1
>> > index a2526a36392..1be2bf31913 100644
>> > --- a/bin/rm/rm.1
>> > +++ b/bin/rm/rm.1
>> > @@ -46,7 +46,7 @@
>> >  .Sh DESCRIPTION
>> >  The
>> >  .Nm
>> > -utility attempts to remove the non-directory type files specified on the
>> > +utility attempts to remove the non-directory files specified on the
>> >  command line.
>> >  If the permissions of the file do not permit writing, and the standard
>> >  input device is a terminal, the user is prompted (on the standard error
>> > 
>
>



Re: update to tcpdump(8)

2021-09-02 Thread Theo de Raadt
I think the following approach will work.

1. changes from tcpdump.8 -r1.00 to -rHEAD need merging into pcap-filter.5

2) DESCRIPTION from pcap-filter body is then copied into tcpdump, replacing the 
same
   chunk.  What I see is far more authoritative.

3) EXAMPLES copied from pcap-filter, and needs "# tcpdump" prefixing on most 
lines

4) OUTPUT FORMAT from tcpdump, remains.

then future work is:

a. maintain DESCRIPTION 1:1
b. maintain EXAMPLES 1:1, but with '# tcpdump' prefixing

It does not seem that bad.



Re: enable cu(4) in amd64/GENERIC by default

2021-09-02 Thread Theo de Raadt
lacking full cables is not a driver problem, and users can learn to cope
with that.

ok deraadt

Jan Klemkow  wrote:

> Hi,
> 
> The card and cables don't have the signaling lines, that getty to use it
> as com(4).  But with "local" in ttys(5) it works.  I have this driver in
> productive use for about 5 years now.
> 
> OK?
> 
> bye,
> Jan
> 
> Index: arch/amd64/conf/GENERIC
> ===
> RCS file: /cvs/src/sys/arch/amd64/conf/GENERIC,v
> retrieving revision 1.499
> diff -u -p -r1.499 GENERIC
> --- arch/amd64/conf/GENERIC   20 Aug 2021 05:23:18 -  1.499
> +++ arch/amd64/conf/GENERIC   2 Sep 2021 08:49:22 -
> @@ -403,7 +403,7 @@ com*  at pcmcia?  # PCMCIA 
> modems/serial
>  com* at puc?
>  
>  # options CY_HW_RTS
> -#cy* at pci? # PCI cyclom serial card
> +cy*  at pci? # PCI cyclom serial card
>  #cz* at pci? # Cyclades-Z multi-port serial boards
>  
>  lpt0 at isa? port 0x378 irq 7# standard PC parallel ports
> 



Re: timeout: Prettify man page and usage

2021-09-02 Thread Theo de Raadt
Jason McIntyre  wrote:

> On Thu, Sep 02, 2021 at 08:56:29AM +, Job Snijders wrote:
> > On Thu, Sep 02, 2021 at 07:23:26AM +0100, Jason McIntyre wrote:
> > > >  .Ar time
> > > > -can be integer or decimal numbers.
> > > > +are positive integer or real (decimal) numbers, with an optional
> > > 
> > > can you have a negative timeout?
> > 
> > Negative values are not permitted
> > 
> > $ timeout -- -1 /bin/ls
> > timeout: invalid duration: Undefined error: 0
> > 
> > Kind regards,
> > 
> > Job
> > 
> 
> sorry, i wasn;t totally clear: i understand that negative values are not
> permitted. but do we have to say it? i mean, you couldn;t expect to run
> a command with a negative timeout, right?
> 
> the whole phrasing about what the timeout is is very precise, but hard
> to understand. i want to simplify it. so if no one could logically
> expect a negative timeout to work, why say it?
> 
> i.e. the current man page does not say it. so i think the proposed
> change is not good.

i agree.  it is obvious that a timeout, being a duration, is positive, and
does not need to be described.



Re: update to tcpdump(8)

2021-09-02 Thread Theo de Raadt
Jason McIntyre  wrote:

> On Thu, Sep 02, 2021 at 12:44:56AM -0600, Theo de Raadt wrote:
> > Jason McIntyre  wrote:
> > 
> > > then i guess i would propose doing exactly that: removing the bulk of
> > > the text describing primitives and qualifiers and leave a pointer to
> > > pcap-filter.3. we could leave a brief description of the main
> > > qualifiers, and perhaps just a list of valid keywords for the other
> > > primitives.
> > 
> > I think that will be very annoying.
> > 
> > When I want to use tcpdump, I don't want to go reading a manual page
> > which isn't called tcpdump.  Why should people need to jump another step.
> > 
> > Why should the tcpdump option become just a stub about the getopt options.
> > 
> > I believe most people run 'man tcpdump' not to read about the getopt 
> > options,
> > but to remind themselves of the pcap grammar they are about to use.  Making
> > them run the man command twice is inserting a distraction into the process.
> > 
> > I recognize the tcpdump manual page doesn't describe the full grammer, but
> > it is still useful as-is.
> > 
> 
> why not just paste in the body of pcap-filter in then and we can try and
> keep them in sync thereafter?

that might work



Re: update to tcpdump(8)

2021-09-02 Thread Theo de Raadt
Jason McIntyre  wrote:

> then i guess i would propose doing exactly that: removing the bulk of
> the text describing primitives and qualifiers and leave a pointer to
> pcap-filter.3. we could leave a brief description of the main
> qualifiers, and perhaps just a list of valid keywords for the other
> primitives.

I think that will be very annoying.

When I want to use tcpdump, I don't want to go reading a manual page
which isn't called tcpdump.  Why should people need to jump another step.

Why should the tcpdump option become just a stub about the getopt options.

I believe most people run 'man tcpdump' not to read about the getopt options,
but to remind themselves of the pcap grammar they are about to use.  Making
them run the man command twice is inserting a distraction into the process.

I recognize the tcpdump manual page doesn't describe the full grammer, but
it is still useful as-is.



Re: Import timeout(1) from NetBSD

2021-09-01 Thread Theo de Raadt
It needs pledge.

> +#include 

This is wrong, it should be 

> +#include 
> +#include 

> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define EXIT_TIMEOUT 124
> +
> +static sig_atomic_t sig_chld = 0;
> +static sig_atomic_t sig_term = 0;
> +static sig_atomic_t sig_alrm = 0;
> +static sig_atomic_t sig_ign = 0;
> +
> +static void __dead
> +usage(void)
> +{
> + fprintf(stderr, "Usage: %s [--signal sig | -s sig] [--preserve-status]"

lower case usage:

> + " [--kill-after time | -k time] [--foreground]  "
> + " \n", getprogname());

This should not use <>, ingo and jmc are going to scream at you.

setprogname/getprogname use is excessive, this could simply be the text 
"timeout"

> + errno = 0;
> + sig = strtol(str, , 10);
> +
> + if (str[0] == '\0' || *ep != '\0')
> + goto err;
> + if (errno == ERANGE && (sig == LONG_MAX || sig == LONG_MIN))
> + goto err;

strtonum will simplify this.

> + return;
> + }
> +
> + switch(signo) {
> + case 0:

I don't understand what the 0 does here.

> + case SIGINT:
> + case SIGHUP:
> + case SIGQUIT:
> + case SIGTERM:
> + sig_term = signo;
> + break;
> + case SIGCHLD:
> + sig_chld = 1;
> + break;
> + case SIGALRM:
> + sig_alrm = 1;
> + break;
> + }
> +}
> +main(int argc, char **argv)
> +{
> + int ch;
> + unsigned long   i;
> + int foreground, preserve;
> + int error, pstat, status;
> + int killsig = SIGTERM;
> + pid_t   pgid, pid, cpid;
> + double  first_kill;
> + double  second_kill;
> + booltimedout = false;
> + booldo_second_kill = false;
> + struct  sigaction signals;
> + int signums[] = {
> + -1,
> + SIGTERM,
> + SIGINT,
> + SIGHUP,
> + SIGCHLD,
> + SIGALRM,
> + SIGQUIT,
> + };

Weird indent.

> +
> + setprogname(argv[0]);

setprogname discussion at the top

> +
> + foreground = preserve = 0;
> + second_kill = 0;
> + cpid = -1;
> + pgid = -1;

This defines variables *after* code:

> +
> + const struct option longopts[] = {
> + { "preserve-status", no_argument,   ,1 },
> + { "foreground",  no_argument,   ,  1 },
> + { "kill-after",  required_argument, NULL,'k'},
> + { "signal",  required_argument, NULL,'s'},
> + { "help",no_argument,   NULL,'h'},
> + { NULL,  0, NULL, 0 }
> + };

can likely do pledge("stdio proc exec") around here.

> +
> + while ((ch = getopt_long(argc, argv, "+k:s:h", longopts, NULL)) != -1) {
> + switch (ch) {
> + case 'k':
> + do_second_kill = true;
> + second_kill = parse_duration(optarg);
> + break;
> + case 's':
> + killsig = parse_signal(optarg);
> + break;
> + case 0:
> + break;
> + case 'h':

'h' is pointless.

> + default:
> + usage();
> + break;
> + }
> + }
> +
> + argc -= optind;
> + argv += optind;
> +
> + if (argc < 2)
> + usage();
> +
> + first_kill = parse_duration(argv[0]);
> + argc--;
> + argv++;
> +
> + if (!foreground) {
> + pgid = setpgid(0,0);
> +
> + if (pgid == -1)
> + err(EX_OSERR, "setpgid()");
> + }
> +
> + memset(, 0, sizeof(signals));
> + sigemptyset(_mask);
> +
> + if (killsig != SIGKILL && killsig != SIGSTOP)
> + signums[0] = killsig;
> +
> + for (i = 0; i < sizeof(signums) / sizeof(signums[0]); i ++)
> + sigaddset(_mask, signums[i]);
> +
> + signals.sa_handler = sig_handler;
> + signals.sa_flags = SA_RESTART;
> +
> + for (i = 0; i < sizeof(signums) / sizeof(signums[0]); i ++) {
> + if (signums[i] != -1 && signums[i] != 0 &&
> + sigaction(signums[i], , NULL) == -1)
> + err(EX_OSERR, "sigaction()");
> + }
> +
> + signal(SIGTTIN, SIG_IGN);
> + signal(SIGTTOU, SIG_IGN);
> +
> + pid = fork();
> + if (pid == -1)
> + err(EX_OSERR, "fork()");
> + else if (pid == 0) {
> + /* child process */
> + signal(SIGTTIN, SIG_DFL);
> + signal(SIGTTOU, SIG_DFL);
> +
> + error = execvp(argv[0], argv);
> + if (error == -1)
> + err(EX_UNAVAILABLE, "exec()");
> + }

Re: rpki-client exclude files from rsync fetch

2021-09-01 Thread Theo de Raadt
Job Snijders  wrote:

> On Wed, Sep 01, 2021 at 11:14:15AM +0200, Claudio Jeker wrote:
> > On Tue, Aug 31, 2021 at 02:23:57PM +0200, Claudio Jeker wrote:
> > > RPKI repository can only include a few specific files, everything else is
> > > just ignored and deleted after every fetch.  Since openrsync supports
> > > --exclude-file now we can use this to limit what is actually accepted by
> > > the client.
> > > 
> > > I used a config file in /etc/rpki instead of using multiple --exclude /
> > > --include arguments. Mostly to keep the execvp argv short.
> > > 
> > > What you think?
> > 
> > It seems using a config file to keep the argv list short is too
> > controversial and all alternate suggestions are worse.
> > So just add the include/exclude list as arguments.
> 
> Looks good.
> 
> $ ps axuwww | fgrep rsync
> _rpki-cl 85084  0.0  0.0   816  1500 p4  S+pU   10:51AM0:00.01 
> rpki-client: rsync (rpki-client)
> _rpki-cl  5288  0.8  0.1 16228 17576 p4  R+pU/1 10:52AM0:06.85 openrsync 
> -rt --no-motd --timeout=180 --include=* --include=*.cer --include=*.crl 
> --include=*.gbr --include=*.mft --include=*.roa --exclude=* 
> rsync://rpki.arin.net/repository rsync/rpki.arin.net/repository

w



Re: async traceroute(8)

2021-09-01 Thread Theo de Raadt
Stuart Henderson  wrote:

> On 2021/09/01 11:25, Florian Obser wrote:
> > So traceroute sends one probe, waits upto 5^W3 seconds for an answer to
> > arrive, sends the next probe and so on.
> > 
> > This makes it a bit faster (10x on a path with two intermediate systems
> > not answering) by sending probes, waiting for the answer and doing
> > reverse DNS lookups async.
> 
> Basics are working here with icmp and udp. -A doesn't work reliably,
> try it with an empty cache - I think it only prints if the route lookup
> had a reply before the rdns lookup.

interesting, this will need to be found and fixed.

> I don't know libevent, is it possible to reset the timer and skip the
> 30ms delay if a response is received? Currently it's a little slower
> for the case where all hops respond.

that is an interesting idea also, it can be even faster.  claudio/florian
and i have talked a lot about trying to send back-to-back packets to a
single ttl target, because it may trigger control plane protection filters
in those intermediate hops.

but increasing the pace to other targets, is fine.

> Hosts that don't respond result in a lot of *'s printed quickly, maybe
> scrolling the useful output off the terminal. The overall output is the
> same as the sequential version if you actually leave it to finish but
> most users would have hit ^C by then. I don't know what could be done
> to improve it (suppressing multiple * * * lines might help?) but user
> experience with that isn't great as-is. Try mp3.com for an example.

there are two proposals for this, which would diverge the output:

   15  * * * [repeating to ttl 64]

or

   15  * * *
   [repeating to ttl 64]

how often do people run scripts against traceroute output, which will care
about this change in format.

I also considered a third option -- to meter the * * * printout lines
rate to 3/second, but this kind of conflicts with the goal for speeding
things up.




Re: diff(1)ing hardlinks

2021-09-01 Thread Theo de Raadt
shocking this test wasn't already in place.

Alexander Hall  wrote:

> If two files to be compared share the same inode, it should
> be reasonable to consider them identical.
> 
> This gives a substantial speedup when comparing directory
> structures with many hardlinked files, e.g. when using
> rsnapshot for incremental backup.
> 
> Comments? OK?
> 
> /Alexander
> 
> Index: diffreg.c
> ===
> RCS file: /cvs/src/usr.bin/diff/diffreg.c,v
> retrieving revision 1.93
> diff -u -p -r1.93 diffreg.c
> --- diffreg.c 28 Jun 2019 13:35:00 -  1.93
> +++ diffreg.c 31 Aug 2021 23:07:51 -
> @@ -429,6 +429,10 @@ files_differ(FILE *f1, FILE *f2, int fla
>   if ((flags & (D_EMPTY1|D_EMPTY2)) || stb1.st_size != stb2.st_size ||
>   (stb1.st_mode & S_IFMT) != (stb2.st_mode & S_IFMT))
>   return (1);
> +
> + if (stb1.st_dev == stb2.st_dev && stb1.st_ino == stb2.st_ino)
> + return (0);
> +
>   for (;;) {
>   i = fread(buf1, 1, sizeof(buf1), f1);
>   j = fread(buf2, 1, sizeof(buf2), f2);
> 



Re: rpki-client exclude files from rsync fetch

2021-08-31 Thread Theo de Raadt
I don't understand -- why would people edit this file?

If this list is in argv, it will be difficult to identify targets using
ps, because the hostname is way at the end.

Job Snijders  wrote:

> Hi,
> 
> I don't think this should be user configurable.
> 
> If folks remove entries like "+ *.crl" it breaks things.
> If folks add entries like "+ *.mp3" it wastes network bandwidth. :-)
> 
> Let's use "--include" and "--exclude" instead.
> 
> kind regards,
> 
> Job
> 
> On Tue, Aug 31, 2021 at 02:23:57PM +0200, Claudio Jeker wrote:
> > RPKI repository can only include a few specific files, everything else is
> > just ignored and deleted after every fetch.  Since openrsync supports
> > --exclude-file now we can use this to limit what is actually accepted by
> > the client.
> > 
> > I used a config file in /etc/rpki instead of using multiple --exclude /
> > --include arguments. Mostly to keep the execvp argv short.
> > 
> > What you think?
> > -- 
> > :wq Claudio
> > 
> > Index: etc/Makefile
> > ===
> > RCS file: /cvs/src/etc/Makefile,v
> > retrieving revision 1.484
> > diff -u -p -r1.484 Makefile
> > --- etc/Makefile1 May 2021 16:11:07 -   1.484
> > +++ etc/Makefile31 Aug 2021 12:17:40 -
> > @@ -156,7 +156,7 @@ distribution-etc-root-var: distrib-dirs
> > ${DESTDIR}/etc/ppp
> > cd rpki; \
> > ${INSTALL} -c -o root -g wheel -m 644 \
> > -   afrinic.tal apnic.tal lacnic.tal ripe.tal \
> > +   afrinic.tal apnic.tal lacnic.tal ripe.tal rsync.filter \
> > ${DESTDIR}/etc/rpki
> > cd examples; \
> > ${INSTALL} -c -o root -g wheel -m 644 ${EXAMPLES} \
> > Index: etc/rpki/rsync.filter
> > ===
> > RCS file: etc/rpki/rsync.filter
> > diff -N etc/rpki/rsync.filter
> > --- /dev/null   1 Jan 1970 00:00:00 -
> > +++ etc/rpki/rsync.filter   31 Aug 2021 12:09:24 -
> > @@ -0,0 +1,7 @@
> > ++ */
> > ++ *.cer
> > ++ *.crl
> > ++ *.gbr
> > ++ *.mft
> > ++ *.roa
> > +- *
> > Index: usr.sbin/rpki-client/rsync.c
> > ===
> > RCS file: /cvs/src/usr.sbin/rpki-client/rsync.c,v
> > retrieving revision 1.24
> > diff -u -p -r1.24 rsync.c
> > --- usr.sbin/rpki-client/rsync.c19 Apr 2021 17:04:35 -  1.24
> > +++ usr.sbin/rpki-client/rsync.c31 Aug 2021 12:17:11 -
> > @@ -279,6 +279,8 @@ proc_rsync(char *prog, char *bind_addr, 
> > args[i++] = "--no-motd";
> > args[i++] = "--timeout";
> > args[i++] = "180";
> > +   args[i++] = "--exclude-from";
> > +   args[i++] = "/etc/rpki/rsync.filter";
> > if (bind_addr != NULL) {
> > args[i++] = "--address";
> > args[i++] = (char *)bind_addr;
> > 
> 



Re: iked(8): client-side DNS support via resolvd(8)

2021-08-31 Thread Theo de Raadt
+   rtm.rtm_priority = RTP_PROPOSAL_STATIC;

So my gut reaction is we should have

/usr/include/net/route.h:#define RTP_PROPOSAL_TEMPORARY62

I hesitate calling this "VPN", or "road warrior", or making it specific to
certain types of proposal offering daemons...



Re: [External] : better use the tokeniser in the pfctl parser

2021-08-31 Thread Theo de Raadt
I am really against the idea of the parser inspecting a static buffer
from the lex.

Also we have a ton of these parsers, and discourage them from deviating.

This tiny little "please use the right keyword" change feels so minor; we
do not have a generic error-correction-proposing parser, 99% of plausible
errors emit "syntax error".

The only reason "no" is a TOKEN is because of previous use in other
parts of the grammer, whereas "yes" does not occur in other places in
the grammer.  Let's keep this simple.

David Gwynne  wrote:

> On Tue, Aug 31, 2021 at 07:33:40AM +0200, Alexandr Nedvedicky wrote:
> > Hello,
> > 
> > On Tue, Aug 31, 2021 at 02:40:57PM +1000, David Gwynne wrote:
> > > handling the "no" option with a token, and "yes" via a string made my
> > > eye twitch.
> > > 
> > > ok? or is the helpful yyerror a nice feature?
> > > 
> > 
> > I actually think it's a nice feature. below is output
> > for current pfctl we have in tree:
> 
> it is nice, but the implementation isn't... rigorous.
> 
> > 
> > lumpy$ pfctl -n -f /tmp/pf.conf
> > /tmp/pf.conf:6: invalid value 'nope', expected 'yes' or 'no'
> > 
> > and output with diff applied:
> > 
> > lumpy$ ./pfctl -n -f /tmp/pf.conf
> > /tmp/pf.conf:6: syntax error
> 
> but if you try to use a keyword instead of a string, you get this:
> 
> dlg@kbuild ~$ echo "set reassemble yes" | pfctl -vnf -
> set reassemble yes 
> dlg@kbuild ~$ echo "set reassemble no" | pfctl -vnf -  
> set reassemble no 
> dlg@kbuild ~$ echo "set reassemble nope" | pfctl -vnf -
> stdin:1: invalid value 'nope', expected 'yes' or 'no'
> dlg@kbuild ~$ echo "set reassemble block" | pfctl -vnf -
> stdin:1: syntax error
> dlg@kbuild ~$ 
> 
> if the tokeniser exposed the buffer it was working on, we could make it
> consistent for all arguments:
> 
> dlg@kbuild pfctl$ echo "set reassemble yes" | ./obj/pfctl -vnf -   
> set reassemble yes 
> dlg@kbuild pfctl$ echo "set reassemble no" | ./obj/pfctl -vnf -  
> set reassemble no 
> dlg@kbuild pfctl$ echo "set reassemble nope" | ./obj/pfctl -vnf -
> stdin:1: syntax error
> stdin:1: invalid value 'nope', expected 'yes' or 'no'
> dlg@kbuild pfctl$ echo "set reassemble block" | ./obj/pfctl -vnf -
> stdin:1: syntax error
> stdin:1: invalid value 'block', expected 'yes' or 'no'
> 
> the extremely rough PoC diff for pfctl that implements this is
> below. because the tokeniser handles some operators without using the
> buffer, if you give "set reassemble" an operator then you get confusing
> output:
> 
> dlg@kbuild pfctl$ echo "set reassemble <" | ./obj/pfctl -vnf - 
> stdin:1: syntax error
> stdin:1: invalid value 'reassemble', expected 'yes' or 'no'
> 
> anyway, it might be easier to drop the diff for now.
> 
> Index: parse.y
> ===
> RCS file: /cvs/src/sbin/pfctl/parse.y,v
> retrieving revision 1.709
> diff -u -p -r1.709 parse.y
> --- parse.y   1 Feb 2021 00:31:04 -   1.709
> +++ parse.y   31 Aug 2021 09:20:38 -
> @@ -458,6 +458,8 @@ typedef struct {
>   int lineno;
>  } YYSTYPE;
>  
> +static u_char *yytext;
> +
>  #define PPORT_RANGE  1
>  #define PPORT_STAR   2
>  int  parseport(char *, struct range *r, int);
> @@ -471,7 +473,7 @@ int   parseport(char *, struct range *r, i
>  %token   PASS BLOCK MATCH SCRUB RETURN IN OS OUT LOG QUICK ON FROM TO 
> FLAGS
>  %token   RETURNRST RETURNICMP RETURNICMP6 PROTO INET INET6 ALL ANY 
> ICMPTYPE
>  %token   ICMP6TYPE CODE KEEP MODULATE STATE PORT BINATTO NODF
> -%token   MINTTL ERROR ALLOWOPTS FILENAME ROUTETO DUPTO REPLYTO NO LABEL
> +%token   MINTTL ERROR ALLOWOPTS FILENAME ROUTETO DUPTO REPLYTO YES NO 
> LABEL
>  %token   NOROUTE URPFFAILED FRAGMENT USER GROUP MAXMSS MAXIMUM TTL TOS 
> DROP TABLE
>  %token   REASSEMBLE ANCHOR SYNCOOKIES
>  %token   SET OPTIMIZATION TIMEOUT LIMIT LOGINTERFACE BLOCKPOLICY RANDOMID
> @@ -3754,16 +3756,11 @@ comma : ','
>   ;
>  
>  yesno: NO{ $$ = 0; }
> - | STRING{
> - if (!strcmp($1, "yes"))
> - $$ = 1;
> - else {
> - yyerror("invalid value '%s', expected 'yes' "
> - "or 'no'", $1);
> - free($1);
> - YYERROR;
> - }
> - free($1);
> + | YES   { $$ = 1; }
> + | error {
> + yyerror("invalid value '%s', expected 'yes' or 'no'",
> + yytext);
> + YYABORT;
>   }
>   ;
>  
> @@ -5048,6 +5045,7 @@ lookup(char *s)
>   { "urpf-failed",URPFFAILED},
>   { "user",   USER},
>   { "weight", WEIGHT},
> + { "yes",   

Re: iked(8): client-side DNS support via resolvd(8)

2021-08-31 Thread Theo de Raadt
This diff doesn't set rtm_index (to identify the interface the dns
proposal is associated with)

I guess that means rtm_index is 0.

Inside resolvd, the proposal rtm_index is used to track proposals in the
learned[] array.

resolvd uses if_indextoname() to annotate the interface name on these
dynamic lines with "# resolvd: em0".  Using 0 means if_indextoname() will
fail, and it will write "# resolvd: ", I guess?

If multiple agents start offering proposals with the same rtm_index +
family, things get a little weird.

What would isakmpd or some other vpn layer do if they want to start
doing the same as this iked diff -- would they also submit rtm_index 0?
The code cannot disambiguate/sort/select from the proposals with the same
id#, so all such offers will act as a single proposal, and I think whoever
submits the latest will win the fight.  That might behave weirdly.

So, does the route message need an extension to indicate "who" is making
the offer, or should iked (ando other vpns) have unique RTP_PROPOSAL_*
identifiers, and then we can have resolvd mix that into the sort also?



Re: iked(8): client-side DNS support via resolvd(8)

2021-08-31 Thread Theo de Raadt
Very interesting.

Please be very careful that proposal withdrawal actually works, or
the experience will be poor.



Re: Kill SYSCALL_DEBUG

2021-08-30 Thread Theo de Raadt
Hang on, SYSCALL_DEBUG is used to bring up new architectures.
That is the only time the #define is enabled.

When you are bringing up a new architecture, bt is useless

I don't think this makes sense.

Martin Pieuchot  wrote:

> Now that dt(4) and btrace(8) are enabled by default and provide a nice
> and flexible way to debug syscalls on GENERIC kernels should we get rid
> of the SYSCALL_DEBUG mechanism?
> 
> Note that the auto-generated kern/syscalls.c providing the `syscallnames'
> array is still needed to build btrace(8).
> 
> ok?
> 
> Index: kern/exec_elf.c
> ===
> RCS file: /cvs/src/sys/kern/exec_elf.c,v
> retrieving revision 1.160
> diff -u -p -r1.160 exec_elf.c
> --- kern/exec_elf.c   10 Mar 2021 10:21:47 -  1.160
> +++ kern/exec_elf.c   30 Aug 2021 07:19:33 -
> @@ -107,9 +107,6 @@ int   elf_os_pt_note_name(Elf_Note *);
>  int  elf_os_pt_note(struct proc *, struct exec_package *, Elf_Ehdr *, int *);
>  
>  extern char sigcode[], esigcode[], sigcoderet[];
> -#ifdef SYSCALL_DEBUG
> -extern char *syscallnames[];
> -#endif
>  
>  /* round up and down to page boundaries. */
>  #define ELF_ROUND(a, b)  (((a) + (b) - 1) & ~((b) - 1))
> @@ -135,11 +132,7 @@ struct emul emul_elf = {
>   SYS_syscall,
>   SYS_MAXSYSCALL,
>   sysent,
> -#ifdef SYSCALL_DEBUG
> - syscallnames,
> -#else
>   NULL,
> -#endif
>   (sizeof(AuxInfo) * ELF_AUX_ENTRIES / sizeof(char *)),
>   elf_copyargs,
>   setregs,
> Index: kern/kern_xxx.c
> ===
> RCS file: /cvs/src/sys/kern/kern_xxx.c,v
> retrieving revision 1.36
> diff -u -p -r1.36 kern_xxx.c
> --- kern/kern_xxx.c   2 Apr 2019 11:00:22 -   1.36
> +++ kern/kern_xxx.c   30 Aug 2021 07:19:17 -
> @@ -84,75 +84,3 @@ __stack_smash_handler(char func[], int d
>   panic("smashed stack in %s", func);
>  }
>  #endif
> -
> -#ifdef SYSCALL_DEBUG
> -#include 
> -
> -#define  SCDEBUG_CALLS   0x0001  /* show calls */
> -#define  SCDEBUG_RETURNS 0x0002  /* show returns */
> -#define  SCDEBUG_ALL 0x0004  /* even syscalls that are 
> implemented */
> -#define  SCDEBUG_SHOWARGS0x0008  /* show arguments to calls */
> -
> -int  scdebug = SCDEBUG_CALLS|SCDEBUG_RETURNS|SCDEBUG_SHOWARGS;
> -
> -void
> -scdebug_call(struct proc *p, register_t code, const register_t args[])
> -{
> - struct process *pr;
> - struct sysent *sy;
> - struct emul *em;
> - int i;
> -
> - if (!(scdebug & SCDEBUG_CALLS))
> - return;
> -
> - pr = p->p_p;
> - em = pr->ps_emul;
> - sy = >e_sysent[code];
> - if (!(scdebug & SCDEBUG_ALL || code < 0 || code >= em->e_nsysent ||
> -  sy->sy_call == sys_nosys))
> - return;
> -
> - printf("proc %d (%s): %s num ", pr->ps_pid, pr->ps_comm, em->e_name);
> - if (code < 0 || code >= em->e_nsysent)
> - printf("OUT OF RANGE (%ld)", code);
> - else {
> - printf("%ld call: %s", code, em->e_syscallnames[code]);
> - if (scdebug & SCDEBUG_SHOWARGS) {
> - printf("(");
> - for (i = 0; i < sy->sy_argsize / sizeof(register_t);
> - i++)
> - printf("%s0x%lx", i == 0 ? "" : ", ", args[i]);
> - printf(")");
> - }
> - }
> - printf("\n");
> -}
> -
> -void
> -scdebug_ret(struct proc *p, register_t code, int error,
> -const register_t retval[])
> -{
> - struct process *pr;
> - struct sysent *sy;
> - struct emul *em;
> -
> - if (!(scdebug & SCDEBUG_RETURNS))
> - return;
> -
> - pr = p->p_p;
> - em = pr->ps_emul;
> - sy = >e_sysent[code];
> - if (!(scdebug & SCDEBUG_ALL || code < 0 || code >= em->e_nsysent ||
> - sy->sy_call == sys_nosys))
> - return;
> - 
> - printf("proc %d (%s): %s num ", pr->ps_pid, pr->ps_comm, em->e_name);
> - if (code < 0 || code >= em->e_nsysent)
> - printf("OUT OF RANGE (%ld)", code);
> - else
> - printf("%ld ret: err = %d, rv = 0x%lx,0x%lx", code,
> - error, retval[0], retval[1]);
> - printf("\n");
> -}
> -#endif /* SYSCALL_DEBUG */
> Index: kern/init_main.c
> ===
> RCS file: /cvs/src/sys/kern/init_main.c,v
> retrieving revision 1.308
> diff -u -p -r1.308 init_main.c
> --- kern/init_main.c  30 Jun 2021 12:21:02 -  1.308
> +++ kern/init_main.c  30 Aug 2021 07:17:55 -
> @@ -155,9 +155,6 @@ void  pool_gc_pages(void *);
>  void percpu_init(void);
>  
>  extern char sigcode[], esigcode[], sigcoderet[];
> -#ifdef SYSCALL_DEBUG
> -extern char *syscallnames[];
> -#endif
>  
>  struct emul emul_native = {
>   "native",
> @@ -165,11 +162,7 @@ struct emul emul_native = {
>   SYS_syscall,
>  

Re: Atomic signal flags for vi(1)

2021-08-29 Thread Theo de Raadt
> *If* more than one GS object ever existed and/or the .gp pointers
> in different SCR objects could point to different GS objects, this
> patch might change behaviour.

If such multiple GS condition ever existed, since signals are (global),
the handler is only indicating a signal has happened.  And the code
observing non-zero of these signal-indicating variables would be
responsible to determine which event it applies to.  It is too hard
to do this kind of work safely/atomically in a signal handler.



Re: allow KARL with config(8)'d kernels

2021-08-29 Thread Theo de Raadt
Ingo Schwarze  wrote:

> One - admittedly completely unUNIXy - way would be to invent a long,
> descriptive name like /etc/kernel.config.commands or even /bsd.config.cmd
> in the root rather than the /etc directory, which is more discoverable
> because it is right next to the kernel itself.  The UNIXy way
> is not necessarily better: Ken would have invented a very short,
> pronouncable name that is not an English word but similar to one,
> like /etc/sycoc (for SYstem COnfiguration Commands).

I have no idea why you talk about Ken, how obscure

back in those days it wasn't called /kernel, either.

the pathname "kernel.something" which is supposed to be somewhat
related to a non-existant pathname "kernel", is what I object to.

perhaps /etc/bsd.re-config

but certainly the sub-phrase "kernel" is not discoverable.



Re: allow KARL with config(8)'d kernels

2021-08-29 Thread Theo de Raadt
man -k kernel, and man -k ukc, both suggest these are poor names
for different reasons.

maybe if you write some diffs to hint at the existance of this mechanism
in the config(8) and boot_config(8) manual pages, a better name will
sneak up on us.

Paul de Weerd  wrote:

> Hi Theo,
> 
> That's a good point, but I have no better alternative.  kernel.conf
> was the best I could come up with, as it is a configuration file for
> the (installed) kernel.  I briefly considered:
> 
> - config.conf (after config(8), but seems hilariously worse to me)
> - ukc.conf (has similar (perhaps even stronger) issues as kernel.conf)
> 
> Do others have a good suggestion for the color of this particular bike
> shed?  Open to suggestions!
> 
> Paul
> 
> On Sun, Aug 29, 2021 at 07:15:34AM -0600, Theo de Raadt wrote:
> | I am not thrilled with the name "kernel.conf".
> | 
> | It does not seem intuitively discoverable.
> | 
> | Paul de Weerd  wrote:
> | 
> | > Got some more positive feedback off-list, which reminded me that
> | > there's a small piece missing:
> | > 
> | > Index: changelist
> | > ===
> | > RCS file: /home/OpenBSD/cvs/src/etc/changelist,v
> | > retrieving revision 1.128
> | > diff -u -p -r1.128 changelist
> | > --- changelist30 Jul 2021 07:00:02 -  1.128
> | > +++ changelist29 Aug 2021 12:12:04 -
> | > @@ -56,6 +56,7 @@
> | >  +/etc/isakmpd/isakmpd.policy
> | >  /etc/isakmpd/local.pub
> | >  +/etc/isakmpd/private/local.key
> | > +/etc/kernel.conf
> | >  /etc/ksh.kshrc
> | >  /etc/ldapd.conf
> | >  /etc/ldpd.conf
> | > 
> | > Full diff (including the original diff, the diff to install.sub and
> | > the above changelist diff) below.  Anything else I overlooked?
> | > 
> | > Paul
> | > 
> | > Index: distrib/miniroot/install.sub
> | > ===
> | > RCS file: /home/OpenBSD/cvs/src/distrib/miniroot/install.sub,v
> | > retrieving revision 1.1172
> | > diff -u -p -r1.1172 install.sub
> | > --- distrib/miniroot/install.sub  9 Aug 2021 13:56:17 -   1.1172
> | > +++ distrib/miniroot/install.sub  25 Aug 2021 19:42:49 -
> | > @@ -2857,7 +2857,10 @@ finish_up() {
> | >   tar -C $_kernel_dir -xzf $_kernel_dir.tgz $_kernel
> | >   rm -f $_kernel_dir.tgz
> | >   chroot /mnt /bin/ksh -e -c "cd ${_kernel_dir#/mnt}/$_kernel; \
> | > - make newbsd; make newinstall"
> | > + make newbsd; \
> | > + [ -e /etc/kernel.conf ] && \
> | > + config -e -c /etc/kernel.conf -f bsd; \
> | > + make newinstall"
> | >   ) >/dev/null 2>&1 && echo " done." || echo " failed."
> | >   fi
> | >  
> | > Index: etc/changelist
> | > ===
> | > RCS file: /home/OpenBSD/cvs/src/etc/changelist,v
> | > retrieving revision 1.128
> | > diff -u -p -r1.128 changelist
> | > --- etc/changelist30 Jul 2021 07:00:02 -  1.128
> | > +++ etc/changelist29 Aug 2021 12:12:04 -
> | > @@ -56,6 +56,7 @@
> | >  +/etc/isakmpd/isakmpd.policy
> | >  /etc/isakmpd/local.pub
> | >  +/etc/isakmpd/private/local.key
> | > +/etc/kernel.conf
> | >  /etc/ksh.kshrc
> | >  /etc/ldapd.conf
> | >  /etc/ldpd.conf
> | > Index: libexec/reorder_kernel/Makefile
> | > ===
> | > RCS file: /home/OpenBSD/cvs/src/libexec/reorder_kernel/Makefile,v
> | > retrieving revision 1.1
> | > diff -u -p -r1.1 Makefile
> | > --- libexec/reorder_kernel/Makefile   21 Aug 2017 21:24:11 -  
> 1.1
> | > +++ libexec/reorder_kernel/Makefile   24 Aug 2021 07:23:38 -
> | > @@ -1,6 +1,7 @@
> | >  #$OpenBSD: Makefile,v 1.1 2017/08/21 21:24:11 rpe Exp $
> | >  
> | >  SCRIPT=  reorder_kernel.sh
> | > +MAN= kernel.conf.5
> | >  
> | >  realinstall:
> | >   ${INSTALL} ${INSTALL_COPY} -o ${BINOWN} -g ${BINGRP} -m ${BINMODE} \
> | > Index: libexec/reorder_kernel/kernel.conf.5
> | > ===
> | > RCS file: libexec/reorder_kernel/kernel.conf.5
> | > diff -N libexec/reorder_kernel/kernel.conf.5
> | > --- /dev/null 1 Jan 1970 00:00:00 -
> | > +++ libexec/reorder_kernel/kernel.conf.5  24 Aug 2021 07:23:07 -

Re: DANE in libressl?

2021-08-29 Thread Theo de Raadt
Is there a strong reason why this has to be in that specific library?

Peter J. Philipp  wrote:

> Hi,
> 
> I was wondering if anyone has wanted to implement DANE functions into OpenBSD?
> And LibreSSL perhaps?  I want this for syslogd with TLS, but not sure if I'd
> be on someones toes here, if I start implementing...
> 
> With unwind we can make use of things such as DANE due to validation of 
> DNSSEC.
> 
> Best Regards,
> -peter
> 



Re: allow KARL with config(8)'d kernels

2021-08-29 Thread Theo de Raadt
I am not thrilled with the name "kernel.conf".

It does not seem intuitively discoverable.

Paul de Weerd  wrote:

> Got some more positive feedback off-list, which reminded me that
> there's a small piece missing:
> 
> Index: changelist
> ===
> RCS file: /home/OpenBSD/cvs/src/etc/changelist,v
> retrieving revision 1.128
> diff -u -p -r1.128 changelist
> --- changelist30 Jul 2021 07:00:02 -  1.128
> +++ changelist29 Aug 2021 12:12:04 -
> @@ -56,6 +56,7 @@
>  +/etc/isakmpd/isakmpd.policy
>  /etc/isakmpd/local.pub
>  +/etc/isakmpd/private/local.key
> +/etc/kernel.conf
>  /etc/ksh.kshrc
>  /etc/ldapd.conf
>  /etc/ldpd.conf
> 
> Full diff (including the original diff, the diff to install.sub and
> the above changelist diff) below.  Anything else I overlooked?
> 
> Paul
> 
> Index: distrib/miniroot/install.sub
> ===
> RCS file: /home/OpenBSD/cvs/src/distrib/miniroot/install.sub,v
> retrieving revision 1.1172
> diff -u -p -r1.1172 install.sub
> --- distrib/miniroot/install.sub  9 Aug 2021 13:56:17 -   1.1172
> +++ distrib/miniroot/install.sub  25 Aug 2021 19:42:49 -
> @@ -2857,7 +2857,10 @@ finish_up() {
>   tar -C $_kernel_dir -xzf $_kernel_dir.tgz $_kernel
>   rm -f $_kernel_dir.tgz
>   chroot /mnt /bin/ksh -e -c "cd ${_kernel_dir#/mnt}/$_kernel; \
> - make newbsd; make newinstall"
> + make newbsd; \
> + [ -e /etc/kernel.conf ] && \
> + config -e -c /etc/kernel.conf -f bsd; \
> + make newinstall"
>   ) >/dev/null 2>&1 && echo " done." || echo " failed."
>   fi
>  
> Index: etc/changelist
> ===
> RCS file: /home/OpenBSD/cvs/src/etc/changelist,v
> retrieving revision 1.128
> diff -u -p -r1.128 changelist
> --- etc/changelist30 Jul 2021 07:00:02 -  1.128
> +++ etc/changelist29 Aug 2021 12:12:04 -
> @@ -56,6 +56,7 @@
>  +/etc/isakmpd/isakmpd.policy
>  /etc/isakmpd/local.pub
>  +/etc/isakmpd/private/local.key
> +/etc/kernel.conf
>  /etc/ksh.kshrc
>  /etc/ldapd.conf
>  /etc/ldpd.conf
> Index: libexec/reorder_kernel/Makefile
> ===
> RCS file: /home/OpenBSD/cvs/src/libexec/reorder_kernel/Makefile,v
> retrieving revision 1.1
> diff -u -p -r1.1 Makefile
> --- libexec/reorder_kernel/Makefile   21 Aug 2017 21:24:11 -  1.1
> +++ libexec/reorder_kernel/Makefile   24 Aug 2021 07:23:38 -
> @@ -1,6 +1,7 @@
>  #$OpenBSD: Makefile,v 1.1 2017/08/21 21:24:11 rpe Exp $
>  
>  SCRIPT=  reorder_kernel.sh
> +MAN= kernel.conf.5
>  
>  realinstall:
>   ${INSTALL} ${INSTALL_COPY} -o ${BINOWN} -g ${BINGRP} -m ${BINMODE} \
> Index: libexec/reorder_kernel/kernel.conf.5
> ===
> RCS file: libexec/reorder_kernel/kernel.conf.5
> diff -N libexec/reorder_kernel/kernel.conf.5
> --- /dev/null 1 Jan 1970 00:00:00 -
> +++ libexec/reorder_kernel/kernel.conf.5  24 Aug 2021 07:23:07 -
> @@ -0,0 +1,46 @@
> +.\"  $OpenBSD$
> +.\"
> +.\" Copyright (c) 2021 Paul de Weerd 
> +.\"
> +.\" Permission to use, copy, modify, and distribute this software for any
> +.\" purpose with or without fee is hereby granted, provided that the above
> +.\" copyright notice and this permission notice appear in all copies.
> +.\"
> +.\" THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
> +.\" WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
> +.\" MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
> +.\" ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
> +.\" WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
> +.\" ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
> +.\" OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> +.\"
> +.Dd $Mdocdate: August 24 2021 $
> +.Dt KERNEL.CONF 5
> +.Os
> +.Sh NAME
> +.Nm kernel.conf
> +.Nd kernel configuration file
> +.Sh DESCRIPTION
> +The
> +.Nm
> +file contains configuration information for the kernel.
> +If present, it is used during system startup to configure the kernel
> +that will be running at the next boot.
> +It can be used to enable or disable specific devices in the kernel.
> +.Sh EXAMPLES
> +To enable the
> +.Xr ipmi 4
> +driver, add the following line to
> +.Nm :
> +.Pp
> +.Dl enable ipmi
> +.Pp
> +See 
> +.Xr config 8
> +for more details on how to configure the kernel.
> +.Sh FILES
> +.Bl -tag -width /etc/kernel.conf -compact
> +.It Pa /etc/kernel.conf
> +Kernel configuration file.
> +.Sh SEE ALSO
> +.Xr config 8
> Index: libexec/reorder_kernel/reorder_kernel.sh
> 

Re: arm64 rpi4 upgrade, "Failed to install bootblocks" at end

2021-08-29 Thread Theo de Raadt
Mark Kettenis  wrote:

> Should installboot(8) handle the case where the filesystem is already
> mounted?

Yes.

I guess now that fsck_msdos is on the media, it will be run against
a mounted partition, with is yet another weird problem



Re: Atomic signal flags for vi(1)

2021-08-29 Thread Theo de Raadt
This does look better.

I appreciate that you are fixing this underlying problem first, before
overlaying your timer diff.

Is this working for the vi crowd?


trondd  wrote:

> "Theo de Raadt"  wrote:
> 
> > +h_alrm(int signo)
> > +{
> > +   GLOBAL_CLP;
> > +
> > +   F_SET(clp, CL_SIGALRM);
> > 
> > F_SET is |=, which is not atomic.
> > 
> > This is unsafe.  Safe signal handlers need to make single stores to
> > atomic-sized variables, which tend to be int-sized, easier to declare
> > as sig_atomic_t.  Most often these are global scope with the following
> > type:
> > 
> > volatile sig_atomic_t variable
> > 
> > I can see you copied an existing practice.  Sadly all the other
> > signal handlers are also broken in the same way.
> > 
> > The flag bits should be replaced with seperate sig_atomic_t variables.
> 
> Ok.  Took a swing at converting the signal handling to sig_atomic_t flags.
> No additional changes added other than removing a redundant check of the
> flags in cl_read.c.  Seemed to be a pretty straight-forward conversion and
> I haven't found any change in behavior.
> 
> Tim.
> 
> 
> Index: cl/cl.h
> ===
> RCS file: /cvs/src/usr.bin/vi/cl/cl.h,v
> retrieving revision 1.10
> diff -u -p -r1.10 cl.h
> --- cl/cl.h   27 May 2016 09:18:11 -  1.10
> +++ cl/cl.h   24 Aug 2021 23:34:27 -
> @@ -11,6 +11,11 @@
>   *   @(#)cl.h10.19 (Berkeley) 9/24/96
>   */
>  
> +extern   volatile sig_atomic_t cl_sighup;
> +extern   volatile sig_atomic_t cl_sigint;
> +extern   volatile sig_atomic_t cl_sigterm;
> +extern   volatile sig_atomic_t cl_sigwinch;
> +
>  typedef struct _cl_private {
>   CHAR_T   ibuf[256]; /* Input keys. */
>  
> @@ -45,11 +50,7 @@ typedef struct _cl_private {
>  #define  CL_RENAME_OK0x0004  /* User wants the windows renamed. */
>  #define  CL_SCR_EX_INIT  0x0008  /* Ex screen initialized. */
>  #define  CL_SCR_VI_INIT  0x0010  /* Vi screen initialized. */
> -#define  CL_SIGHUP   0x0020  /* SIGHUP arrived. */
> -#define  CL_SIGINT   0x0040  /* SIGINT arrived. */
> -#define  CL_SIGTERM  0x0080  /* SIGTERM arrived. */
> -#define  CL_SIGWINCH 0x0100  /* SIGWINCH arrived. */
> -#define  CL_STDIN_TTY0x0200  /* Talking to a terminal. */
> +#define  CL_STDIN_TTY0x0020  /* Talking to a terminal. */
>   u_int32_t flags;
>  } CL_PRIVATE;
>  
> Index: cl/cl_funcs.c
> ===
> RCS file: /cvs/src/usr.bin/vi/cl/cl_funcs.c,v
> retrieving revision 1.20
> diff -u -p -r1.20 cl_funcs.c
> --- cl/cl_funcs.c 27 May 2016 09:18:11 -  1.20
> +++ cl/cl_funcs.c 24 Aug 2021 23:34:27 -
> @@ -601,7 +601,7 @@ cl_suspend(SCR *sp, int *allowedp)
>   if (cl_ssize(sp, 1, NULL, NULL, ))
>   return (1);
>   if (changed)
> - F_SET(CLP(sp), CL_SIGWINCH);
> + cl_sigwinch = 1;
>  
>   return (0);
>  }
> Index: cl/cl_main.c
> ===
> RCS file: /cvs/src/usr.bin/vi/cl/cl_main.c,v
> retrieving revision 1.33
> diff -u -p -r1.33 cl_main.c
> --- cl/cl_main.c  5 May 2016 20:36:41 -   1.33
> +++ cl/cl_main.c  24 Aug 2021 23:34:27 -
> @@ -33,6 +33,11 @@
>  
>  GS *__global_list;   /* GLOBAL: List of screens. */
>  
> +volatile sig_atomic_t cl_sighup;
> +volatile sig_atomic_t cl_sigint;
> +volatile sig_atomic_t cl_sigterm;
> +volatile sig_atomic_t cl_sigwinch;
> +
>  static void cl_func_std(GS *);
>  static CL_PRIVATE *cl_init(GS *);
>  static GS  *gs_init(void);
> @@ -217,16 +222,14 @@ h_hup(int signo)
>  {
>   GLOBAL_CLP;
>  
> - F_SET(clp, CL_SIGHUP);
> + cl_sighup = 1;
>   clp->killersig = SIGHUP;
>  }
>  
>  static void
>  h_int(int signo)
>  {
> - GLOBAL_CLP;
> -
> - F_SET(clp, CL_SIGINT);
> + cl_sigint = 1;
>  }
>  
>  static void
> @@ -234,16 +237,14 @@ h_term(int signo)
>  {
>   GLOBAL_CLP;
>  
> - F_SET(clp, CL_SIGTERM);
> + cl_sigterm = 1;
>   clp->killersig = SIGTERM;
>  }
>  
>  static void
>  h_winch(int signo)
>  {
> - GLOBAL_CLP;
> -
> - F_SET(clp, CL_SIGWINCH);
> + cl_sigwinch = 1;
>  }
>  #undef   GLOBAL_CLP
>  
> @@ -259,6 +260,11 @@ sig_init(GS *gp, SCR *sp)
>   CL_PRIVATE *clp;
>  
>   clp = GCLP(gp);
> +
> + cl_sighup = 0;
> + cl_sigint 

Re: [patch] traceroute timeouts

2021-08-20 Thread Theo de Raadt
Global configuration state for per-use commands is crazy.

Tom Smyth  wrote:

> Hello all,,
> would it make sense
> to have the value as a sysctl  option or an environment variable ?
> so that it can be tailored for users  /admins needs,
> 
> 
> 
> On Fri 20 Aug 2021, 12:22 Mark Kettenis,  wrote:
> 
> > > From: Florian Obser 
> > > Date: Fri, 20 Aug 2021 10:46:21 +0200
> > >
> > > Makes sense to me, OK florian
> >
> > Doesn't make sense to me.  The RTT for an ICMP packet can be a
> > significant part of a second (think Europe-Australia the wrong way
> > around cause that is where all the bandwidth is, or when satellites
> > are involved).  I think this means that a single dropped packet could
> > result in a failure to resolve one of the hops on such a path.
> >
> > I don't necessarily object to giving folks the ammunition to shoot
> > themselves into the foot by dropping the minimum value to 1 second.
> > But the default should be larger I think.
> >
> > > On 2021-08-19 23:47 -07,  wrote:
> > > > The default traceroute timeout of 5 seconds is excruciatingly long
> > > > when there are elements of the route that don't respond, and it
> > > > wasn't allowed to be set lower than 2 seconds.
> > > >
> > > > This changes the minimum to 1 second, matching FreeBSD, and also
> > > > makes that the default, which should be reasonable for the vast
> > > > majority of users today.
> > > >
> > > > The two awk files in this directory are two decades old, and
> > > > not installed anywhere they can be executed as part of a traceroute
> > > > pipeline; can they be removed? If the functionality is useful,
> > > > implementing mean/median reporting as a new option in C would be
> > > > straightforward.
> > > >
> > > > Index: usr.sbin/traceroute/traceroute.8
> > > > ===
> > > > RCS file: /cvs/src/usr.sbin/traceroute/traceroute.8,v
> > > > retrieving revision 1.69
> > > > diff -u -p -u -r1.69 traceroute.8
> > > > --- usr.sbin/traceroute/traceroute.811 Feb 2020 18:41:39
> > -  1.69
> > > > +++ usr.sbin/traceroute/traceroute.820 Aug 2021 06:33:30 -
> > > > @@ -201,7 +201,7 @@ and
> > > >  are listed.
> > > >  .It Fl w Ar waittime
> > > >  Set the time, in seconds, to wait for a response to a probe.
> > > > -The default is 5.
> > > > +The default is 1.
> > > >  .It Fl x
> > > >  Print the ICMP extended headers if available.
> > > >  This option is not available for IPv6.
> > > > Index: usr.sbin/traceroute/traceroute.c
> > > > ===
> > > > RCS file: /cvs/src/usr.sbin/traceroute/traceroute.c,v
> > > > retrieving revision 1.164
> > > > diff -u -p -u -r1.164 traceroute.c
> > > > --- usr.sbin/traceroute/traceroute.c12 Jul 2021 15:09:21
> > -  1.164
> > > > +++ usr.sbin/traceroute/traceroute.c20 Aug 2021 06:33:30 -
> > > > @@ -351,7 +351,7 @@ main(int argc, char *argv[])
> > > > rcvsock4 = rcvsock6 = sndsock4 = sndsock6 = -1;
> > > > v4sock_errno = v6sock_errno = 0;
> > > >
> > > > -   conf->waittime = 5 * 1000;
> > > > +   conf->waittime = 1000;
> > > >
> > > > if ((rcvsock6 = socket(AF_INET6, SOCK_RAW, IPPROTO_ICMPV6)) == -1)
> > > > v6sock_errno = errno;
> > > > @@ -554,9 +554,9 @@ main(int argc, char *argv[])
> > > > err(1, "setsockopt SO_RTABLE");
> > > > break;
> > > > case 'w':
> > > > -   conf->waittime = strtonum(optarg, 2, INT_MAX,
> > );
> > > > +   conf->waittime = strtonum(optarg, 1, INT_MAX,
> > );
> > > > if (errstr)
> > > > -   errx(1, "wait must be >1 sec.");
> > > > +   errx(1, "wait must be >=1 sec.");
> > > > conf->waittime *= 1000;
> > > > break;
> > > > case 'x':
> > > >
> > > >
> > >
> > > --
> > > I'm not entirely sure you are real.
> > >
> > >
> >
> >



Re: [patch] traceroute timeouts

2021-08-20 Thread Theo de Raadt
I am aware of (terrible) paths inside Canada which get close to 600ms.

I believe 1 second will make traceroute subtly fail to diagnose network
issues.  I suspect 3 seconds might still work in the majority of cases,
but even 2 seconds seems too tight.

I remember many years ago when the around-australia research mpls was
incorrectly configured such that icmp replies generated inside had to go
all the way around to find an exit.  From my point of view in Calgary, I
recall timestamps exceeding 2 seconds.  If the default for listening was
1 second, those replies would simply not be captured, and you cannot
figure out what is happening.  But since I could see the outrageous
timestamps, I was able to to figure it out, laugh, and then tell them
about it.

If traceroute was refactored to probe and listen out of order (like
MTR), then it's speed could be increased.  There have been discussions
about this before...

Florian Obser  wrote:

> I guess I was too optimistic.
> I regularly work on machines that are 600-700 ms away and figured an 
> additional 300 ms is good enough. Maybe not in case of congested links...
> 
> On 20 August 2021 13:17:12 CEST, Mark Kettenis  
> wrote:
> >> From: Florian Obser 
> >> Date: Fri, 20 Aug 2021 10:46:21 +0200
> >> 
> >> Makes sense to me, OK florian
> >
> >Doesn't make sense to me.  The RTT for an ICMP packet can be a
> >significant part of a second (think Europe-Australia the wrong way
> >around cause that is where all the bandwidth is, or when satellites
> >are involved).  I think this means that a single dropped packet could
> >result in a failure to resolve one of the hops on such a path.
> >
> >I don't necessarily object to giving folks the ammunition to shoot
> >themselves into the foot by dropping the minimum value to 1 second.
> >But the default should be larger I think.
> >
> >> On 2021-08-19 23:47 -07,  wrote:
> >> > The default traceroute timeout of 5 seconds is excruciatingly long
> >> > when there are elements of the route that don't respond, and it
> >> > wasn't allowed to be set lower than 2 seconds.
> >> >
> >> > This changes the minimum to 1 second, matching FreeBSD, and also
> >> > makes that the default, which should be reasonable for the vast
> >> > majority of users today.
> >> >
> >> > The two awk files in this directory are two decades old, and
> >> > not installed anywhere they can be executed as part of a traceroute
> >> > pipeline; can they be removed? If the functionality is useful,
> >> > implementing mean/median reporting as a new option in C would be
> >> > straightforward.
> >> >
> >> > Index: usr.sbin/traceroute/traceroute.8
> >> > ===
> >> > RCS file: /cvs/src/usr.sbin/traceroute/traceroute.8,v
> >> > retrieving revision 1.69
> >> > diff -u -p -u -r1.69 traceroute.8
> >> > --- usr.sbin/traceroute/traceroute.8 11 Feb 2020 18:41:39 -  
> >> > 1.69
> >> > +++ usr.sbin/traceroute/traceroute.8 20 Aug 2021 06:33:30 -
> >> > @@ -201,7 +201,7 @@ and
> >> >  are listed.
> >> >  .It Fl w Ar waittime
> >> >  Set the time, in seconds, to wait for a response to a probe.
> >> > -The default is 5.
> >> > +The default is 1.
> >> >  .It Fl x
> >> >  Print the ICMP extended headers if available.
> >> >  This option is not available for IPv6.
> >> > Index: usr.sbin/traceroute/traceroute.c
> >> > ===
> >> > RCS file: /cvs/src/usr.sbin/traceroute/traceroute.c,v
> >> > retrieving revision 1.164
> >> > diff -u -p -u -r1.164 traceroute.c
> >> > --- usr.sbin/traceroute/traceroute.c 12 Jul 2021 15:09:21 -  
> >> > 1.164
> >> > +++ usr.sbin/traceroute/traceroute.c 20 Aug 2021 06:33:30 -
> >> > @@ -351,7 +351,7 @@ main(int argc, char *argv[])
> >> >  rcvsock4 = rcvsock6 = sndsock4 = sndsock6 = -1;
> >> >  v4sock_errno = v6sock_errno = 0;
> >> >  
> >> > -conf->waittime = 5 * 1000;
> >> > +conf->waittime = 1000;
> >> >  
> >> >  if ((rcvsock6 = socket(AF_INET6, SOCK_RAW, IPPROTO_ICMPV6)) == 
> >> > -1)
> >> >  v6sock_errno = errno;
> >> > @@ -554,9 +554,9 @@ main(int argc, char *argv[])
> >> >  err(1, "setsockopt SO_RTABLE");
> >> >  break;
> >> >  case 'w':
> >> > -conf->waittime = strtonum(optarg, 2, INT_MAX, 
> >> > );
> >> > +conf->waittime = strtonum(optarg, 1, INT_MAX, 
> >> > );
> >> >  if (errstr)
> >> > -errx(1, "wait must be >1 sec.");
> >> > +errx(1, "wait must be >=1 sec.");
> >> >  conf->waittime *= 1000;
> >> >  break;
> >> >  case 'x':
> >> >
> >> >
> >> 
> >> -- 
> >> I'm not entirely sure you are real.
> >> 
> >> 
> 
> -- 
> Sent from a mobile device. Please excuse poor 

Re: Improve vi(1) recovery

2021-08-19 Thread Theo de Raadt
+h_alrm(int signo)
+{
+   GLOBAL_CLP;
+
+   F_SET(clp, CL_SIGALRM);

F_SET is |=, which is not atomic.

This is unsafe.  Safe signal handlers need to make single stores to
atomic-sized variables, which tend to be int-sized, easier to declare
as sig_atomic_t.  Most often these are global scope with the following
type:

volatile sig_atomic_t variable

I can see you copied an existing practice.  Sadly all the other
signal handlers are also broken in the same way.

The flag bits should be replaced with seperate sig_atomic_t variables.



Re: netstart routing domain loopback

2021-08-18 Thread Theo de Raadt
If you change the evaluation order in netstart, something similar
may be needed in the install script emulation of netstart.

Alexander Bluhm  wrote:

> Hi,
> 
> I want to create an enc1 interface for routing domain 1 and set
> additional addresses on lo1.  So my net config looks like this.
> 
> ==> /etc/hostname.enc1 <==
> rdomain 1
> 
> ==> /etc/hostname.lo1 <==
> rdomain 1
> inet alias 10.188.10.74 255.255.255.255
> ...
> 
> /etc/netstart creates enc1 in routing domain 0, but does not place
> it into rdomain 1 due to this error:
> starting network
> ifconfig: SIOCSIFRDOMAIN: Invalid argument
> 
> With some additional debug output netstart does this:
> { ifconfig enc1 || ifconfig enc1 create; }
> { ifconfig lo0 || ifconfig lo0 create; }
> { ifconfig lo1 || ifconfig lo1 create; }
> { ifconfig enc1 || ifconfig enc1 create; }
> ifconfig enc1 rdomain 1
> ...
> { ifconfig lo1 || ifconfig lo1 create; }
> ifconfig lo1 rdomain 1
> ifconfig lo1 inet alias 10.188.10.74 netmask 255.255.255.255
> 
> ifconfig enc1 rdomain 1 fails as lo1 exists, but is still in routing
> domain 0.
> 
> To fix this, I think lo1, lo2, ... should not be created upfront.
> Also more debug output for /etc/netstart -n is necessary to understand
> what is going on.
> 
> ok?
> 
> bluhm
> 
> Index: netstart
> ===
> RCS file: /data/mirror/openbsd/cvs/src/etc/netstart,v
> retrieving revision 1.214
> diff -u -p -r1.214 netstart
> --- netstart  6 Aug 2021 07:06:35 -   1.214
> +++ netstart  18 Aug 2021 14:13:01 -
> @@ -86,7 +86,11 @@ parse_hn_line() {
>  ifcreate() {
>   local _if=$1
>  
> - { ifconfig $_if || ifconfig $_if create; } >/dev/null 2>&1
> + if $PRINT_ONLY; then
> + print -r -- "{ ifconfig $_if || ifconfig $_if create; }"
> + else
> + { ifconfig $_if || ifconfig $_if create; } >/dev/null 2>&1
> + fi
>  }
>  
>  # Create interfaces for network pseudo-devices referred to by hostname.if 
> files.
> @@ -99,6 +103,9 @@ vifscreate() {
>   [[ -f $_hn ]] || continue
>   _if=${_hn#/etc/hostname.}
>  
> + # loopback for routing domain is created by kernel
> + [[ -n "${_if##lo[1-9]*}" ]] || continue
> +
>   if ! ifcreate $_if; then
>   print -u2 "${0##*/}: create for '$_if' failed."
>   fi
> @@ -130,9 +137,7 @@ ifstart() {
>   fi
>  
>   # Check for ifconfig'able interface, except if -n option is specified.
> - if ! $PRINT_ONLY; then
> - ifcreate $_if || return
> - fi
> + ifcreate $_if || return
>  
>   # Parse the hostname.if(5) file and fill _cmds array with interface
>   # configuration commands.
> @@ -232,9 +237,6 @@ while getopts ":n" opt; do
>   esac
>  done
>  shift $((OPTIND-1))
> -
> -# Option -n is only supported if interface names are specified as parameters.
> -$PRINT_ONLY && (($# == 0)) && usage
>  
>  # Load key material for the generation of IPv6 Semantically Opaque Interface
>  # Identifiers (SOII) used for link local and SLAAC addresses.
> 



Re: cal(1): Clean up mutually exclusive options

2021-08-16 Thread Theo de Raadt
Jason McIntyre  wrote:

> well, in those cases i think the authors shared the viewpoint that
> mutually exclusive means you can;t mix them but in this case it is
> essentially not an error to do so, and so documented it that way.
> 
> maybe it is more helpful to think of "mutually exclusive" as "causes an
> error", but i am not sure.

I agree with you that programs should not neccessary error out.

I appreciate that you pointed at ls(1) before, which has so many options
I think we should look there for examples.

 -A  List all entries except for `.' and `..'.  Always set for the
 superuser.

 -a  Include directory entries whose names begin with a dot (`.').

These two options are not described as mutually exclusive, except, a
mutually exclusive action is described.

How do -Aa and -aA behave?  Try it.

Does POSIX take a position?

If POSIX doesn't take a position, why should we?

Should we break scripts that accidentally use both options?  Argument
composition does happen in scripting and interactive use.

Should we search for places to make POSIX programs exit(1) if people
use them in the newly-defined incorrect way?

If we decide to not cause ls(1) -a and -A to exit, then I think we
should not make other programs exit either.

And then probably follow jmc's lead that "mutually exclusive" either
in text, or in interpreted text, doesn't neccessarily describe an exit
action.



Re: cal(1): Clean up mutually exclusive options

2021-08-15 Thread Theo de Raadt
This conversation is strange, because we had a very similar argument
recently about some other command (I forgot which one), about what
policy should occur for repeated commands.

I think once we step outside a narrowly defined POSIX option argument
scheme, we need to understand we are outside POSIX.  Meanwhile, there
is this "sed and line 0" conversation flowing through my mailbox, where
a strictly defined POSIX command's GNU extensions causes people trouble.

Is it really reasonable that we should strictly follow a non-applicable
standard in such rarely used non-standard utilities, when
strictly-standardized utilities so often have non-standard extensions
which acutely poison the user expectations in more harmful ways (meaning,
incorrect results IN SCRIPTS).

We can't push back against established facts in sed because it would break
people's scripts somewhere, why do we need to push back against established
facts for people who might use cal in this weird way in a script?

Beyond the theory of "lacking guidance, let's impose a POSIX rule after
the fact", has there been any attempt to JUDGE if this will affect anyone
in scripts?

Jason McIntyre  wrote:

> On 15 August 2021 22:40:49 BST, Martijn van Duren 
>  wrote:
> >To quote schwarze in the jot mutually exclusive thread:
> >On Fri, 2021-08-13 at 11:48 +0200, Ingo Schwarze wrote:
> >> In this case, even though this is not a POSIX command, POSIX utility
> >> convention 12.2.11 is pertinent:
> >> 
> >>   The order of different options relative to one another should not
> >>   matter, unless the options are documented as mutually-exclusive
> >>   and such an option is documented to override any incompatible
> >>   options preceding it.  If an option that has option-arguments is
> >>   repeated, the option and option-argument combinations should be
> >>   interpreted in the order specified on the command line.
> >
> >This is also violated by cal(1) (and maybe others, but this one came
> >up first). Diff below should fix this.
> >
> >OK?
> >
> >martijn@
> >
> >Index: cal.1
> >===
> >RCS file: /cvs/src/usr.bin/cal/cal.1,v
> >retrieving revision 1.31
> >diff -u -p -r1.31 cal.1
> >--- cal.127 Nov 2016 10:37:22 -  1.31
> >+++ cal.115 Aug 2021 21:39:28 -
> >@@ -53,11 +53,8 @@ The options are as follows:
> > .Bl -tag -width Ds
> > .It Fl j
> > Display Julian dates (days one-based, numbered from January 1).
> >-The options
> >-.Fl j
> >-and
> >-.Fl w
> >-are mutually exclusive.
> >+Overrides earlier
> >+.Fl w .
> > .It Fl m
> > Display weeks starting on Monday instead of Sunday.
> > .It Fl w
> >@@ -65,11 +62,8 @@ Display week numbers in the month displa
> > If
> > .Fl m
> > is specified the ISO week format is assumed.
> >-The options
> >-.Fl j
> >-and
> >-.Fl w
> >-are mutually exclusive.
> >+Overrides earlier
> >+.Fl j .
> > .It Fl y
> > Display a calendar for the current year.
> > .El
> >Index: cal.c
> >===
> >RCS file: /cvs/src/usr.bin/cal/cal.c,v
> >retrieving revision 1.30
> >diff -u -p -r1.30 cal.c
> >--- cal.c9 Oct 2015 01:37:06 -   1.30
> >+++ cal.c15 Aug 2021 21:39:28 -
> >@@ -158,12 +158,14 @@ main(int argc, char *argv[])
> > switch(ch) {
> > case 'j':
> > julian = 1;
> >+wflag = 0;
> > break;
> > case 'm':
> > mflag = 1;
> > break;
> > case 'w':
> > wflag = 1;
> >+julian = 0;
> > break;
> > case 'y':
> > yflag = 1;
> >@@ -174,9 +176,6 @@ main(int argc, char *argv[])
> > }
> > argc -= optind;
> > argv += optind;
> >-
> >-if (julian && wflag)
> >-usage();
> > 
> > day_headings = DAY_HEADINGS_S;
> > sep1752 = sep1752s;
> >
> >
> 
> hi.
> 
> i don't like this approach, because i think the changes are confusing. 
> 
> can't we take a stance that where options override each other, the last one 
> wins?  and then not document this fact every time. or at least document 
> exceptions only?
> 
> ...and continue to document where options are mutually exclusive? 
> 
> also the text "overrides earlier" is unclear.
> 
> jmc
> 



Re: fresh prompt after Ctrl-C in cdio(1)

2021-08-13 Thread Theo de Raadt
Ingo Schwarze  wrote:

> Any user of this program with a sufficient knowledge of signal
> handling is welcome to design, implement, and test signal handling
> that remains active during other parts of the execution of the
> cdio(1) program.  The committed patch provides a starting point.

Pepole with sufficient knowledge of signal handling are too rare
for that to happen.
 



Re: dhcpleased(8): ignore servers / parts of lease

2021-08-09 Thread Theo de Raadt
Using the word "security", you've got to be kidding.

If a dhcp server on a L2 segment can be "rogue" about one thing, it can
most certainly lie about any other answer, or act out in many other
ways.

The only way to avoid "rogue" DHCP servers on a segment is to not ask
DHCP questions on that segment.

This is not a security feature.  It is purely for selecting aspects of
the answer from TRUSTED DHCP servers.

Andras Vinter  wrote:

> The Linux dhclient supports it and it's actually a nice to have
> feature as it can increase the security by keeping out the rogue DHCP
> servers from an entire LAN range. But probably you can achieve similar
> functionality with the interface restriction.
> 
> On Mon, Aug 9, 2021 at 3:33 PM Stuart Henderson  wrote:
> >
> > On 2021/08/09 15:03, Andras Vinter wrote:
> > > It's probably an overkill for first implementation, but in the future
> > > I think we should support subnet definitions in CIDR notation (e.x.:
> > > 192.168.0.0/24) and IP ranges for fine control (e.x.:
> > > 192.168.0.100-192.168.0.254).
> >
> > dhclient never needed that.
> >
> 



Re: libedit: stop ignoring SIGINT

2021-08-09 Thread Theo de Raadt
Ingo Schwarze  wrote:

> > So if we decide which of our interpretations should take precedence
> > it might be a good idea to put it into snaps for a while.
> 
> I don't think so in this case.  Let's not over-use the feature of
> putting stuff in snaps.  I think that should be reserved for stuff
> that is quite important and somewhat urgent and can't easily be
> tested in a less disruptive way.  But here, testing a program is
> quite feasible once you know which program to test.

We know what needs testing, because it links against the library.
Once the first set of base programs is tested to work well with the
change, it is quite likely we have complete confidence it only
improves the situation in ports.

Snapshot testing is reserved for discovering unknown breakage, quickly.
Like "we don't know how or which programs abuse something" or "what
weird machines do people have" or "we don't know how our user's fingers
work".  It's not like our users are going to quickly discover a weird
behaviour from ^C, considering I only noticed this decade-old problem
in sftp a few days ago.



Re: libedit: stop ignoring SIGINT

2021-08-09 Thread Theo de Raadt
Ingo Schwarze  wrote:

> as mentioned earlier, deraadt@ reported that sftp(1) ignores Ctrl-C.
> Fixing that without longjmp(3) requires making editline(3) better
> behaved.

If a library interface encourages use longjmp(), that library should be
thrown into the dustbin of history.  Our src tree has very few longjmp
these days.  Thank you for the effort to discourage addition of more.

> The following patch causes el_gets(3) and el_wgets(3) to return
> failure when read(2)ing from the terminal is interrupted by a
> signal other than SIGCONT or SIGWINCH.  That allows the calling
> program to decide what to do, usually either exit the program or
> provide a fresh prompt to the user.

Looks good.

>  * bc(1)
>It behaves well with the patch: Ctrl-C discards the current
>input line and starts a new input line.
>The reason why this already works even without the patch
>is that bc(1) does very scary stuff inside the signal handler:
>pass a file-global EditLine pointer on the heap to el_line(3)
>and access fields inside the returned struct.  Needless to
>say that no signal handler should do such things...

Otto -- if you are short of time, at minimum mark the variable usage
line with /* XXX signal race */ as we have done throughout the tree.  I
would encourage anyone who sees such problems inside any signal handler
to show such comment-adding diffs.  If these problems are documented,
they can be fixed eventually, usually through event-loop logic, but I'll
admit many of the comments are in non-event-loop programs.

>  * ftp(1)
>It behaves well with the patch: Ctrl-C discards the current
>input line and provides a fresh prompt.
>The reason why it already works without the patch is that ftp(1)
>uses setjmp(3)/longjmp(3) to forcefully grab back control
>from el_gets(3) without waiting for it to return.

Horrible isn't it.

>  * sftp(1)
>Behaviour is improved with the patch: Ctrl-C now exits sftp(1).
>If desired, i can supply a very simple follow-up patch to sftp.c
>to instead behave like ftp(1) and bc(1), i.e. discard the
>current input line and provide a fresh prompt.
>Neither doing undue work in the signal handler nor longjmp(3)
>will be required for that (if this patch gets committed).

I suspect dtucker will want to decide on the interactive behaviour,
but what you describe sounds right.

> Also note that deraadt@ pointed out in private mail to me that the
> fact that read__fixio() clears FIONBIO is probably a symptom of
> botched editline(3) API design.  That might be worth fixing, too,
> as far as that is feasible, but it is unrelated to the sftp(1)
> Ctrl-C issue; let's address one topic at a time.

I mentioned rarely having seen a good outcome from code mixing any of
the 3: FIONBIO, FIONREAD, and select/poll.  Often the non-blocking was
added to select/poll code to hide some sort of bug, or the select/poll
code was added amateurishly to older code without removing the FIONBIO.
There are a few situations you need both approaches mixed, but it isn't
the general case, and thus FIONBIO has a "polled IO" smell for me.



Re: Fix unsafe snmpd defaults

2021-08-07 Thread Theo de Raadt
Martijn van Duren  wrote:

> Maybe, but it also made it clear that relying on the defaults for a 
> protocol (USM) that doesn't allow to be negotiated will make users
> cry.

You have selected defaults that noone will naturally use in their existing
*OR NEW* environment, and they could spend a lots of time debugging this.

> So you mean we should default to hmac-md5? Because p5-Net-SNMP and
> netSNMP default to hmac-md5 and I've seen some machines (HP printers)
> that only support hmac-md5, but not hmac-sha1.

Yes, probably hmac-md5

hmac-md5 is not insecure.  You've been told your premise for changing
the hmac is unsound, since you started 

> Shall we also revert back the DES? AES for USM is part of RFC3826, which
> is also not standard. It's also not the default in many implementations
> (both p5-Net-SNMP and netSNMP default to DES) and the same HP printers
> mentioned above only support DES.

The cipher is not the hmac.  It is illogical to tie the default for one
to the other.

> > The older hmac continues to satisfy the security requirement.
> 
> I wouldn't equal "This isn't all that great by today's standards, but far
> from completely bonkers." to "continues to satisfy". But sure.

Wrong.  Any hmac completely satisfies the requirement -- today and
forever.  As a rule, a crappy hash inside a hmac construction makes for
a good hmac.

There are people who want to kill old hashes, which is a good effort.
Removing old hashes everywhere is a big effort, and lots of
compatibility breaks will be painful.  So it is important to change the
most unsafe constructions first, and it is OK to delay disruptive change
for the safe constructs.  Unfortunately those people are a cult who
ignore the case that there are safe constructs, and they wants to break
everything at once, no matter how inconvenient it is.  Did they send you
a card to carry?

> But the change to AES also causes the same breakage for users who choose
> to go with the default (which was DES) and DES is still the default for
> most USM implementations out there.

The admin commmunity is well aware of "cipher", and generally know how to
debug that.  They are less aware there is a secondary "auth", and I suspect
being unaware of this layer causes more pain.

> I see where you're coming from, but I still disagree.
>
> But let me summarize:
> - We're being increadibly disruptive on the defaults this cycle.
> - We're not taking away anyones ability to interoperate: they just need
>   to add 4 little words per user.
> - Defaults for USM are a bad idea, because there's no negotiation
>   between agents.
> - sha256 is slower then md5/sha1 which should help with slowing down
>   brute force attacks against the hmac key if they're password derived.
> - From the implementations I've seen in the wild hmac-md5 is the default
>   for most, not hmac-sha1
> - Similar DES is still the default for most implementations, not AES
> - snmp(1) should be kept in sync with snmpd(8)
> 
> Given all this I'm inclined to remove the defaults all together and let
> people get errors on startup, so that they know that they need to be
> explicit; as per sthen@'s previous suggestion. This will also solve
> any des-incompatibility issues that might arrise from the default
> change.
> But if that's not desired, as stated: Go ahead with sthen's diff
> without my OK and make sure that snmp(1) is brought along.

For a made-up and incorrect security reason, let's screw all the
unfamiliar people trying to get work done quickly with the software,
maybe they'll just install the snmp package tools and you won't have to
worry about complaints.




Re: scp(1) changes in snaps

2021-08-07 Thread Theo de Raadt
Damien Miller  wrote:

> On Fri, 6 Aug 2021, Christian Weisgerber wrote:
> 
> > Damien Miller:
> > 
> > > Just a head-up: snaps currently contain a set of changes[1] to
> > > make scp(1) use the SFTP protocol by default.
> > 
> > > Please report any incompatibilities or bugs that you encounter here
> > > (tech@), to bugs@ or to openssh@.
> > 
> > An obvious cosmetic difference is that relative paths are prefixed
> > with the home directory of the remote source in the progress bar:
> > 
> > $ scp lorvorc:foo /dev/null
> > /home/naddy/foo   100% 4099 1.6MB/s   00:00 
> >
> > $ scp -O lorvorc:foo /dev/null
> > foo   100% 4099 3.7MB/s   00:00 
> >
> > 
> > I don't know if we care.
> 
> Yeah, I'm inclined to leave the full paths unless there's a good argument
> for truncating them.

1) Are there circumstances where the remote homedir expansion is slightly 
secret?

2) With very long names, it truncates the end of the path. This is less useful
information.  Imagine a copy operation with multiple files being transferred,
one of them is huge and surprisingly long, but you cannot identify which one
because all the truncated long paths are the same.

3) (old) scp and rsync show the path only component only.

So I think the path-component-only scheme is better, and this should change.




Re: less(1): refreshing file of size 0 results in file being treated as a pipe

2021-08-06 Thread Theo de Raadt
OK deraadt

Ingo Schwarze  wrote:

> Hi,
> 
> after quite some head-scratching, i consider the following bug report
> legitimate:
> 
> user wrote on Thu, Aug 05, 2021 at 12:43:21AM -0500:
> > On Thu, Aug 05, 2021 at 12:37:00AM -0500, user wrote:
> >> On Fri, Jul 23, 2021 at 11:15:59AM -0500, user wrote:
> 
> >>> Less contains a hack to force files of size 0 to become non-seekable
> >>> in order to workaround a linux kernel bug. 
> 
> I'm inserting a few words into the next sentence to make it clearer
> what it is trying to say:
> 
> >>> When the file becomes non-seekable any further reads from the file
> >>> are appended
> 
>   to the temporary buffer in which less(1) holds the content
>   of the file
> 
> >>> rather than overwriting the original contents of the file
> 
>   in that buffer.
> 
> >> Bug Reproduction:
> 
>$ rm -rf /tmp/test
> 
> >> $ touch /tmp/test
> >> $ less /tmp/test
> >> Press r
> 
> > $ echo a > /tmp/test# my comment: from a different shell
> > Press h and q in less to reload the file
> > $ echo b > /tmp/test
> > Press h and q in less to reload the file
> 
> Now less(1) shows the following on the screen because it thinks
> that would be the current content of the file:
> 
> >> a
> >> b
> 
> That is wrong.  Instead, it should show the actual file content,
> which is just:
> 
>   b
> 
> I think the proposed patch makes sense and should be committed:
> File size has nothing to do with whether a file is seekable,
> so i don't think it can cause regressions.
> Also, it fixes the bug described above.
> 
> Any developer willing to provide an OK?
> Alternatively, commit yourself with OK schwarze@.
> 
> I'm attaching the patch again because the OP mangled it (tabs
> replaced by spaces) and it did not apply.
> 
> Yours,
>   Ingo
> 
> 
> Index: ch.c
> ===
> RCS file: /cvs/src/usr.bin/less/ch.c,v
> retrieving revision 1.21
> diff -u -p -r1.21 ch.c
> --- ch.c  3 Sep 2019 23:08:42 -   1.21
> +++ ch.c  6 Aug 2021 16:14:29 -
> @@ -643,19 +643,6 @@ ch_flush(void)
>   ch_block = 0; /* ch_fpos / LBUFSIZE; */
>   ch_offset = 0; /* ch_fpos % LBUFSIZE; */
>  
> -#if 1
> - /*
> -  * This is a kludge to workaround a Linux kernel bug: files in
> -  * /proc have a size of 0 according to fstat() but have readable
> -  * data.  They are sometimes, but not always, seekable.
> -  * Force them to be non-seekable here.
> -  */
> - if (ch_fsize == 0) {
> - ch_fsize = -1;
> - ch_flags &= ~CH_CANSEEK;
> - }
> -#endif
> -
>   if (lseek(ch_file, (off_t)0, SEEK_SET) == (off_t)-1) {
>   /*
>* Warning only; even if the seek fails for some reason,
> 



Re: vmx(4): remove useless code

2021-08-06 Thread Theo de Raadt
Patrick Wildt  wrote:

> On Fri, Aug 06, 2021 at 11:05:53AM +0200, Patrick Wildt wrote:
> > Am Thu, Aug 05, 2021 at 02:33:01PM +0200 schrieb Jan Klemkow:
> > > Hi,
> > > 
> > > The following diff removes useless code from the driver.  As discussed
> > > here [1] and committed there [2], the hypervisor doesn't do anything
> > > with the data structures.  We even just set NULL to the pointer since
> > > the initial commit of vmx(4).  So, I guess it better to remove all of
> > > these.  The variables are bzero'd in vmxnet3_dma_allocmem() anyway.
> > > 
> > > OK?
> > 
> > My main concern was if the structs are getting zeroed correctly, but
> > they do, so that's fine.
> > 
> > That said, it looks like Linux sets the pointer to ~0ULL, not 0.  Should
> > we follow Linux' pattern there and do that as well?
> > 
> 
> Thinking about it a little more, I think we should do that as well.  And
> maybe explicitly set driver_data_len to 0 even though it's already zero.
> Basically for readability.

The better approach is to zero the whole struct, then fill in non-zero
fields... and fill in zero'd fields when doing so idiomaticaly helps
future code readers.

But if you avoid zero'ing the whole struct, then padding bytes in the
struct may be a problem, and depending on the allocator and where the
data is later passed to, leak.

So full structure zero is the best practice.



Re: Fix unsafe snmpd defaults

2021-08-06 Thread Theo de Raadt
Martijn van Duren  wrote:

> What about something like the phrasing below? It puts a heavy emphasis
> on not relying on the defaults (currently the auth and enc keyword
> aren't marked as optional in the syntax anyway), but keeps the current
> defaults as a strong hint on what is adviced. I also downscaled the
> example a little by setting seclevel auth, so it gives a little more
> substance to how seclevel can be used. Don't know if that's needed.

That is pretty ridiculous.

Shall we force users to select their ssh hmacs?  Or, maybe you don't
mean users, shall we force admins to select their sshd hmacs before
sshd will startup?

Defaults exist so that systems are easy to use.  This discussion has made
it clear that there is no security justification, it is simply a case
of murdering older hash algorithms EVEN IF THEY ARE SAFELY EMBEDDED INSIDE
A HMAC.

There are circumstances where the sha256 cult are wrong, and this is one
of them.



Re: Fix unsafe snmpd defaults

2021-08-06 Thread Theo de Raadt
Theo Buehler  wrote:

> > hmac-md5 might not be vulnerable, but snmp doesn't use pure hmac-*; in
> > the case of md5 and sha1 it strips the result back to 12 bytes (for
> > sha256 it's 24 bytes). I'm not saying that is insecure because of it; I
> > haven't seen any research on the truncation of HMAC, but when combined
> > with known problematic hashing algorithms it doesn't increase my
> > confidence level.
> 
> The hash used in an HMAC construction needs much weaker properties than
> a cryptographic hash (in particular no collision resistance). Basically,
> that's because there's still a private key involved that isn't known to
> the attacker. The brokenness of md5 and sha1 isn't strong enough yet to
> compromise hmac-md5 and hmac-sha1.
> 
> The rule of thumb is that you need to keep at least half the size of the
> hash in a truncated HMAC.Since SHA-1 has 160 bits and MD5 128 bits,
> keeping 12 bytes is fine.  This corresponds to "96 bits of security".
> This isn't all that great by today's standards, but far from completely
> bonkers.

to conclude, snmpd configuration should be defaulting to the *industry
standard* hmac that has highest interopability, because there is no
cause for being the first snmp stack to become non-interoperable.

The older hmac continues to satisfy the security requirement.

The newer mac is causing users problems, because no other systems are
configured thus, so you are simply adding an irrational hurdle to snmpd
use.

The addition of these advanced mac's to the spec is really just
ASPIRATIONAL -- they are added for future community use, and possible
mainstream adoption in the next decade.  There is nothing insecure to fix
here.  There is no mandate to push people towards maximally non-interoperable
defaults.  When stated this way, I hope you see it the same way.

I recommend changing it back to the industry standard.



Re: iked(8): Increase the default Child SA data lifetime limit

2021-08-02 Thread Theo de Raadt
Vitaliy Makkoveev  wrote:

> > ssh_packet_need_rekeying() appears to have some nice decisions.  The
> > idea is to rekey based upon time, primarily.
> 
> It does the same: the two limits and rekying starts when you exceeded
> any of them. But in the ssh case we have no massive traffic load, so we
> reach time limit first. The ipsec case is opposite.

First off, this is not ipsec.  This is a iked decision.  And thus the
iked case is only the opposite of ssh because iked has ludicrously small
data limits.  I doubt this choice was made purposefully.

Correct me if I am wrong about ssh:

debug1: rekey in after 134217728 blocks

Imagine AES is being used such that blocksize is 16.

So ssh will rekey at 2GB data, also note this is a non-datagram model, it
is a session that stops such that no traffic is lost.  And this 2G is
generally the data rekey decision for a flow by _a single user_.

But IPSEC is not moving a single user's data!  And as a result of this,
datagrams get lost.  Seems a poor tradeoff to lose packets when turning
on crypto.

So 2GB is a ludicrous position to take, very much like "the cipher being
used is unsafe".  Please explain how AES is suddenly this poor.

If there are unsafe cipher choices, then those specific unsafe ones
should make tighter datasize choices, but the modern ciphers we have are
much better so their choices should be huge.

I suspect the first step is to make the rekey decision be based upon the
strength of the ciphers.



Re: iked(8): Increase the default Child SA data lifetime limit

2021-08-02 Thread Theo de Raadt
Joerg Sonnenberger  wrote:

> On Tue, Aug 03, 2021 at 01:12:54AM +0300, Vitaliy Makkoveev wrote:
> > Index: sbin/iked/types.h
> > ===
> > RCS file: /cvs/src/sbin/iked/types.h,v
> > retrieving revision 1.43
> > diff -u -p -r1.43 types.h
> > --- sbin/iked/types.h   13 May 2021 15:20:48 -  1.43
> > +++ sbin/iked/types.h   2 Aug 2021 21:41:55 -
> > @@ -67,7 +67,7 @@
> >  #define IKED_CYCLE_BUFFERS 8   /* # of static buffers for mapping */
> >  #define IKED_PASSWORD_SIZE 256 /* limited by most EAP types */
> >  
> > -#define IKED_LIFETIME_BYTES536870912 /* 512 Mb */
> > +#define IKED_LIFETIME_BYTES1073741824 /* 512 Mb */
> >  #define IKED_LIFETIME_SECONDS  10800 /* 3 hours */
> >  
> >  #define IKED_E 0x1000  /* Decrypted flag */
> > 
> 
> Comment and value don't match? Also, isn't 512MB quite low with modern
> crypto algorithms?

I think the low values are exceedingly cynical, and should be increased
substantially.

ssh_packet_need_rekeying() appears to have some nice decisions.  The
idea is to rekey based upon time, primarily.



Re: dhcpleased vs gif(4) (and othes like that)

2021-08-01 Thread Theo de Raadt
Stuart Henderson  wrote:

> On 2021/08/01 12:57, Gregory Edigarov wrote:
> > Hello Everybody,
> > 
> > The very minimal change to make it work correctly:
> 
> Running configuration twice on all interfaces is not 'very minimal' and
> could easily cause more problems than it solves.

No kidding.



> Most use of gif(4) doesn't need this. Is this so you can use hostnames
> in the files, or something else?


Right.  The proposal lacks a clear problem statement.




Re: gettimeofday.2: miscellaneous cleanup and rewrites

2021-07-30 Thread Theo de Raadt
Todd C. Miller  wrote:

> I really don't like "get or set the UTC time" in the NAME section.
> It's not like we have a "get or set the local time" function.
> I would prefer if you left the UTC information for the DESCRIPTION.

I agree.

Excessive obvious detail detracts from simple points.



Re: ftp.1: -o and multiple files

2021-07-28 Thread Theo de Raadt
Klemens Nanni  wrote:

> With the current wording, I'd either expect usage error when passing
> `-o output' or more wording explaining the behaviour when fetching
> multiple files.
> 
> Usage error seems wrong since our synopsis explicitly allows it, but
> writing the contents behind multiple URLs into the same file, while
> truncating the file for each URL does not make much sense, either.
> 
> 
> I came across this as I wanted to fetch multiple URLs at once without
> scripting, looked at ftp(1) and didn't found anything to do that.
> Tried `-o' with multiple URLs to see how it behaves as that case isn't
> documented.
> 
> So should we at least go with this correction?
> 
> Index: ftp.1
> ===
> RCS file: /cvs/src/usr.bin/ftp/ftp.1,v
> retrieving revision 1.122
> diff -u -p -r1.122 ftp.1
> --- ftp.1 2 Feb 2021 12:58:42 -   1.122
> +++ ftp.1 28 Jul 2021 15:46:19 -
> @@ -212,7 +212,7 @@ will prompt for the remote machine login
>  identity on the local machine) and, if necessary, prompt for a password
>  and an account with which to log in.
>  .It Fl o Ar output
> -When fetching a single file or URL, save the contents in
> +When fetching files or URLs, save the contents in
>  .Ar output .
>  To make the contents go to stdout,
>  use
> 

So it concatenates the output to the one file?  Fine.  But we should
also block users from more than one -o option.

Index: main.c
===
RCS file: /cvs/src/usr.bin/ftp/main.c,v
retrieving revision 1.138
diff -u -p -u -r1.138 main.c
--- main.c  14 Jul 2021 13:33:57 -  1.138
+++ main.c  28 Jul 2021 16:31:31 -
@@ -501,6 +501,8 @@ main(volatile int argc, char *argv[])
break;
 
case 'o':
+   if (outfile)
+   usage();
outfile = optarg;
if (*outfile == '\0') {
pipeout = 0;



Re: sleep.3: miscellaneous cleanup and rewrites

2021-07-25 Thread Theo de Raadt
Scott Cheloha  wrote:

> Given this, I want to tell the reader, roughly:
> 
>   "hey!  it's plausible there is a SIGALRM-based sleep() implementation
>using still floating around out there in the wild.  If you find one,
>you'll want to avoid using it because there are unfixable bugs in
>such an implementation.  Maybe use nanosleep() instead.  If you *do*
>use it, just know that it will behave differently from OpenBSD's
>sleep() in some corner cases."
> 
> But if you really think there is no point in mentioning that, and
> others agree with you, then we won't mention it.

I don't think the manual pages need to be proscriptive about a concern
which doesn't occur in the wild.

Being proscriptive in OpenBSD manual pages isn't going to stop someone
from creating the precise problem you describe in some other body of
code.  Except, you are saying they don't create that problem.

So why foam at the mouth over it?






Re: sleep.3: miscellaneous cleanup and rewrites

2021-07-25 Thread Theo de Raadt
Scott Cheloha  wrote:

> On Sun, Jul 25, 2021 at 01:35:19PM -0600, Theo de Raadt wrote:
> > This is a lot of fuss.
> > 
> > How many bugs have you found relating to this issue?
> > 
> > Let me guess: zero?
> 
> Right, that's why I'm asking if we need to make a portability
> recommendation at all.
> 
> We could also say something like:
> 
>   The standard permits sleep() implementations based on
>   alarm(3).  This implementation does not use alarm(3).
> 
> ... or we could say nothing about SIGALRM.  I don't know.

Have you found any impacted implimentations?  No.

Why bother documenting a gotcha which noone has ever made?



Re: sleep.3: miscellaneous cleanup and rewrites

2021-07-25 Thread Theo de Raadt
This is a lot of fuss.

How many bugs have you found relating to this issue?

Let me guess: zero?





Scott Cheloha  wrote:

> On Sun, Jul 25, 2021 at 08:15:34AM +0100, Jason McIntyre wrote:
> > On Sat, Jul 24, 2021 at 10:39:49PM -0500, Scott Cheloha wrote:
> > > Okay, the nanosleep.2 changes are committed, let's do sleep.3 next.
> > 
> > hi.
> > 
> > the changes read fine to me. only one comment:
> > 
> > > [...]
> > > 
> > > STANDARDS
> > > 
> > > - Bump the relevant standard to POSIX.1-2001.  That version
> > >   incorporates details about threads, which our version respects.
> > > 
> > >   For sake of completeness, I will note that SUSv2 mentions threads
> > >   too:
> > > 
> > >   https://pubs.opengroup.org/onlinepubs/7908799/xsh/sleep.html
> > > 
> > >   SUSv2 is older than POSIX.1-2001.  Do we prefer the earlier
> > >   standard?
> > > 
> > 
> > no, we tend to reference the latest spec.
> 
> So, should I just say we conform to POSIX.1-2008?
> 
> There were no changes to the sleep() spec in that edition relevant to
> our implementation.  If sleep() is based on nanosleep() the spec
> hasn't changed in any meaningful way since 2001.  All the changes
> since then are about the SIGALRM approach, which seems to be on its
> way out.
> 



Re: ntpd: Remove -sS compat

2021-07-24 Thread Theo de Raadt
If you delete this, people's ntpd will fail to start.

getopt will fail to match, drop into usage, and fail.

It is way too early to cause that problem.  Couple more years I think.



Klemens Nanni  wrote:

> On Mon, Jul 12, 2021 at 09:08:59PM +, Klemens Nanni wrote:
> > deraadt neutered these options in november 2019, I'd say it's time to
> > remove them so the next release won't have it.
> > 
> > Feedback? Objections? OK?
> 
> Ping.
> ntpd(8) obviously doesn't document them any longer.
> 
> Index: ntpd.c
> ===
> RCS file: /cvs/src/usr.sbin/ntpd/ntpd.c,v
> retrieving revision 1.132
> diff -u -p -r1.132 ntpd.c
> --- ntpd.c16 Jul 2021 14:36:09 -  1.132
> +++ ntpd.c24 Jul 2021 19:45:34 -
> @@ -140,7 +140,6 @@ main(int argc, char *argv[])
>   char**argv0 = argv;
>   char*pname = NULL;
>   time_t   settime_deadline;
> - int  sopt = 0;
>  
>   if (strcmp(__progname, "ntpctl") == 0) {
>   ctl_main(argc, argv);
> @@ -151,7 +150,7 @@ main(int argc, char *argv[])
>  
>   memset(, 0, sizeof(lconf));
>  
> - while ((ch = getopt(argc, argv, "df:nP:sSv")) != -1) {
> + while ((ch = getopt(argc, argv, "df:nP:v")) != -1) {
>   switch (ch) {
>   case 'd':
>   lconf.debug = 1;
> @@ -166,10 +165,6 @@ main(int argc, char *argv[])
>   case 'P':
>   pname = optarg;
>   break;
> - case 's':
> - case 'S':
> - sopt = ch;
> - break;
>   case 'v':
>   lconf.verbose++;
>   break;
> @@ -185,12 +180,6 @@ main(int argc, char *argv[])
>   logdest |= LOG_TO_SYSLOG;
>  
>   log_init(logdest, lconf.verbose, LOG_DAEMON);
> -
> - if (sopt) {
> - log_warnx("-%c option no longer works and will be removed 
> soon.",
> - sopt);
> - log_warnx("Please reconfigure to use constraints or trusted 
> servers.");
> - }
>  
>   argc -= optind;
>   argv += optind;
> 



Re: nanosleep.2: miscellaneous cleanup

2021-07-20 Thread Theo de Raadt
Scott Cheloha  wrote:

> I'm not wed to keeping all the examples.  What if we merge examples 1
> and 2 and dumb the result down a bit?

Are you teaching people how to fill a structure, how to perform a loop,
or that they need to check errno?

I don't see the point of having any example.

Do we need an example in the close(2) manual page next?



Re: nanosleep.2: miscellaneous cleanup

2021-07-20 Thread Theo de Raadt
>  [EFAULT]  foo points outside the process's allocated address space.
>
> But i don't really i like that.  The word "allocated" makes me wonder
> because it sounds too much like malloc(3) for my taste.
> Usually, pointers to automatic and to static objects are acceptable,
> too, and are those "allocated"?  Some might say they are not.
> Besides, "process's" looks awkward.

Disagree on your dislike of this wording.

A process has an address space, VM_MIN_ADDRESS to VM_MAXUSER_ADDRESS.
Parts of this are not mapped, because nothing has allocated backing
resources.  Allocation happens via stack growth, execve, shm, mmap, etc
etc etc etc.  Those are all methods for "allocating" within the
processes' address space.

Unallocated space generates EFAULT.

Addresses outside the VM_MIN_ADDRESS to VM_MAXUSER_ADDRESS range also
generate EFAULT, and this is because backing resources are not permitted
to be allocated there.  No allocation, thus EFAULT.

"allocated" is correct terminology, and it has nothing to do with
malloc.

I don't see how trying to mince words is going to help anyone.

Unifying all the varients into one form will be quite a task.
I don't know if this is the right form to use, but I don't like the
argument you made.



Re: nanosleep.2: miscellaneous cleanup

2021-07-20 Thread Theo de Raadt
I think this is excessively verbose for such a simple function, especially
the addition of examples. 

There are many APIs which are hundreds of times more complicated which
don't have this level of detailing, and I would argue such manual pages
have the correct tone and complexity.

It stops feeling like a historically+adapted BSD manual page.



Re: dhcpleased/resolved: omit ISP DNS in DHCP offer

2021-07-18 Thread Theo de Raadt
stolen data  wrote:

> After reading the man pages of the replacements for dhclient(8) it looks
> like it will no longer be possible to prevent DNS entries in a DHCP offer
> from ending up in the resolv.conf - as is practically always the case with
> a residential Internet connection with DHCP. I don't want my ISP's DNS
> servers *anywhere* in my resolv.conf, so I tell dhclient(8) to SUPERSEDE
> DOMAIN-NAME-SERVERS, but now it looks like OpenBSD will no longer allow me
> the choice and instead decides for me that DNS entries in DHCPOFFER must
> always be included and made available for eventual use. Why? This is taking
> control out of the hands of users and I cannot see any sense in the
> rationale.

What you want is trivial, using unwind(8) and unwind.conf(8)

You comments ooze of drama and noone is appreciating it.  If you don't
like what we are doing to our operating system for ourselves, either
stop the insulting tone or run some other operating system.

Sadly, you have have control over how things work unless you write your
own complete operating system, or maintain large diffs to another system
on a continual basis.  You are only a whining user -- every single
operating system group will treat you exactly the same.  Either you get
used to dissapointment, or you adapt and your use of a PREVIOUS KNOBSET
("dhclient + supersede") to a NEW KNOBSET ("dhcpleased + resolvd + unwind +
unwind.conf with/without forwarder or preference + operating on 127.0.0.1)
which we firmly believe is a better solution for the average case.

Additionally, unwind performs opportunistic DNSSEC and other preventative
validations.  What is not to like?

At this point you should be incredibly gracious you even get a reply which
provides alternative ways of doing things.



Re: cron(8): add '@' interval time specifier

2021-07-10 Thread Theo de Raadt
Theo de Raadt  wrote:

> It will do 5 minutes of compute, 15 minutes of pause, and do it all
> again.

A recent rpki-client took longer:

# Processing time 989 seconds (199 seconds user, 155 seconds system)

So that would be 6 minutes cpu, over 16 minutes elapsed.

Followed by a 15 minute pause.  Then do it again?

I am worried by where this is heading.



Re: cron(8): add '@' interval time specifier

2021-07-10 Thread Theo de Raadt
Job Snijders  wrote:

> A use case could be running rpki-client more frequently than once an
> hour:

I want to say I think running rpki-client so often is misguided for
these rpki-processing-on-the-router configurations.

rpki-processing-on-the-router seems like more a technology demonstration
than performant practical solution.  A rpki-client run is a lot of x509
compute, roughly 5 minutes of full cpu.  After cron has this support, I
suspect you will propose that root's crontab be changed to use the
feature, thus further increasing the compute done directly on a router.
It will do 5 minutes of compute, 15 minutes of pause, and do it all
again.  This will have impact on traffic flow through the OpenBSD
software routing stack.

Surely the on-router approach is only acceptable in very narrow
circumstances, and those situations don't need to be updating RPKI every
20 minutes, as I doubt the internet at large is doing anything close to
20 minute updates.

The normal RPKI use pattern should be offline processing (on a different
dedicated system), then uploaded to a router.

How often RPKI is run should be sensitively scaled to the resources
available.

Are we actually helping people by proposing they use 5 minutes of total
cpu compute, every 20 minutes?  I have serious doubts.

That then begs the question why cron needs this extension for this purpose.
 
> @15 -n rpki-client && bgpctl reload
> 
> The above is equivalent to:
> 
> * * * * * -sn sleep 900 && rpki-client && bgpctl reload

Somewhat the same effect, but I think there are big differences in
behaviour.  The second version creates a process hierarchy which sleeps
for a long time.  I suspect crontab -e causes drastic difference in
behaviour when the job is already running.

I hate the sleep pattern, because you can't see what is happening from a
system perspective (ie. in ps), and if root decides to run rpki-client
or various bgpctl commands by hand, you can get surprising results.

So I agree with getting away from the sleep pattern.  I have never used
it myself, and we should discourage others from using it by providing
something better.

> I borrowed the idea from FreeBSD's cron [1]. A difference between the
> below changeset and the freebsd implementation is that they specify the
> interval in seconds, while the below specifies in minutes.

Why be incompatible?  Better have a strong justification.




Re: human-readable audio device descriptions

2021-07-07 Thread Theo de Raadt
> This diff is an attempt to make identifying audio devices easier by
> giving each device a human-readable description that can be read from
> userspace (and without scraping dmesg).

But why does anyone want that?

Isn't everyone served best when there is a public portable interface,
and all the back-ends must try to work the same?



Re: netlock ktrace nfs

2021-07-05 Thread Theo de Raadt
Alexander Bluhm  wrote:

> pledge(2) and so_state SS_DNS are special.  There is no real risk
> of a race and the flag is set only at socket creation.

Yes, this looks safe to me.  ps_flags PS_PLEDGE and ps_pledge are only
set inside locked pledge(), which makes the pledge_socket() SS_DNS check
non-racy



Re: systat(1) counter overflow

2021-07-02 Thread Theo de Raadt
Claudio Jeker  wrote:

> I know that golang has its definition of uvmexp and so if you change the
> ABI then you would break at least that. struct uvmexp is used more often
> than we would like.


It is a huge ABI change.  If we are going to change the size of the subunits,
we have to get it right in one step.

Are there issues with using 64-bit types for some fields on the non-atomic
architectures?



Re: style(9) identifier alignment

2021-07-01 Thread Theo de Raadt
I disagree with this change.  I don't see that specific indent as
prevailing.  But more importantly, the specification is overly
proscriptive.

Developers type a variety of things in their variable declaration
blocks, some of this is due to editor assist, and being exceedingly
strict about a few spaces in declarations won't actually help their
fingers or later readers of the code.

style(9)'s focus is really more about codeblocks layout: It encourages a
vertically dense layout which remains easy to read/review.  The most
important aspect is the tab + 4space seperation at certain line-wrap
conditions, in particular it makes complex condition blocks seperate out
nicely from the following code, and this is easy on (our) reviewer's eye.

The principle style should follow is encourage code to look similar, so
that our eyes read it faster.  But this is a 'soft similar' (encourage,
similar enough the eye doesn't wander) not a 'hard similar' (must do it
thus), and I've pushed back against previous style.9 diffs which make
this page into a harder document.


Charlie Burnett  wrote:

> Hi @tech,
> 
> Currently the prevailing style in the kernel seems to have identifiers
> (minus
> their pointers) aligned. style(9) however indicates that the first
> qualifiers
> should be aligned. This patch fixes the style guide to match the predominant
> style. I tried to give it a bit more of an OpenBSD flavor :) I also have a
> .clang-format file that very carefully fits the style guide as such,
> however a
> bug in clang-format that's fixed in clang 13 prevents it from being much
> use,
> but I can submit it nonetheless!
> 
> Best Regards,
> Charlie Burnett
> 
> Index: style.9
> ===
> RCS file: /cvs/src/share/man/man9/style.9,v
> retrieving revision 1.77
> diff -u -p -r1.77 style.9
> --- style.9 22 Jan 2021 14:13:57 - 1.77
> +++ style.9 1 Jul 2021 04:39:25 -
> @@ -202,13 +202,18 @@ Major structures should be declared at t
>  are used, or in separate header files if they are used in multiple
>  source files.
>  Use of the structures should be by separate declarations and should be
> -.Dq Li extern
> -if they are declared in a header file.
> +.Dq Li extern
> +if they are declared in a header file. Consecutive declarations should be
> +formatted such that the column of the rightmost character in the
> Consecutive
> +declarations should be formatted such that the furthest left character of
> the
> +identifier (pointers and references included) is at least one space away
> from
> +the furthest right qualifier.
>  .Bd -literal -offset indent
> -struct foo {
> - struct foo *next; /* List of active foo */
> - struct mumble amumble; /* Comment for mumble */
> - int bar;
> +struct humppa {
> + struct song  *next; /* Pointer to the next song */
> + struct band   band_info; /* Comment for author */
> + const char  **lyrics; /* Array of song lines */
> + int  nlines;
>  };
>  struct foo *foohead; /* Head of global foo list */
>  .Ed



Re: ualarm.3: cleanup

2021-07-01 Thread Theo de Raadt
  Calling more than one of the interfaces setitimer(2), alarm(3),
  and ualarm(3) in the same program results in unspecified behaviour.

I don't think it is unspecified.  It will behave precisely as
expected...



  1   2   3   4   5   6   7   8   9   10   >