Re: accton(8) requires a reboot after being enabled

2020-10-30 Thread Theo de Raadt
Yes, that diff is a whole bunch of TOCTUO.

If this was going to be changed, it should be in the kernel.

But I don't know if it should be changed yet, which is why I asked
a bunch of questions.

Stepping back to the man page change, we could decide that accton
should continue to behave how it does, and this conversation started
because the man page fell into the trap of documenting the rc scripts
RATHER than documenting accton.



Re: accton(8) requires a reboot after being enabled

2020-10-30 Thread Theo de Raadt
Jason McIntyre  wrote:

> On Fri, Oct 30, 2020 at 04:24:43PM +0100, Solene Rapenne wrote:
> > reading accton(8) it's not clear that if you
> > enable it you need to restart the system for
> > accounting to be effective.
> > 
> > Here is a change to add the explanation, but
> > I'm not sure if the wording is correct.
> > 
> 
> hi. i think the text that follows is really trying to say the same thing
> (you enable it at boot, so until you boot it isn;t enabled). we could
> just try to make that one bit of text a bit clearer:
> 
>   .Nm
>   should be enabled at boot time, either by running
>   .Dq rcctl enable accounting
>   or by setting
>   .Dq accounting=YES
>   in
>   .Xr rc.conf.local 8 .

With a careful reading of the current manual page, everything is there
and it is accurate.

With an argument naming an existing file
   

Ok so let's create it with touch.  Probably has wrong permissions.
But now accton to that file works.  Or enable it and reboot, and now
disable it and reboot, and the file still exists, so now accton works
because it is an existing file (with the right permissions I guess).

So it *IS* working as documented.  It is just a bit weird, because the
accton command (and system call) do not create the file.


My problem with these changes is this is the man page for the accton(8)
command, it documents *the binary*.  The manpage has already been subverted
talk about rcctl, and about how /etc/rc runs the command.  But the man
page should first and foremost be about the command, not about /etc/rc
and rcctl, am i not right?  For instance, the ntpd manual page has a tiny
section about rc.conf.

So in conclusion, I think both of you are writing text about the startup
subsystem, into the wrong manual page.  I think both proposals are skewed.

So questions.

1 - historically it requires a file to be pre-created.  In the rc scripts,
this is a touch.  That grabs the umask and ownership of root's run of
/etc/rc.
2 - could we do better, in some way?
3 - accton system call does not have a umask, is that where this design came
from?
4 - could we improve upon this?
5 - could accton always (attempt to) create the file, without harming existing
use cases, with proper owner and chmod flags?
6 - or should that be tied to a flag, making it easier to document?

> sth like that?
> jmc
> 
> > Index: accton.8
> > ===
> > RCS file: /home/reposync/src/usr.sbin/accton/accton.8,v
> > retrieving revision 1.11
> > diff -u -p -r1.11 accton.8
> > --- accton.810 Nov 2019 20:51:53 -  1.11
> > +++ accton.830 Oct 2020 15:22:14 -
> > @@ -43,6 +43,10 @@ causes system accounting information for
> >  to be placed at the end of the file.
> >  If no argument is given, accounting is turned off.
> >  .Pp
> > +Accounting is done if it was enabled when system booted.
> > +If you activate accounting, a reboot is required for it to become
> > +effective.
> > +.Pp
> >  To have
> >  .Nm
> >  enabled at boot time, use
> > 
> 



Re: snmpd(8) Remove old listen on syntax

2020-10-29 Thread Theo de Raadt
ok deraadt

Martijn van Duren  wrote:

> 6.8 has been released. Time to remove the old syntax.
> 
> OK?
> 
> martijn@
> 
> ? dispatcher_tm_udp.c
> Index: parse.y
> ===
> RCS file: /cvs/src/usr.sbin/snmpd/parse.y,v
> retrieving revision 1.61
> diff -u -p -r1.61 parse.y
> --- parse.y   10 Sep 2020 17:54:47 -  1.61
> +++ parse.y   29 Oct 2020 15:14:24 -
> @@ -130,7 +130,7 @@ typedef struct {
>  %token NUMBER
>  %type  hostcmn
>  %type  srcaddr port
> -%type  optwrite yesno seclevel proto
> +%type  optwrite yesno seclevel
>  %typeobjtype cmd
>  %type oid hostoid trapoid
>  %typeauth
> @@ -279,7 +279,7 @@ main  : LISTEN ON listenproto
>  
>  listenproto  : UDP listen_udp
>   | TCP listen_tcp
> - | listen_empty
> + | listen_udp
>  
>  listen_udp   : STRING port   {
>   struct sockaddr_storage ss[16];
> @@ -335,34 +335,6 @@ listen_tcp   : STRING port   {
>   }
>   }
>  
> -/* Remove after deprecation period and replace with listen_udp */
> -listen_empty : STRING port proto {
> - struct sockaddr_storage ss[16];
> - int nhosts, i;
> -
> - nhosts = host($1, $2, $3, ss, nitems(ss));
> - if (nhosts < 1) {
> - yyerror("invalid address: %s", $1);
> - free($1);
> - if ($2 != snmpd_port)
> - free($2);
> - YYERROR;
> - }
> - if (nhosts > (int)nitems(ss))
> - log_warn("%s:%s resolves to more than %zu 
> hosts",
> - $1, $2, nitems(ss));
> -
> - free($1);
> - if ($2 != snmpd_port)
> - free($2);
> - for (i = 0; i < nhosts; i++) {
> - if (listen_add(&(ss[i]), $3) == -1) {
> - yyerror("calloc");
> - YYERROR;
> - }
> - }
> - }
> -
>  port : /* empty */   {
>   $$ = snmpd_port;
>   }
> @@ -385,21 +357,6 @@ port : /* empty */   {
>   YYERROR;
>   }
>   $$ = number;
> - }
> - ;
> -
> -proto: /* empty */   {
> - $$ = SOCK_DGRAM;
> - }
> - | UDP   {
> - log_warnx("udp as last keyword on listen on line is "
> - "deprecated");
> - $$ = SOCK_DGRAM;
> - }
> - | TCP   {
> - log_warnx("tcp as last keyword on listen on line is "
> - "deprecated");
> - $$ = SOCK_STREAM;
>   }
>   ;
>  
> 



Re: [PATCH] Macbook1,1 - minimal diff to swap 2 key for ISO internal kbd

2020-10-28 Thread Theo de Raadt
Yes to all of that, the better names also.


Mark Kettenis  wrote:

> > From: Mathias Schmocker 
> > Date: Wed, 28 Oct 2020 20:54:13 +0100
> > 
> > Hello,
> > Here a minimal diff to solve the swapped keys of the internal ISO 
> > keyboard/trackpad found on my older Macbook1,1 13inch black laptop
> > (6.8 i386 release)
> > cvs diff: Diffing .
> > Index: ukbd.c
> > ===
> > RCS file: /home/cvs/src/sys/dev/usb/ukbd.c,v
> > retrieving revision 1.79
> > diff -u -p -r1.79 ukbd.c
> > --- ukbd.c  23 Aug 2020 11:08:02 -  1.79
> > +++ ukbd.c  27 Oct 2020 19:18:51 -
> > @@ -259,6 +259,7 @@ ukbd_attach(struct device *parent, struc
> > case USB_PRODUCT_APPLE_GEYSER_ISO:
> > case USB_PRODUCT_APPLE_WELLSPRING6_ISO:
> > case USB_PRODUCT_APPLE_WELLSPRING8_ISO:
> > +   case USB_PRODUCT_APPLE_INT_KBTP_218_ISO:
> > sc->sc_munge = ukbd_apple_iso_munge;
> > break;
> > case USB_PRODUCT_APPLE_WELLSPRING_ISO:
> > Index: usbdevs
> > ===
> > RCS file: /home/cvs/src/sys/dev/usb/usbdevs,v
> > retrieving revision 1.720
> > diff -u -p -r1.720 usbdevs
> > --- usbdevs 3 Aug 2020 14:25:44 -   1.720
> > +++ usbdevs 27 Oct 2020 19:24:15 -
> > @@ -941,6 +941,7 @@ product APPLE FOUNTAIN_ANSI 0x020e  Keybo
> >   product APPLE FOUNTAIN_ISO0x020f  Keyboard/Trackpad
> >   product APPLE GEYSER_ANSI 0x0214  Keyboard/Trackpad
> >   product APPLE GEYSER_ISO  0x0215  Keyboard/Trackpad
> > +product APPLE INT_KBTP_218_ISO 0x0218  Keyboard/Trackpad
> >   product APPLE WELLSPRING_ANSI 0x0223  Keyboard/Trackpad
> >   product APPLE WELLSPRING_ISO  0x0224  Keyboard/Trackpad
> >   product APPLE WELLSPRING_JIS  0x0225  Keyboard/Trackpad
> > 
> > With this, the sf keyboard maps correcly the less/greater and the 
> > (swiss-french and swiss-german) section/degree key.
> > On the console and on with the X-server.
> > 
> 
> NetBSD has a better name for the device I think.  So I propose the
> diff below which also adds a few more missing device IDs in the range.
> 
> ok?
> 
> 
> Index: dev/usb/usbdevs
> ===
> RCS file: /cvs/src/sys/dev/usb/usbdevs,v
> retrieving revision 1.724
> diff -u -p -r1.724 usbdevs
> --- dev/usb/usbdevs   25 Oct 2020 09:03:01 -  1.724
> +++ dev/usb/usbdevs   28 Oct 2020 20:29:58 -
> @@ -951,6 +951,13 @@ product APPLE FOUNTAIN_ANSI  0x020e  Keybo
>  product APPLE FOUNTAIN_ISO   0x020f  Keyboard/Trackpad
>  product APPLE GEYSER_ANSI0x0214  Keyboard/Trackpad
>  product APPLE GEYSER_ISO 0x0215  Keyboard/Trackpad
> +product APPLE GEYSER_JIS 0x0216  Keyboard/Trackpad
> +product APPLE GEYSER3_ANSI   0x0217  Keyboard/Trackpad
> +product APPLE GEYSER3_ISO0x0218  Keyboard/Trackpad
> +product APPLE GEYSER3_JIS0x0219  Keyboard/Trackpad
> +product APPLE GEYSER4_ANSI   0x021a  Keyboard/Trackpad
> +product APPLE GEYSER4_ISO0x021b  Keyboard/Trackpad
> +product APPLE GEYSER4_JIS0x021c  Keyboard/Trackpad
>  product APPLE WELLSPRING_ANSI0x0223  Keyboard/Trackpad
>  product APPLE WELLSPRING_ISO 0x0224  Keyboard/Trackpad
>  product APPLE WELLSPRING_JIS 0x0225  Keyboard/Trackpad
> Index: dev/usb/ukbd.c
> ===
> RCS file: /cvs/src/sys/dev/usb/ukbd.c,v
> retrieving revision 1.79
> diff -u -p -r1.79 ukbd.c
> --- dev/usb/ukbd.c23 Aug 2020 11:08:02 -  1.79
> +++ dev/usb/ukbd.c28 Oct 2020 20:29:58 -
> @@ -257,6 +257,7 @@ ukbd_attach(struct device *parent, struc
>   switch (uha->uaa->product) {
>   case USB_PRODUCT_APPLE_FOUNTAIN_ISO:
>   case USB_PRODUCT_APPLE_GEYSER_ISO:
> + case USB_PRODUCT_APPLE_GEYSER3_ISO:
>   case USB_PRODUCT_APPLE_WELLSPRING6_ISO:
>   case USB_PRODUCT_APPLE_WELLSPRING8_ISO:
>   sc->sc_munge = ukbd_apple_iso_munge;
> 



Re: RFC: kern.video.record

2020-10-24 Thread Theo de Raadt
Seems better.

Toggling the behaviour while video occurs should be tested, to make
sure it matches expectations.


Marcus Glocker  wrote:

> On Sat, Oct 24, 2020 at 11:34:07AM -0600, Theo de Raadt wrote:
> 
> > Marcus Glocker  wrote:
> > 
> > > On Sun, Sep 27, 2020 at 05:57:51PM +0100, Laurence Tratt wrote:
> > > 
> > > > On Sun, Sep 13, 2020 at 09:23:36AM +0100, Laurence Tratt wrote:
> > > > 
> > > > > Since I recently opened my big fat mouth and suggested that
> > > > > "kern.video.record" (analogous to kern.audio.record) might be a good 
> > > > > idea, I
> > > > > decided to put together a quick prototype (heavily based on the
> > > > > kern.audio.record code). This at least roughly works for me but 
> > > > > raises some
> > > > > questions such as:
> > > > >
> > > > >   * Is uvideo the only driver that can capture video? [I imagine not, 
> > > > > but I
> > > > > don't really know.]
> > > 
> > > utvfu(4) comes in mind.
> > 
> > And therefore, it makes no sense the diff contains change to uvideo(4)
> > only.  I know video(4) is a thin shim, but the blocking logic isn't
> > correctly placed.
> 
> I agree.
> 
> Why don't we simply deny turning on the cams video stream when
> kern.video.record=0 in video(1)?  See diff.
> 
>   imac:~ [hacki] $ doas video
>   video: ioctl VIDIOC_STREAMON: Operation not permitted
> 
> > One more comment.  If the bcopy's are skipped, what data is in those
> > buffers?  Are they zero'd freshly, or allocated with a M_ZERO type
> > allocation, or what's the story?  Or if one toggles the control do
> > the buffers continue to show old records rather than 'black'?
> 
> The video buffers aren't initialized ever in uvideo(4).
> But with the above suggestion, we don't need to worry about the buffers
> anymore, they simply won't get passed (nor even filled).
> 
> 
> Index: video.c
> ===
> RCS file: /cvs/src/sys/dev/video.c,v
> retrieving revision 1.44
> diff -u -p -u -p -r1.44 video.c
> --- video.c   16 May 2020 10:47:22 -  1.44
> +++ video.c   24 Oct 2020 19:08:28 -
> @@ -40,6 +40,12 @@
>  #define DPRINTF(x)
>  #endif
>  
> +/*
> + * Global flag to control if audio recording is enabled when the
> + * mixerctl setting is record.enable=sysctl
> + */
> +int video_record_enable = 0;
> +
>  struct video_softc {
>   struct devicedev;
>   void*hw_hdl;/* hardware driver handle */
> @@ -159,6 +165,9 @@ videoread(dev_t dev, struct uio *uio, in
>   int unit, error;
>   size_t size;
>  
> + if (!video_record_enable)
> + return (EPERM);
> +
>   unit = VIDEOUNIT(dev);
>   if (unit >= video_cd.cd_ndevs ||
>   (sc = video_cd.cd_devs[unit]) == NULL)
> @@ -302,6 +311,8 @@ videoioctl(dev_t dev, u_long cmd, caddr_
>   sc->sc_frames_ready--;
>   break;
>   case VIDIOC_STREAMON:
> + if (!video_record_enable)
> + return (EPERM);
>   if (sc->hw_if->streamon)
>   error = (sc->hw_if->streamon)(sc->hw_hdl,
>   (int)*data);



Re: RFC: kern.video.record

2020-10-24 Thread Theo de Raadt
Marcus Glocker  wrote:

> On Sun, Sep 27, 2020 at 05:57:51PM +0100, Laurence Tratt wrote:
> 
> > On Sun, Sep 13, 2020 at 09:23:36AM +0100, Laurence Tratt wrote:
> > 
> > > Since I recently opened my big fat mouth and suggested that
> > > "kern.video.record" (analogous to kern.audio.record) might be a good 
> > > idea, I
> > > decided to put together a quick prototype (heavily based on the
> > > kern.audio.record code). This at least roughly works for me but raises 
> > > some
> > > questions such as:
> > >
> > >   * Is uvideo the only driver that can capture video? [I imagine not, but 
> > > I
> > > don't really know.]
> 
> utvfu(4) comes in mind.

And therefore, it makes no sense the diff contains change to uvideo(4)
only.  I know video(4) is a thin shim, but the blocking logic isn't
correctly placed.

One more comment.  If the bcopy's are skipped, what data is in those
buffers?  Are they zero'd freshly, or allocated with a M_ZERO type
allocation, or what's the story?  Or if one toggles the control do
the buffers continue to show old records rather than 'black'?



Re: ping(8) put hop_limit warning in verbose mode

2020-10-20 Thread Theo de Raadt
I believe most of the new local variables you added in main,
can instead be added in the flush loop you wrote, even if you
have to instantiate them.  Or move it into a new function,
then it is even easier.  their scope is just too large...

> @@ -240,6 +240,17 @@ void  pr_retip6(struct ip6_hdr *, 
> u_char *);
>  int
>  main(int argc, char *argv[])
>  {
> + struct msghdr   m;
> + union {
> + struct cmsghdr hdr;
> + u_char buf[CMSG_SPACE(1024)];
> + }   cmsgbuf;
> + struct ioveciov[1];
> + struct pollfd   pfd;
> + struct sockaddr_in  peer4;
> + struct sockaddr_in6 peer6;
> + ssize_t cc;
> +
>   struct addrinfo hints, *res;
>   struct itimerval itimer;
>   struct sockaddr *from, *dst;
> @@ -786,6 +797,44 @@ main(int argc, char *argv[])
>   smsghdr.msg_iov = 
>   smsghdr.msg_iovlen = 1;
>  
> + if (v6flag) {
> + m.msg_name = 
> + m.msg_namelen = sizeof(peer6);
> + } else {
> + m.msg_name = 
> + m.msg_namelen = sizeof(peer4);
> + }
> + memset(, 0, sizeof(iov));
> + iov[0].iov_base = (caddr_t)packet;
> + iov[0].iov_len = packlen;
> +
> + /* Drain our socket. */
> + (void)signal(SIGALRM, onsignal);
> + memset(, 0, sizeof(itimer));
> + itimer.it_value.tv_sec = 1; /* make sure we don't get stuck */
> + (void)setitimer(ITIMER_REAL, , NULL);
> + for (;;) {
> + if (seenalrm)
> + break;
> +
> + pfd.fd = s;
> + pfd.events = POLLIN;
> +
> + if (poll(, 1, 0) <= 0)
> + break;
> +
> + m.msg_iov = iov;
> + m.msg_iovlen = 1;
> + m.msg_control = (caddr_t)
> + m.msg_controllen = sizeof(cmsgbuf.buf);
> +
> + cc = recvmsg(s, , 0);
> + if (cc == -1 && errno != EINTR)
> + break;
> + }
> + memset(, 0, sizeof(itimer));
> + (void)setitimer(ITIMER_REAL, , NULL);
> +
>   while (preload--)   /* Fire off them quickies. */
>   pinger(s);
>  
> @@ -805,17 +854,7 @@ main(int argc, char *argv[])
>   seeninfo = 0;
>  
>   for (;;) {
> - struct msghdr   m;
> - union {
> - struct cmsghdr hdr;
> - u_char buf[CMSG_SPACE(1024)];
> - }   cmsgbuf;
> - struct ioveciov[1];
> - struct pollfd   pfd;
> - struct sockaddr_in  peer4;
> - struct sockaddr_in6 peer6;
> - ssize_t cc;
> - int timeout;
> + int timeout;
>  
>   /* signal handling */
>   if (seenint)
> @@ -864,16 +903,6 @@ main(int argc, char *argv[])
>   if (poll(, 1, timeout) <= 0)
>   continue;
>  
> - if (v6flag) {
> - m.msg_name = 
> - m.msg_namelen = sizeof(peer6);
> - } else {
> - m.msg_name = 
> - m.msg_namelen = sizeof(peer4);
> - }
> - memset(, 0, sizeof(iov));
> - iov[0].iov_base = (caddr_t)packet;
> - iov[0].iov_len = packlen;
>   m.msg_iov = iov;
>   m.msg_iovlen = 1;
>   m.msg_control = (caddr_t)
> 
> 
> -- 
> I'm not entirely sure you are real.
> 



Re: arm64, armv7: proper illegal instruction

2020-10-19 Thread Theo de Raadt
Mark Kettenis  wrote:

> > Date: Mon, 19 Oct 2020 16:36:14 +0200
> > From: Christian Weisgerber 
> > 
> > Belatedly, ARM has taken a slice of the reserved opcode space and
> > assigned it as a properly defined illegal instruction, udf #imm16.
> > (Armv8 Architecture Reference Manual, edition F.c, section C6.2.335).
> > Clang already knows about it.
> > 
> > We really should use this instead of picking something ad-hoc out
> > of the opcode space.
> > 
> > I have verified that this builds on arm64, produces a SIGILL in
> > userland, and drops me into ddb in the kernel.
> > 
> > armv7 has an equivalent instruction.  kettenis@ confirms it builds
> > and SIGILLs there.
> > 
> > OK?
> 
> So on armv7 there is an additional consideration.  The architecture
> defines tow instruction sets: A32 and T32 (Thumb).  A32 instructions
> are 32-bit but T32 instructions can be 16-bit.  If an attacker can
> switch the CPU into T32 mode, it will interpret this UDF instruction
> as two different instructions.  We may have to consider how "bad"
> these two instructions are and maybe tune that #imm16 accordingly.

If the attacker has turned on thumb in the kernel I think all causes
are lost ... aren't there thousands of thumb gadgets?



Re: arm64, armv7: proper illegal instruction

2020-10-19 Thread Theo de Raadt
Thanks for finding a better instruction!

ok deraadt

Christian Weisgerber  wrote:

> Belatedly, ARM has taken a slice of the reserved opcode space and
> assigned it as a properly defined illegal instruction, udf #imm16.
> (Armv8 Architecture Reference Manual, edition F.c, section C6.2.335).
> Clang already knows about it.
> 
> We really should use this instead of picking something ad-hoc out
> of the opcode space.
> 
> I have verified that this builds on arm64, produces a SIGILL in
> userland, and drops me into ddb in the kernel.
> 
> armv7 has an equivalent instruction.  kettenis@ confirms it builds
> and SIGILLs there.
> 
> OK?
> 
> Index: lib/csu/aarch64/md_init.h
> ===
> RCS file: /cvs/src/lib/csu/aarch64/md_init.h,v
> retrieving revision 1.9
> diff -u -p -r1.9 md_init.h
> --- lib/csu/aarch64/md_init.h 15 Oct 2020 16:30:21 -  1.9
> +++ lib/csu/aarch64/md_init.h 19 Oct 2020 11:57:02 -
> @@ -115,5 +115,5 @@
>   "   svc #0  \n" \
>   "   dsb nsh \n" \
>   "   isb \n" \
> - "   .word 0xa000f7f0 /* illegal */  \n" \
> + "   udf #0  \n" \
>   ".previous");
> Index: lib/csu/arm/md_init.h
> ===
> RCS file: /cvs/src/lib/csu/arm/md_init.h,v
> retrieving revision 1.16
> diff -u -p -r1.16 md_init.h
> --- lib/csu/arm/md_init.h 15 Oct 2020 16:30:23 -  1.16
> +++ lib/csu/arm/md_init.h 19 Oct 2020 13:23:00 -
> @@ -159,5 +159,5 @@
>   "   swi #0  \n" \
>   "   dsb nsh \n" \
>   "   isb \n" \
> - "   .word 0xa000f7f0 /* illegal */  \n" \
> + "   udf #0  \n" \
>   ".previous");
> Index: lib/libc/arch/aarch64/sys/tfork_thread.S
> ===
> RCS file: /cvs/src/lib/libc/arch/aarch64/sys/tfork_thread.S,v
> retrieving revision 1.5
> diff -u -p -r1.5 tfork_thread.S
> --- lib/libc/arch/aarch64/sys/tfork_thread.S  18 Oct 2020 14:28:16 -  
> 1.5
> +++ lib/libc/arch/aarch64/sys/tfork_thread.S  19 Oct 2020 11:59:32 -
> @@ -43,6 +43,6 @@ ENTRY(__tfork_thread)
>   mov x0, x3
>   blr x2
>   SYSTRAP(__threxit)
> - .word   0xa000f7f0  /* illegal on all cpus? */
> + udf #0
>   .cfi_endproc
>  END(__tfork_thread)
> Index: lib/libc/arch/arm/sys/tfork_thread.S
> ===
> RCS file: /cvs/src/lib/libc/arch/arm/sys/tfork_thread.S,v
> retrieving revision 1.5
> diff -u -p -r1.5 tfork_thread.S
> --- lib/libc/arch/arm/sys/tfork_thread.S  18 Oct 2020 14:28:17 -  
> 1.5
> +++ lib/libc/arch/arm/sys/tfork_thread.S  19 Oct 2020 13:23:35 -
> @@ -37,5 +37,5 @@ ENTRY(__tfork_thread)
>   mov pc, r2
>   nop
>   SYSTRAP(__threxit)
> - .word   0xa000f7f0  /* illegal on all cpus? */
> + udf #0
>  END(__tfork_thread)
> Index: sys/arch/arm/arm/sigcode.S
> ===
> RCS file: /cvs/src/sys/arch/arm/arm/sigcode.S,v
> retrieving revision 1.9
> diff -u -p -r1.9 sigcode.S
> --- sys/arch/arm/arm/sigcode.S13 Mar 2020 08:46:50 -  1.9
> +++ sys/arch/arm/arm/sigcode.S19 Oct 2020 13:23:55 -
> @@ -72,7 +72,7 @@ _C_LABEL(esigcode):
>  
>   .globl  sigfill
>  sigfill:
> - .word   0xa000f7f0  /* illegal on all cpus? */
> + udf #0
>  esigfill:
>  
>   .data
> Index: sys/arch/arm64/arm64/locore.S
> ===
> RCS file: /cvs/src/sys/arch/arm64/arm64/locore.S,v
> retrieving revision 1.31
> diff -u -p -r1.31 locore.S
> --- sys/arch/arm64/arm64/locore.S 13 Mar 2020 00:14:38 -  1.31
> +++ sys/arch/arm64/arm64/locore.S 19 Oct 2020 12:02:23 -
> @@ -366,7 +366,7 @@ _C_LABEL(esigcode):
>  
>   .globl  sigfill
>  sigfill:
> - .word   0xa000f7f0  /* FIXME: illegal on all cpus? */
> + udf #0
>  esigfill:
>  
>   .data
> -- 
> Christian "naddy" Weisgerber  na...@mips.inka.de
> 



Re: uvm_grow(): serialize updates

2020-10-16 Thread Theo de Raadt
Mark Kettenis  wrote:

> > /* For user defined stacks (from sendsig). */
> > if (sp < (vaddr_t)vm->vm_maxsaddr)
> > -   return;
> > +   goto out;
> 
> Since vm_maxsaddr is ummutable, this check can be done without holding
> the lock.  I think that's worth it as it will prevent contention in
> multi-threaded processes as this check will almost always be true for
> anything but the first thread since those will use a user-defined
> stack.

at k2k20 when revamping trap.c, we puzzled about the double check, and
the next day you pointed this out this is lock avoidance for the common
case.

I agree it should remain.  If I recall correctly one of the trap.c was
coded to grab the kernel lock for uvm_fault, release it, and then
re-grab it for uvm_grow.  Such double grabs also seem harmful.



Re: timeout(9): add clock-based timeouts (attempt 2)

2020-10-09 Thread Theo de Raadt
> I think this is good to go.  ok kettenis@
> 
> Did briefly discuss with Theo during k2k20 and the consensus was it
> should go in after relase.  Which is now!

Agree.



Re: doas: improve error message

2020-10-08 Thread Theo de Raadt
Klemens Nanni  wrote:

> The diff does not change behaviour or output for end-users on the
> command line;  instead it changes syslog messages which by default are
> only readable by root.

  ^

That's the key detail for me, as it means no additional information
is being exposed.  ok deraadt



Re: Bad definition of SIOCG80211JOIN

2020-10-05 Thread Theo de Raadt
OK deraadt


Gerhard Roth  wrote:

> The current definition of SIOCG80211JOIN uses 256 for the command,
> but the _IOC() macro only allows 8 bit for the command value.
> Using 256 would set the lowermost bit of the ioctl group.
> Fortunately, 'i' (0x69) already has the lowermost bit set. Otherwise
> SIOCG80211JOIN would never reach ifioctl().
> 
> The patch below is compatible with the current definition and
> results in no binary change, it just reflects reality better.
> 
> Gerhard
> 
> 
> --- sys/net80211/ieee80211_ioctl.h2020/04/29 13:13:30 1.40
> +++ sys/net80211/ieee80211_ioctl.h2020/10/05 11:24:06
> @@ -275,11 +275,11 @@ struct ieee80211_keyrun {
>  
>  #define SIOCS80211SCAN_IOW('i', 210, struct ifreq)
>  
>  #define  SIOCG80211JOINALL   _IOWR('i', 218, struct 
> ieee80211_joinreq_all)
>  #define  SIOCS80211JOIN  _IOWR('i', 255, struct ifreq)
> -#define  SIOCG80211JOIN  _IOWR('i', 256, struct ifreq)
> +#define  SIOCG80211JOIN  _IOWR('i', 0, struct ifreq)
>  
>  /* join is pointed at by ifr.ifr_data */
>  struct ieee80211_join {
>   u_int8_ti_len;  /* length of i_nwid */
>   u_int8_ti_nwid[IEEE80211_NWID_LEN];
> 



Re: pthread_spin_unlock and ownership behaviour

2020-10-03 Thread Theo de Raadt
> For shits and giggles I deceided to look into pthread_spin_unlock and
> saw that we return EPERM here, while POSIX states that this behaviour
> is unconditionally undefined.[0] Is there a reason why we shouldn't
> abort here as well?

Undefined doesn't mean it can turn into a bowl of petunias.

As a general rule, it is completely terrible if base libraries abort,
it is terrible if programs dump core in a directory for an inexplicit
reason which is hard to diagnose from the corefile.

And have you read abort.c?  It does and performs multiple system calls
before actually dying..



Re: new: rp(4): RocketPort multi serial port pci card driver

2020-10-02 Thread Theo de Raadt
This lacks diffs for MAKEDEV and other files.  We need to see
those.



Re: mfokclock(4) -> mfokrtc(4): loongson's hugging arm

2020-09-29 Thread Theo de Raadt
Patrick Wildt  wrote:

> On Tue, Sep 29, 2020 at 11:23:03AM -0600, Theo de Raadt wrote:
> > +   if (strcmp(ia->ia_name, "st,m41t83") == 0)
> > 
> > fine with me.  But in that case, why not call the driver i2c/m41t83.c
> 
> Because it's a series of chips, like m41t80, m41t81, m41t82.  Machines
> and their device trees encode the specific chip, because some versions
> might have a different feature set as others.  We do the same x-place-
> holder-notation for other drivers as well.
> 
> For now I only match on st,m41t83 since that is the version of the chip
> the driver was originally written for.  For every additional supported
> variant of that series, the code needs to be checked for correctness.
> 
> But the file is still supposed to cover more than this one specific
> variant of the series.

If it supports many chips, why are you only matching for one variation?
To save code bytes, I guess.  But later on, will there be any variation
handling in the driver?  Not having checked the vendor docs, I suspect
they are highly compatible.  In chips like this, the variation tends
to be electronics, or power connectivity or such, rather than register
or behaviour layout.

The other way this works, is the new chip has a very similar number
but totally different behaviour.

Which is it?



Re: mfokclock(4) -> mfokrtc(4): loongson's hugging arm

2020-09-29 Thread Theo de Raadt
+   if (strcmp(ia->ia_name, "st,m41t83") == 0)

fine with me.  But in that case, why not call the driver i2c/m41t83.c



Re: [PATCH netcat] Only force fd's to -1 once

2020-09-27 Thread Theo de Raadt
Duncan Roe  wrote:

> > I disagree with your plan.  This kind of "force it off" programming is
> > typical.
> >
> Might be typical, but I think it's sloppy.

I don't think it is sloppy.

Instead, I think your additional logic complicates review.




Re: [PATCH netcat] Only force fd's to -1 once

2020-09-27 Thread Theo de Raadt
Duncan Roe  wrote:

> On Sun, Sep 27, 2020 at 02:18:21PM -0600, Bob Beck wrote:
> > On Sun, Sep 27, 2020 at 02:46:39PM +1000, Duncan Roe wrote:
> > > The motivation for this is to make debug logs less confusing.
> >
> > What is this fixing and what behavior are you changing?
> 
> You can only see the difference when running nc under gdb, if you choose to
> breakpoint those lines that set pfd[POLL_xx].fd to -1.
> 
> I was doing this because I wanted to track when file units got closed.
> But sometimes an fd was getting set to -1 when it was -1 already, hence the
> patch.

I disagree with your plan.  This kind of "force it off" programming is
typical.



Re: setpriority(2): booleans are not scalars

2020-09-25 Thread Theo de Raadt
David Riley  wrote:

> On Sep 25, 2020, at 3:13 PM, Todd C. Miller  wrote:
> > 
> > On Fri, 25 Sep 2020 13:58:01 -0500, Scott Cheloha wrote:
> > 
> >> `found' serves as a boolean here.  I'd prefer to simple and set it to
> >> 1 instead of incrementing it when we find what we're looking for.
> > 
> > Makes sense to me, the use of var++ instead of var=1 is old-school
> > style ;-)
> 
> There are still some architectures where an increment vs. an immediate load 
> is a shorter instruction, but I strongly suspect the performance difference 
> (where there even is one) is not worth the unclarity here.

Which architecture is faster to load+increment+store, than store a constant?
I'd like to know.

But let's look deeper.  How about we fix OpenBSD to support 2GB or 4GB
of processes.  What happens next?

Alternatively on some architectures it is faster to increment a char than
an int, how about we change var to 'char var = 0'?  Still feeling good about
the change?

Premature optimization can be hazardous.  The ++ code as written carries
meanings and intents, which are incorrect.





Re: setpriority(2): booleans are not scalars

2020-09-25 Thread Theo de Raadt
We can't have 2 billion processes to reach a wrap.

But I agree, it seems quite wrong, and seems better to just observe
match rather than count.


Scott Cheloha  wrote:

> `found' serves as a boolean here.  I'd prefer to simple and set it to
> 1 instead of incrementing it when we find what we're looking for.
> 
> ok?
> 
> Index: kern_resource.c
> ===
> RCS file: /cvs/src/sys/kern/kern_resource.c,v
> retrieving revision 1.68
> diff -u -p -r1.68 kern_resource.c
> --- kern_resource.c   15 Jul 2019 20:44:48 -  1.68
> +++ kern_resource.c   25 Sep 2020 18:54:34 -
> @@ -157,7 +157,7 @@ sys_setpriority(struct proc *curp, void 
>   if (pr == NULL)
>   break;
>   error = donice(curp, pr, SCARG(uap, prio));
> - found++;
> + found = 1;
>   break;
>  
>   case PRIO_PGRP: {
> @@ -169,7 +169,7 @@ sys_setpriority(struct proc *curp, void 
>   break;
>   LIST_FOREACH(pr, >pg_members, ps_pglist) {
>   error = donice(curp, pr, SCARG(uap, prio));
> - found++;
> + found = 1;
>   }
>   break;
>   }
> @@ -180,14 +180,14 @@ sys_setpriority(struct proc *curp, void 
>   LIST_FOREACH(pr, , ps_list)
>   if (pr->ps_ucred->cr_uid == SCARG(uap, who)) {
>   error = donice(curp, pr, SCARG(uap, prio));
> - found++;
> + found = 1;
>   }
>   break;
>  
>   default:
>   return (EINVAL);
>   }
> - if (found == 0)
> + if (!found)
>   return (ESRCH);
>   return (error);
>  }
> 



Re: amap: panic -> KASSERT

2020-09-25 Thread Theo de Raadt
Mark Kettenis  wrote:

> And as I implied, KASSERTs are no-ops in RAMDISK kernels so they don't
> stop things going horribly of the rails in that context, whereas a
> panic would still happen.

That's an important point, with big consequences.

Compile a RAMDISK, and the KASSERT gets removed.  Also, SMALL_KERNEL
gets set, which changes a large number of low-level configurations.

Let's say the bad condition has occured.  Now the code runs off the
rails, and silently causes other side effects.

And now, everyone focuses on the damaging side-effects created as
aftermath as the code RETURNS SUCCESFULLY upwards through the kernel
from that function into an incoherent machine-state, because the
specific condition wasn't detected early and created an immediate
verbose stop.

Now tell me, how do we debug this?

Well, it is not easy.

The labour of fixing the ramdisks "SMALL_KERNEL" subset falls upon a
smaller group of developers who vaguely understand the constraints but
have had the debug logic removed.  I suspect that group of developers
does not include the people with We Love KASSERT stickers on their
laptops...




Re: amap: panic -> KASSERT

2020-09-25 Thread Theo de Raadt
Scott Cheloha  wrote:

> Is there any scenario where
> 
> if (condition)
> panic();
> 
> is preferable to
> 
> KASSERT(condition);
> 
> outside of function calls with side effects?

Definately, sometimes you want to use simple english language for the
failure condition, rather than the relationship between kernel
variables.



Re: Call uvm_grow() on powerpc64

2020-09-24 Thread Theo de Raadt
>> From: Theo de Raadt 
>> Date: Thu, 24 Sep 2020 15:27:06 -0600 (MDT)
>> 
>> >The call is missing from the trap handler, probably because I was
>> >looking at arm64 where it is missing as well.  The result is that the
>> >stack size accounting will be wrong.
>> 
>> Nice find.
>> 
>> >In the diff below I only added the call to the "data" trap.  That
>> >means that an "instruction" trap will not run the accounting code.  Is
>> >that correct?  The uvm_fault() call should never return success in
>> >that case unless the stack has been mapped executable...
>> 
>> I think both should have it.  munmap and mprotect exist, and
>> people can do strange things.
>
>Which would be this diff.  ok?

Yes.

Architectures with a single fault routing aren't seperating out
exec-faults (for instance x86 PGEX_I faults) and skipping uvm_grow.

uvm_grow is detecting things in the "stack zone", and we don't
specifically prevent X mappings there.  We've pushed back on some
toolchain parts that wanted executable stacks very succesfully.
W|X in the stack region won't work because of W^X, but R|X would.

I'm not sure we can forcibly exclude it.  I see little risk in
uvm_grow from calling it, and letting it adjust the numbers. It is
just accounting..


>Index: arch/powerpc64/powerpc64/trap.c
>===
>RCS file: /cvs/src/sys/arch/powerpc64/powerpc64/trap.c,v
>retrieving revision 1.39
>diff -u -p -r1.39 trap.c
>--- arch/powerpc64/powerpc64/trap.c24 Sep 2020 20:22:15 -  1.39
>+++ arch/powerpc64/powerpc64/trap.c24 Sep 2020 21:36:27 -
>@@ -181,6 +181,8 @@ trap(struct trapframe *frame)
>   ftype = PROT_READ;
>   KERNEL_LOCK();
>   error = uvm_fault(map, trunc_page(va), 0, ftype);
>+  if (error == 0)
>+  uvm_grow(p, trunc_page(va));
>   KERNEL_UNLOCK();
>   if (error) {
> #ifdef TRAP_DEBUG
>@@ -225,6 +227,8 @@ trap(struct trapframe *frame)
>   ftype = PROT_READ | PROT_EXEC;
>   KERNEL_LOCK();
>   error = uvm_fault(map, trunc_page(va), 0, ftype);
>+  if (error == 0)
>+  uvm_grow(p, trunc_page(va));
>   KERNEL_UNLOCK();
>   if (error) {
> #ifdef TRAP_DEBUG
>



Re: Call uvm_grow() on powerpc64

2020-09-24 Thread Theo de Raadt
>The call is missing from the trap handler, probably because I was
>looking at arm64 where it is missing as well.  The result is that the
>stack size accounting will be wrong.

Nice find.

>In the diff below I only added the call to the "data" trap.  That
>means that an "instruction" trap will not run the accounting code.  Is
>that correct?  The uvm_fault() call should never return success in
>that case unless the stack has been mapped executable...

I think both should have it.  munmap and mprotect exist, and
people can do strange things.

>Index: arch/powerpc64/powerpc64/trap.c
>===
>RCS file: /cvs/src/sys/arch/powerpc64/powerpc64/trap.c,v
>retrieving revision 1.39
>diff -u -p -r1.39 trap.c
>--- arch/powerpc64/powerpc64/trap.c24 Sep 2020 20:22:15 -  1.39
>+++ arch/powerpc64/powerpc64/trap.c24 Sep 2020 21:11:08 -
>@@ -181,6 +181,8 @@ trap(struct trapframe *frame)
>   ftype = PROT_READ;
>   KERNEL_LOCK();
>   error = uvm_fault(map, trunc_page(va), 0, ftype);
>+  if (error == 0)
>+  uvm_grow(p, trunc_page(va));
>   KERNEL_UNLOCK();
>   if (error) {
> #ifdef TRAP_DEBUG
>
>



Re: amap: panic -> KASSERT

2020-09-24 Thread Theo de Raadt
Mark Kettenis  wrote:

> > Date: Thu, 24 Sep 2020 11:53:59 +0200
> > From: Martin Pieuchot 
> > 
> > Convert various "if (x) panic()" idioms into "KASSERT(!x)".  The panic
> > message isn't helping for such sanity checks and this help reducing the
> > diff with NetBSD.
> > 
> > ok?
> 
> Yes, the KASSERTs are probably more useful for debugging.  The
> downside is that we lose the checks in RAMDISK kernels.  The upside of
> that is that it makes the kernel smaller.
> 
> ok kettenis@

That's the complete assessment of the situation, and on the scale I'm
happy with the diff.

ok deraadt



Re: uvm_map_inentry() checks in trap()

2020-09-23 Thread Theo de Raadt
Theo de Raadt  wrote:

> kettenis, mortimer, and I have had long discussions about the
> uvm_map_inentry() checks in MD trap.c being excessively broad.  We can
> this check-function less often.
> 
> I don't want to explain the full picture about how SP and PC checks
> in syscall() and trap() hinder ROP, ROP stack pivoting, gadgets with
> unfortunate memory access side-effects, etc.  But...
> 
> When a ROP chain pivots onto a non-MAP_STACK chunk of data, to use a
> data/heap loaded secondary chain, it will eventually want to do a system
> call.
> 
> (a) Thus, we force the chain to pivot back to MAP_STACK memory before doing
> the first syscall, otherwise the SP check in syscall() will kill it.
> This pivot-back is additional labour not neccessary for attack on other
> operating systems.
> 
> (b) We also do sp checking at trap time.  On most architectures it is
> currently done for MANY types of userland traps.  (Additionally, it can
> tsleep, which exposed a bunch of old bugs on many architecturs we fixed
> at k2k20).  We now propose only performing uvm_map_inentry() for the
> standard userland "page fault" case which goes to uvm_fault()
> 
> It is pointless (and costly) to use interrupts, or other strange and
> rare traps, hoping to catch a rop chain which is just chewing through
> properly mapped memory.  Those other traps tend to also get us into
> issues like tsleep.  Without a performance cost, and negligable security
> benefit.
> 
> However, unmapped memory is the achilles heel.
> 
> A poorly written chain will eventually accesseses unmapped memory because
> it is using a gadget which has a memory-access side effect (and
> references to a not-yet-mapped page (either cow fault, or zfod, or
> demand-from-vnode), or because it is trying to use memory-scanning or
> sled methods).  Additionally, such unmapped memory accesses maybe done as
> ptr derefs against uncontrolled registers, and libc randomization plays
> a powerful role (but the margin way too narrow)
> 
> So for an attacker, this reduces gadget selection to ONLY THE BEST
> GADGETS -- meaning, minimal or no side effects.  We've reduced gadget
> count by a lot, placed code in very random places, so this becomes a
> further hinderance in selection.
> 
> By narrowing the check, it will bring back a bit of performance from
> before the addition of trap.c uvm_map_inentry()

Here are all the diffs.  kettenis worked on a few with me, and visa
wrote the mips64 one.

arm64 and armv7 don't need changes.

Index: alpha/alpha/trap.c
===
RCS file: /cvs/src/sys/arch/alpha/alpha/trap.c,v
retrieving revision 1.89
diff -u -p -u -r1.89 trap.c
--- alpha/alpha/trap.c  19 Aug 2020 10:10:57 -  1.89
+++ alpha/alpha/trap.c  23 Sep 2020 20:30:45 -
@@ -244,10 +244,6 @@ trap(a0, a1, a2, entry, framep)
if (user) {
p->p_md.md_tf = framep;
refreshcreds(p);
-   if (!uvm_map_inentry(p, >p_spinentry, PROC_STACK(p),
-  "[%s]%d/%d sp=%lx inside %lx-%lx: not MAP_STACK\n",
-   uvm_map_inentry_sp, p->p_vmspace->vm_map.sserial))
-   goto out;
}
 
switch (entry) {
@@ -370,6 +366,12 @@ trap(a0, a1, a2, entry, framep)
break;
 
case ALPHA_KENTRY_MM:
+   if (user &&
+   !uvm_map_inentry(p, >p_spinentry, PROC_STACK(p),
+  "[%s]%d/%d sp=%lx inside %lx-%lx: not MAP_STACK\n",
+   uvm_map_inentry_sp, p->p_vmspace->vm_map.sserial))
+   goto out;
+
switch (a1) {
case ALPHA_MMCSR_FOR:
case ALPHA_MMCSR_FOE:
Index: amd64/amd64/trap.c
===
RCS file: /cvs/src/sys/arch/amd64/amd64/trap.c,v
retrieving revision 1.81
diff -u -p -u -r1.81 trap.c
--- amd64/amd64/trap.c  14 Sep 2020 12:51:28 -  1.81
+++ amd64/amd64/trap.c  23 Sep 2020 12:17:10 -
@@ -343,11 +343,6 @@ usertrap(struct trapframe *frame)
p->p_md.md_regs = frame;
refreshcreds(p);
 
-   if (!uvm_map_inentry(p, >p_spinentry, PROC_STACK(p),
-   "[%s]%d/%d sp=%lx inside %lx-%lx: not MAP_STACK\n",
-   uvm_map_inentry_sp, p->p_vmspace->vm_map.sserial))
-   goto out;
-
switch (type) {
case T_PROTFLT: /* protection fault */
case T_TSSFLT:
@@ -381,6 +376,10 @@ usertrap(struct trapframe *frame)
break;
 
case T_PAGEFLT: /* page fault */
+   if (!uvm_map_inentry(p, >p_spinentry, PROC_STACK(p),
+   "[%s]%d/%d sp=%lx inside %lx-%lx: not MAP_

Re: uvm_map_inentry() checks in trap()

2020-09-23 Thread Theo de Raadt
Theo de Raadt  wrote:

> Theo de Raadt  wrote:
> 
> > kettenis, mortimer, and I have had long discussions about the
> > uvm_map_inentry() checks in MD trap.c being excessively broad.  We can
> > this check-function less often.
> > 
> > I don't want to explain the full picture about how SP and PC checks
> > in syscall() and trap() hinder ROP, ROP stack pivoting, gadgets with
> > unfortunate memory access side-effects, etc.  But...
> > 
> > When a ROP chain pivots onto a non-MAP_STACK chunk of data, to use a
> > data/heap loaded secondary chain, it will eventually want to do a system
> > call.
> > 
> > (a) Thus, we force the chain to pivot back to MAP_STACK memory before doing
> > the first syscall, otherwise the SP check in syscall() will kill it.
> > This pivot-back is additional labour not neccessary for attack on other
> > operating systems.
> > 
> > (b) We also do sp checking at trap time.  On most architectures it is
> > currently done for MANY types of userland traps.  (Additionally, it can
> > tsleep, which exposed a bunch of old bugs on many architecturs we fixed
> > at k2k20).  We now propose only performing uvm_map_inentry() for the
> > standard userland "page fault" case which goes to uvm_fault()
> > 
> > It is pointless (and costly) to use interrupts, or other strange and
> > rare traps, hoping to catch a rop chain which is just chewing through
> > properly mapped memory.  Those other traps tend to also get us into
> > issues like tsleep.  Without a performance cost, and negligable security
> > benefit.
> > 
> > However, unmapped memory is the achilles heel.
> > 
> > A poorly written chain will eventually accesseses unmapped memory because
> > it is using a gadget which has a memory-access side effect (and
> > references to a not-yet-mapped page (either cow fault, or zfod, or
> > demand-from-vnode), or because it is trying to use memory-scanning or
> > sled methods).  Additionally, such unmapped memory accesses maybe done as
> > ptr derefs against uncontrolled registers, and libc randomization plays
> > a powerful role (but the margin way too narrow)
> > 
> > So for an attacker, this reduces gadget selection to ONLY THE BEST
> > GADGETS -- meaning, minimal or no side effects.  We've reduced gadget
> > count by a lot, placed code in very random places, so this becomes a
> > further hinderance in selection.
> > 
> > By narrowing the check, it will bring back a bit of performance from
> > before the addition of trap.c uvm_map_inentry()
> 
> Repeating diffs from i386, amd64, sh.  Improve alpha diff to handle an
> additional fault case.  Adding diffs for powerpc, powerc64, and m88k.
> 
> armv7, sparc64, hppa, mips64 still missing.  arm64 is already doing it
> mostly right.

Plus hppa.

armv7, sparc64, mips64 missing.

Index: alpha/alpha/trap.c
===
RCS file: /cvs/src/sys/arch/alpha/alpha/trap.c,v
retrieving revision 1.89
diff -u -p -u -r1.89 trap.c
--- alpha/alpha/trap.c  19 Aug 2020 10:10:57 -  1.89
+++ alpha/alpha/trap.c  23 Sep 2020 13:11:48 -
@@ -244,10 +244,6 @@ trap(a0, a1, a2, entry, framep)
if (user) {
p->p_md.md_tf = framep;
refreshcreds(p);
-   if (!uvm_map_inentry(p, >p_spinentry, PROC_STACK(p),
-  "[%s]%d/%d sp=%lx inside %lx-%lx: not MAP_STACK\n",
-   uvm_map_inentry_sp, p->p_vmspace->vm_map.sserial))
-   goto out;
}
 
switch (entry) {
@@ -370,6 +366,12 @@ trap(a0, a1, a2, entry, framep)
break;
 
case ALPHA_KENTRY_MM:
+   if (user &&
+   !uvm_map_inentry(p, >p_spinentry, PROC_STACK(p),
+  "[%s]%d/%d sp=%lx inside %lx-%lx: not MAP_STACK\n",
+   uvm_map_inentry_sp, p->p_vmspace->vm_map.sserial))
+   goto out;
+
switch (a1) {
case ALPHA_MMCSR_FOR:
case ALPHA_MMCSR_FOE:
Index: amd64/amd64/trap.c
===
RCS file: /cvs/src/sys/arch/amd64/amd64/trap.c,v
retrieving revision 1.81
diff -u -p -u -r1.81 trap.c
--- amd64/amd64/trap.c  14 Sep 2020 12:51:28 -  1.81
+++ amd64/amd64/trap.c  23 Sep 2020 12:17:10 -
@@ -343,11 +343,6 @@ usertrap(struct trapframe *frame)
p->p_md.md_regs = frame;
refreshcreds(p);
 
-   if (!uvm_map_inentry(p, >p_spinentry, PROC_STACK(p),
-   "[%s]%d/%d sp=%lx inside %lx-%lx: not MAP_STACK\n",
-   uvm_map_inentry_s

Re: uvm_map_inentry() checks in trap()

2020-09-23 Thread Theo de Raadt
Theo de Raadt  wrote:

> kettenis, mortimer, and I have had long discussions about the
> uvm_map_inentry() checks in MD trap.c being excessively broad.  We can
> this check-function less often.
> 
> I don't want to explain the full picture about how SP and PC checks
> in syscall() and trap() hinder ROP, ROP stack pivoting, gadgets with
> unfortunate memory access side-effects, etc.  But...
> 
> When a ROP chain pivots onto a non-MAP_STACK chunk of data, to use a
> data/heap loaded secondary chain, it will eventually want to do a system
> call.
> 
> (a) Thus, we force the chain to pivot back to MAP_STACK memory before doing
> the first syscall, otherwise the SP check in syscall() will kill it.
> This pivot-back is additional labour not neccessary for attack on other
> operating systems.
> 
> (b) We also do sp checking at trap time.  On most architectures it is
> currently done for MANY types of userland traps.  (Additionally, it can
> tsleep, which exposed a bunch of old bugs on many architecturs we fixed
> at k2k20).  We now propose only performing uvm_map_inentry() for the
> standard userland "page fault" case which goes to uvm_fault()
> 
> It is pointless (and costly) to use interrupts, or other strange and
> rare traps, hoping to catch a rop chain which is just chewing through
> properly mapped memory.  Those other traps tend to also get us into
> issues like tsleep.  Without a performance cost, and negligable security
> benefit.
> 
> However, unmapped memory is the achilles heel.
> 
> A poorly written chain will eventually accesseses unmapped memory because
> it is using a gadget which has a memory-access side effect (and
> references to a not-yet-mapped page (either cow fault, or zfod, or
> demand-from-vnode), or because it is trying to use memory-scanning or
> sled methods).  Additionally, such unmapped memory accesses maybe done as
> ptr derefs against uncontrolled registers, and libc randomization plays
> a powerful role (but the margin way too narrow)
> 
> So for an attacker, this reduces gadget selection to ONLY THE BEST
> GADGETS -- meaning, minimal or no side effects.  We've reduced gadget
> count by a lot, placed code in very random places, so this becomes a
> further hinderance in selection.
> 
> By narrowing the check, it will bring back a bit of performance from
> before the addition of trap.c uvm_map_inentry()

Repeating diffs from i386, amd64, sh.  Improve alpha diff to handle an
additional fault case.  Adding diffs for powerpc, powerc64, and m88k.

armv7, sparc64, hppa, mips64 still missing.  arm64 is already doing it
mostly right.

Index: arch/alpha/alpha/trap.c
===
RCS file: /cvs/src/sys/arch/alpha/alpha/trap.c,v
retrieving revision 1.89
diff -u -p -u -r1.89 trap.c
--- arch/alpha/alpha/trap.c 19 Aug 2020 10:10:57 -  1.89
+++ arch/alpha/alpha/trap.c 23 Sep 2020 13:11:48 -
@@ -244,10 +244,6 @@ trap(a0, a1, a2, entry, framep)
if (user) {
p->p_md.md_tf = framep;
refreshcreds(p);
-   if (!uvm_map_inentry(p, >p_spinentry, PROC_STACK(p),
-  "[%s]%d/%d sp=%lx inside %lx-%lx: not MAP_STACK\n",
-   uvm_map_inentry_sp, p->p_vmspace->vm_map.sserial))
-   goto out;
}
 
switch (entry) {
@@ -370,6 +366,12 @@ trap(a0, a1, a2, entry, framep)
break;
 
case ALPHA_KENTRY_MM:
+   if (user &&
+   !uvm_map_inentry(p, >p_spinentry, PROC_STACK(p),
+  "[%s]%d/%d sp=%lx inside %lx-%lx: not MAP_STACK\n",
+   uvm_map_inentry_sp, p->p_vmspace->vm_map.sserial))
+   goto out;
+
switch (a1) {
case ALPHA_MMCSR_FOR:
case ALPHA_MMCSR_FOE:
Index: arch/amd64/amd64/trap.c
===
RCS file: /cvs/src/sys/arch/amd64/amd64/trap.c,v
retrieving revision 1.81
diff -u -p -u -r1.81 trap.c
--- arch/amd64/amd64/trap.c 14 Sep 2020 12:51:28 -  1.81
+++ arch/amd64/amd64/trap.c 23 Sep 2020 12:17:10 -
@@ -343,11 +343,6 @@ usertrap(struct trapframe *frame)
p->p_md.md_regs = frame;
refreshcreds(p);
 
-   if (!uvm_map_inentry(p, >p_spinentry, PROC_STACK(p),
-   "[%s]%d/%d sp=%lx inside %lx-%lx: not MAP_STACK\n",
-   uvm_map_inentry_sp, p->p_vmspace->vm_map.sserial))
-   goto out;
-
switch (type) {
case T_PROTFLT: /* protection fault */
case T_TSSFLT:
@@ -381,6 +376,10 @@ usertrap(struct trapframe *frame)
break;
 
case T_PAGEFLT: /* page

uvm_map_inentry() checks in trap()

2020-09-23 Thread Theo de Raadt
kettenis, mortimer, and I have had long discussions about the
uvm_map_inentry() checks in MD trap.c being excessively broad.  We can
this check-function less often.

I don't want to explain the full picture about how SP and PC checks
in syscall() and trap() hinder ROP, ROP stack pivoting, gadgets with
unfortunate memory access side-effects, etc.  But...

When a ROP chain pivots onto a non-MAP_STACK chunk of data, to use a
data/heap loaded secondary chain, it will eventually want to do a system
call.

(a) Thus, we force the chain to pivot back to MAP_STACK memory before doing
the first syscall, otherwise the SP check in syscall() will kill it.
This pivot-back is additional labour not neccessary for attack on other
operating systems.

(b) We also do sp checking at trap time.  On most architectures it is
currently done for MANY types of userland traps.  (Additionally, it can
tsleep, which exposed a bunch of old bugs on many architecturs we fixed
at k2k20).  We now propose only performing uvm_map_inentry() for the
standard userland "page fault" case which goes to uvm_fault()

It is pointless (and costly) to use interrupts, or other strange and
rare traps, hoping to catch a rop chain which is just chewing through
properly mapped memory.  Those other traps tend to also get us into
issues like tsleep.  Without a performance cost, and negligable security
benefit.

However, unmapped memory is the achilles heel.

A poorly written chain will eventually accesseses unmapped memory because
it is using a gadget which has a memory-access side effect (and
references to a not-yet-mapped page (either cow fault, or zfod, or
demand-from-vnode), or because it is trying to use memory-scanning or
sled methods).  Additionally, such unmapped memory accesses maybe done as
ptr derefs against uncontrolled registers, and libc randomization plays
a powerful role (but the margin way too narrow)

So for an attacker, this reduces gadget selection to ONLY THE BEST
GADGETS -- meaning, minimal or no side effects.  We've reduced gadget
count by a lot, placed code in very random places, so this becomes a
further hinderance in selection.

By narrowing the check, it will bring back a bit of performance from
before the addition of trap.c uvm_map_inentry()

Here are the first four simple ones: i386, amd64, alpha, sh

Others are a bit more complex, and will follow.

Index: alpha/alpha/trap.c
===
RCS file: /cvs/src/sys/arch/alpha/alpha/trap.c,v
retrieving revision 1.89
diff -u -p -u -r1.89 trap.c
--- alpha/alpha/trap.c  19 Aug 2020 10:10:57 -  1.89
+++ alpha/alpha/trap.c  23 Sep 2020 12:18:25 -
@@ -244,10 +244,6 @@ trap(a0, a1, a2, entry, framep)
if (user) {
p->p_md.md_tf = framep;
refreshcreds(p);
-   if (!uvm_map_inentry(p, >p_spinentry, PROC_STACK(p),
-  "[%s]%d/%d sp=%lx inside %lx-%lx: not MAP_STACK\n",
-   uvm_map_inentry_sp, p->p_vmspace->vm_map.sserial))
-   goto out;
}
 
switch (entry) {
@@ -390,6 +386,12 @@ trap(a0, a1, a2, entry, framep)
struct vm_map *map;
int rv;
extern struct vm_map *kernel_map;
+
+   if (user &&
+   !uvm_map_inentry(p, >p_spinentry, PROC_STACK(p),
+  "[%s]%d/%d sp=%lx inside %lx-%lx: not MAP_STACK\n",
+   uvm_map_inentry_sp, p->p_vmspace->vm_map.sserial))
+   goto out;
 
switch (a2) {
case -1:/* instruction fetch fault */
Index: amd64/amd64/trap.c
===
RCS file: /cvs/src/sys/arch/amd64/amd64/trap.c,v
retrieving revision 1.81
diff -u -p -u -r1.81 trap.c
--- amd64/amd64/trap.c  14 Sep 2020 12:51:28 -  1.81
+++ amd64/amd64/trap.c  23 Sep 2020 12:17:10 -
@@ -343,11 +343,6 @@ usertrap(struct trapframe *frame)
p->p_md.md_regs = frame;
refreshcreds(p);
 
-   if (!uvm_map_inentry(p, >p_spinentry, PROC_STACK(p),
-   "[%s]%d/%d sp=%lx inside %lx-%lx: not MAP_STACK\n",
-   uvm_map_inentry_sp, p->p_vmspace->vm_map.sserial))
-   goto out;
-
switch (type) {
case T_PROTFLT: /* protection fault */
case T_TSSFLT:
@@ -381,6 +376,10 @@ usertrap(struct trapframe *frame)
break;
 
case T_PAGEFLT: /* page fault */
+   if (!uvm_map_inentry(p, >p_spinentry, PROC_STACK(p),
+   "[%s]%d/%d sp=%lx inside %lx-%lx: not MAP_STACK\n",
+   uvm_map_inentry_sp, p->p_vmspace->vm_map.sserial))
+   goto out;
if (pageflttrap(frame, cr2, 1))
goto out;
/* FALLTHROUGH */

Re: missing support for OHMS and ENERGY sensors in sensorsd

2020-09-22 Thread Theo de Raadt
It also lacks SENSOR_UNUSEDFEATURES.

No.

Paul de Weerd  wrote:

> Hi all,
> 
> While browsing sensorsd.c for inspiration for a small project I'm
> working on, I noticed that both src/usr.sbin/sensorsd/sensorsd.c and
> src/sbin/sysctl/sysctl.c have a 'print_sensor' function that
> returns/prints the value of the given sensor, including unit.  
> 
> However, sensorsd doesn't handle sensors with type SENSOR_OHMS or
> SENSOR_ENERGY.  That doesn't seem right.  Below is a diff that adds
> support for these types of sensors to sensorsd(8), using the same
> format string as the print_sensor in sysctl(8).
> 
> I'm not sure if these kinds of sensors even exist (I don't have
> them), so I've not been able to test this outside of compiling the
> program and seeing that it starts normally.
> 
> Cheers,
> 
> Paul
> 
> Index: sensorsd.c
> ===
> RCS file: /home/OpenBSD/cvs/src/usr.sbin/sensorsd/sensorsd.c,v
> retrieving revision 1.67
> diff -u -p -r1.67 sensorsd.c
> --- sensorsd.c22 Jul 2020 15:33:49 -  1.67
> +++ sensorsd.c21 Sep 2020 21:37:24 -
> @@ -654,6 +654,9 @@ print_sensor(enum sensor_type type, int6
>   case SENSOR_VOLTS_AC:
>   snprintf(fbuf, RFBUFSIZ, "%.2f V AC", value / 100.0);
>   break;
> + case SENSOR_OHMS:
> + snprintf(fbuf, RFBUFSIZ, "%lld ohm", value);
> + break;
>   case SENSOR_WATTS:
>   snprintf(fbuf, RFBUFSIZ, "%.2f W", value / 100.0);
>   break;
> @@ -707,6 +710,9 @@ print_sensor(enum sensor_type type, int6
>   break;
>   case SENSOR_VELOCITY:
>   snprintf(fbuf, RFBUFSIZ, "%4.3f m/s", value / 100.0);
> + break;
> + case SENSOR_ENERGY:
> + snprintf(fbuf, RFBUFSIZ, "%.2f J", value / 10.0);
>   break;
>   default:
>   snprintf(fbuf, RFBUFSIZ, "%lld ???", value);
> 
> -- 
> >[<++>-]<+++.>+++[<-->-]<.>+++[<+
> +++>-]<.>++[<>-]<+.--.[-]
>  http://www.weirdnet.nl/ 
> 



Re: sigabort(), p_sigmask & p_siglist

2020-09-16 Thread Theo de Raadt
Something doesn't feel right.

db_kill_cmd finds a process, called p, then kills it.  In your new diff
calling sigexit, take note of the comment at the top:

 * Force the current process to exit with the specified signal, dumping core

current process?  Doesn't look like it, it looks like it kills p.  But then
it calls exit1?

Is this actually behaving the same for the db_kill_cmd() call?

 



Re: systat(1): vmstat: compute rates with CLOCK_UPTIME

2020-09-16 Thread Theo de Raadt
Two days ago during my work on ongoing work for non-acpi suspend,
kettenis and I observed the same thing.

your diff works very well for me.



Re: agentx in services

2020-09-15 Thread Theo de Raadt
Absolutely.

Having it in the file also makes sure that early call to rresvport(3),
bindresvport(3), or bindresvport_sa(3) won't allocate it before the
daemon is started locally.

Martijn van Duren  wrote:

> I currently don't see any reason for adding agentx over tcp support to
> our daemons, but according to RFC2741 section 8.1.1 it should go over
> "wel-known port 705".
> 
> Worth adding or just drop it?
> 
> martijn@
> 
> Index: services
> ===
> RCS file: /cvs/src/etc/services,v
> retrieving revision 1.97
> diff -u -p -r1.97 services
> --- services  26 Jun 2020 19:51:14 -  1.97
> +++ services  15 Sep 2020 09:43:00 -
> @@ -175,6 +175,7 @@ ldaps 636/tcp # LDAP 
> over SSL
>  ldaps636/udp
>  ldp  646/tcp
>  ldp  646/udp
> +agentx   705/tcp
>  silc 706/tcp # Secure Live Internet 
> Conferencing
>  silc 706/udp
>  kerberos-adm 749/tcp # Kerberos 5 kadmin
> 



Re: syslogd listen keep alive

2020-09-14 Thread Theo de Raadt
Excellent!


Alexander Bluhm  wrote:

> Hi,
> 
> A while ago dhill@ pointed out that syslogd TCP sockets will stay
> open forever if a client aborts the connection silently.  As syslogd
> does not write anything into incoming connections, it will not
> recognize failure and the socket will stay forever.
> 
> Setting TCP keep alive on the listen socket will prevent that.  Note
> that outgoing connections don't need it as syslogd will write data
> into them.
> 
> After keep alive timeout you get this:
> 
> syslogd[51331]: tcp logger "10.188.74.74:32769" connection error: Operation 
> timed out
> syslogd[51331]: tls logger "10.188.74.74:15557" connection error: read 
> failed: error:02FFF03C:system library:func(4095):Operation timed out
> 
> ok?
> 
> bluhm
> 
> Index: usr.sbin/syslogd/syslogd.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/syslogd.c,v
> retrieving revision 1.263
> diff -u -p -r1.263 syslogd.c
> --- usr.sbin/syslogd/syslogd.c25 May 2020 10:38:32 -  1.263
> +++ usr.sbin/syslogd/syslogd.c14 Sep 2020 15:09:14 -
> @@ -354,6 +354,7 @@ int   socket_bind(const char *, const char
>  int  unix_socket(char *, int, mode_t);
>  void double_sockbuf(int, int, int);
>  void set_sockbuf(int);
> +void set_keepalive(int);
>  void tailify_replytext(char *, int);
>  
>  int
> @@ -979,8 +980,10 @@ socket_bind(const char *proto, const cha
>   }
>   if (!shutread && res->ai_protocol == IPPROTO_UDP)
>   double_sockbuf(*fdp, SO_RCVBUF, 0);
> - else if (res->ai_protocol == IPPROTO_TCP)
> + else if (res->ai_protocol == IPPROTO_TCP) {
>   set_sockbuf(*fdp);
> + set_keepalive(*fdp);
> + }
>   reuseaddr = 1;
>   if (setsockopt(*fdp, SOL_SOCKET, SO_REUSEADDR, ,
>   sizeof(reuseaddr)) == -1) {
> @@ -3104,6 +3107,15 @@ set_sockbuf(int fd)
>   log_warn("setsockopt sndbufsize %d", size);
>   if (setsockopt(fd, SOL_SOCKET, SO_RCVBUF, , sizeof(size)) == -1)
>   log_warn("setsockopt rcvbufsize %d", size);
> +}
> +
> +void
> +set_keepalive(int fd)
> +{
> + int val = 1;
> +
> + if (setsockopt(fd, SOL_SOCKET, SO_KEEPALIVE, , sizeof(val)) == -1)
> + log_warn("setsockopt keepalive %d", val);
>  }
>  
>  void
> 



Re: go/rust vs uvm_map_inentry()

2020-09-14 Thread Theo de Raadt
A similar fix for the "sh" cpu, which is in the landisk.

Index: sh/sh/trap.c
===
RCS file: /cvs/src/sys/arch/sh/sh/trap.c,v
retrieving revision 1.40
diff -u -p -u -r1.40 trap.c
--- sh/sh/trap.c6 Sep 2019 12:22:01 -   1.40
+++ sh/sh/trap.c14 Sep 2020 11:44:56 -
@@ -155,7 +155,7 @@ void
 general_exception(struct proc *p, struct trapframe *tf, uint32_t va)
 {
int expevt = tf->tf_expevt;
-   int tra;
+   int tra = _reg_read_4(SH_(TRA));
int usermode = !KERNELMODE(tf->tf_ssr);
union sigval sv;
 
@@ -191,7 +193,6 @@ general_exception(struct proc *p, struct
case EXPEVT_TRAPA:
 #ifdef DDB
/* Check for ddb request */
-   tra = _reg_read_4(SH_(TRA));
if (tra == (_SH_TRA_BREAK << 2) &&
db_ktrap(expevt, tra, tf))
return;
@@ -201,7 +202,6 @@ general_exception(struct proc *p, struct
break;
case EXPEVT_TRAPA | EXP_USER:
/* Check for debugger break */
-   tra = _reg_read_4(SH_(TRA));
switch (tra) {
case _SH_TRA_BREAK << 2:
tf->tf_spc -= 2; /* back to the breakpoint address */



Re: go/rust vs uvm_map_inentry()

2020-09-14 Thread Theo de Raadt
i386 has the same problem.

Index: arch/i386/i386/trap.c
===
RCS file: /cvs/src/sys/arch/i386/i386/trap.c,v
retrieving revision 1.143
diff -u -p -u -r1.143 trap.c
--- arch/i386/i386/trap.c   19 Aug 2020 10:10:58 -  1.143
+++ arch/i386/i386/trap.c   14 Sep 2020 11:23:01 -
@@ -119,7 +119,7 @@ trap(struct trapframe *frame)
vm_prot_t ftype;
union sigval sv;
caddr_t onfault;
-   uint32_t cr2;
+   uint32_t cr2 = rcr2();
 
uvmexp.traps++;
 
@@ -135,7 +135,7 @@ trap(struct trapframe *frame)
if (trapdebug) {
printf("trap %d code %x eip %x cs %x eflags %x cr2 %x cpl %x\n",
frame->tf_trapno, frame->tf_err, frame->tf_eip,
-   frame->tf_cs, frame->tf_eflags, rcr2(), lapic_tpr);
+   frame->tf_cs, frame->tf_eflags, rcr2, lapic_tpr);
printf("curproc %p\n", curproc);
}
 #endif
@@ -182,7 +182,7 @@ trap(struct trapframe *frame)
printf(" in %s mode\n", (type & T_USER) ? "user" : 
"supervisor");
printf("trap type %d code %x eip %x cs %x eflags %x cr2 %x cpl 
%x\n",
type, frame->tf_err, frame->tf_eip, frame->tf_cs,
-   frame->tf_eflags, rcr2(), lapic_tpr);
+   frame->tf_eflags, rcr2, lapic_tpr);
 
panic("trap type %d, code=%x, pc=%x",
type, frame->tf_err, frame->tf_eip);
@@ -333,7 +333,6 @@ trap(struct trapframe *frame)
goto we_re_toast;
 
pcb = >p_addr->u_pcb;
-   cr2 = rcr2();
KERNEL_LOCK();
/* This will only trigger if SMEP is enabled */
if (cr2 <= VM_MAXUSER_ADDRESS && frame->tf_err & PGEX_I)
@@ -353,7 +352,6 @@ trap(struct trapframe *frame)
int error;
int signal, sicode;
 
-   cr2 = rcr2();
KERNEL_LOCK();
faultcommon:
vm = p->p_vmspace;
@@ -434,11 +432,11 @@ trap(struct trapframe *frame)
 #endif
 
case T_BPTFLT|T_USER:   /* bpt instruction fault */
-   sv.sival_int = rcr2();
+   sv.sival_int = rcr2;
trapsignal(p, SIGTRAP, type &~ T_USER, TRAP_BRKPT, sv);
break;
case T_TRCTRAP|T_USER:  /* trace trap */
-   sv.sival_int = rcr2();
+   sv.sival_int = rcr2;
trapsignal(p, SIGTRAP, type &~ T_USER, TRAP_TRACE, sv);
break;
 



Re: go/rust vs uvm_map_inentry()

2020-09-13 Thread Theo de Raadt
We need to know which one is hitting a broken counter party -- is it the
stack checker, or the syscall checker.



Re: go/rust vs uvm_map_inentry()

2020-09-13 Thread Theo de Raadt
Sebastien Marie  wrote:

> On Sun, Sep 13, 2020 at 03:29:57PM +0200, Martin Pieuchot wrote:
> > I'm no longer able to reproduce the corruption while building lang/go
> > with the diff below.  Something relevant to threading change in go since
> > march?
> > 
> > Can someone try this diff and tell me if go and/or rust still fail?
> 
> quickly tested with rustc build (nightly here), and it is failing at random 
> places (not always at the same) with memory errors (signal 11, compiler ICE 
> signal 6...)

Ah, so that is a firm no.  Totally busted.

Clearly uvm_map_inentry_fix() obviously needs the KERNEL_LOCK in the
presence of threads, I guess one thread can get into here while another
is changing the map.

The first check in uvm_map_inentry_fix does two checks against the map,
but the map is not locked:

if (addr < map->min_offset || addr >= map->max_offset)


> 
> 
> > Index: uvm/uvm_map.c
> > ===
> > RCS file: /cvs/src/sys/uvm/uvm_map.c,v
> > retrieving revision 1.266
> > diff -u -p -r1.266 uvm_map.c
> > --- uvm/uvm_map.c   12 Sep 2020 17:08:50 -  1.266
> > +++ uvm/uvm_map.c   13 Sep 2020 10:12:25 -
> > @@ -1893,16 +1893,16 @@ uvm_map_inentry(struct proc *p, struct p
> > boolean_t ok = TRUE;
> >  
> > if (uvm_map_inentry_recheck(serial, addr, ie)) {
> > -   KERNEL_LOCK();
> > ok = uvm_map_inentry_fix(p, ie, addr, fn, serial);
> > if (!ok) {
> > +   KERNEL_LOCK();
> > printf(fmt, p->p_p->ps_comm, p->p_p->ps_pid, p->p_tid,
> > addr, ie->ie_start, ie->ie_end);
> > p->p_p->ps_acflag |= AMAP;
> > sv.sival_ptr = (void *)PROC_PC(p);
> > trapsignal(p, SIGSEGV, 0, SEGV_ACCERR, sv);
> > +   KERNEL_UNLOCK();
> > }
> > -   KERNEL_UNLOCK();
> > }
> > return (ok);
> >  }
> > 
> 
> -- 
> Sebastien Marie
> 



Re: trunk: keep interface up on port removal

2020-09-13 Thread Theo de Raadt
Stuart Henderson  wrote:

> On 2020/09/13 11:12, Stuart Henderson wrote:
> > This has been tried before, I forget what but there were problems
> 
> from chat logs when I tried this before:
> 
> 14:52 < sthen> if i kill the if_down, no crash, but the mac address doesn't 
> get updated so i end up with the same one on em0, em1, trunk0
> 

Ah, that seems bad.  And in the presence of port-sec or mac security on
switches, this could provide a poor experience?

Is this the right time to change things?



Re: Format string check for dprintf(3)

2020-09-11 Thread Theo de Raadt
ok deraadt

Christian Weisgerber  wrote:

> Add format string checking annotations for dprintf(3) and vdprintf(3).
> 
> This was apparently forgotten when the functions were added.  It is
> required so the compiler can warn
> 
> t.c:25:25: warning: format string is not a string literal (potentially
> insecure)
>   [-Wformat-security]
> dprintf(STDOUT_FILENO, msg);
>^~~
> 
> Absent -Werror, I do not expect any fallout from this, but I ran a
> successful amd64 make build with it anyway.
> 
> ok?
> 
> Index: include/stdio.h
> ===
> RCS file: /cvs/src/include/stdio.h,v
> retrieving revision 1.53
> diff -u -p -r1.53 stdio.h
> --- include/stdio.h   9 Sep 2016 18:12:37 -   1.53
> +++ include/stdio.h   10 Sep 2020 15:07:08 -
> @@ -204,7 +204,9 @@ __END_DECLS
>  __BEGIN_DECLS
>  void  clearerr(FILE *);
>  #if __POSIX_VISIBLE >= 200809
> -int   dprintf(int, const char * __restrict, ...);
> +int   dprintf(int, const char * __restrict, ...)
> + __attribute__((__format__ (printf, 2, 3)))
> + __attribute__((__nonnull__ (2)));
>  #endif
>  int   fclose(FILE *);
>  int   feof(FILE *);
> @@ -266,7 +268,9 @@ intvfprintf(FILE *, const char *, __va
>  int   vprintf(const char *, __va_list);
>  int   vsprintf(char *, const char *, __va_list);
>  #if __POSIX_VISIBLE >= 200809
> -int   vdprintf(int, const char * __restrict, __va_list);
> +int   vdprintf(int, const char * __restrict, __va_list)
> + __attribute__((__format__ (printf, 2, 0)))
> + __attribute__((__nonnull__ (2)));
>  #endif
>  
>  #if __ISO_C_VISIBLE >= 1999 || __XPG_VISIBLE >= 500 || __BSD_VISIBLE
> -- 
> Christian "naddy" Weisgerber  na...@mips.inka.de
> 



Re: Allow builds on arm64 without DDB

2020-09-04 Thread Theo de Raadt
Too much ifdef.  Can you try a different approach -- leave the ipi code
intact, but wrap the call to db_enter() with #ifdef DDB?  The ipi won't
be activated, so it should be fine for it to return 0.

Matt Baulch  wrote:

> Currently, it's not possible to build on arm64 without the in-kernel debugger.
> This patch fixes the two offending files:
> 
> Index: agintc.c
> ===
> RCS file: /cvs/src/sys/arch/arm64/dev/agintc.c,v
> retrieving revision 1.26
> diff -u -p -u -p -r1.26 agintc.c
> --- agintc.c  17 Jul 2020 08:07:33 -  1.26
> +++ agintc.c  4 Sep 2020 13:18:33 -
> @@ -156,7 +156,9 @@ struct agintc_softc {
>   struct agintc_dmamem*sc_pend;
>   struct interrupt_controller sc_ic;
>   int  sc_ipi_num[2]; /* id for NOP and DDB ipi */
> +#ifdef DDB
>   int  sc_ipi_reason[MAXCPUS]; /* NOP or DDB caused */
> +#endif
>   void*sc_ipi_irq[2]; /* irqhandle for each ipi */
>  };
>  struct agintc_softc *agintc_sc;
> @@ -226,7 +228,9 @@ void  agintc_wait_rwp(struct agintc_soft
>  void agintc_r_wait_rwp(struct agintc_softc *sc);
>  uint32_t agintc_r_ictlr(void);
>  
> +#ifdef DDB
>  int  agintc_ipi_ddb(void *v);
> +#endif
>  int  agintc_ipi_nop(void *v);
>  int  agintc_ipi_combined(void *);
>  void agintc_send_ipi(struct cpu_info *, int);
> @@ -576,15 +580,19 @@ agintc_attach(struct device *parent, str
>   sc->sc_ipi_irq[0] = agintc_intr_establish(ipiirq[0],
>   IPL_IPI|IPL_MPSAFE, NULL, agintc_ipi_combined, sc, "ipi");
>   sc->sc_ipi_num[ARM_IPI_NOP] = ipiirq[0];
> +#ifdef DDB
>   sc->sc_ipi_num[ARM_IPI_DDB] = ipiirq[0];
> +#endif
>   break;
>   case 2:
>   sc->sc_ipi_irq[0] = agintc_intr_establish(ipiirq[0],
>   IPL_IPI|IPL_MPSAFE, NULL, agintc_ipi_nop, sc, "ipinop");
>   sc->sc_ipi_num[ARM_IPI_NOP] = ipiirq[0];
> +#ifdef DDB
>   sc->sc_ipi_irq[1] = agintc_intr_establish(ipiirq[1],
>   IPL_IPI|IPL_MPSAFE, NULL, agintc_ipi_ddb, sc, "ipiddb");
>   sc->sc_ipi_num[ARM_IPI_DDB] = ipiirq[1];
> +#endif
>   break;
>   default:
>   panic("nipi unexpected number %d", nipi);
> @@ -1117,6 +1125,7 @@ agintc_r_wait_rwp(struct agintc_softc *s
>  }
>  
>  #ifdef MULTIPROCESSOR
> +#ifdef DDB
>  int
>  agintc_ipi_ddb(void *v)
>  {
> @@ -1124,6 +1133,7 @@ agintc_ipi_ddb(void *v)
>   db_enter();
>   return 1;
>  }
> +#endif
>  
>  int
>  agintc_ipi_nop(void *v)
> @@ -1135,14 +1145,16 @@ agintc_ipi_nop(void *v)
>  int
>  agintc_ipi_combined(void *v)
>  {
> +#ifdef DDB
>   struct agintc_softc *sc = v;
>  
>   if (sc->sc_ipi_reason[cpu_number()] == ARM_IPI_DDB) {
>   sc->sc_ipi_reason[cpu_number()] = ARM_IPI_NOP;
>   return agintc_ipi_ddb(v);
> - } else {
> - return agintc_ipi_nop(v);
>   }
> +#endif
> +
> + return agintc_ipi_nop(v);
>  }
>  
>  void
> @@ -1154,9 +1166,11 @@ agintc_send_ipi(struct cpu_info *ci, int
>   if (ci == curcpu() && id == ARM_IPI_NOP)
>   return;
>  
> +#ifdef DDB
>   /* never overwrite IPI_DDB with IPI_NOP */
>   if (id == ARM_IPI_DDB)
>   sc->sc_ipi_reason[ci->ci_cpuid] = id;
> +#endif
>  
>   /* will only send 1 cpu */
>   sendmask = (ci->ci_mpidr & MPIDR_AFF3) << 16;
> Index: ampintc.c
> ===
> RCS file: /cvs/src/sys/arch/arm64/dev/ampintc.c,v
> retrieving revision 1.19
> diff -u -p -u -p -r1.19 ampintc.c
> --- ampintc.c 17 Jul 2020 08:07:33 -  1.19
> +++ ampintc.c 4 Sep 2020 13:18:33 -
> @@ -141,7 +141,9 @@ struct ampintc_softc {
>   uint8_t  sc_cpu_mask[ICD_ICTR_CPU_M + 1];
>   struct evcount   sc_spur;
>   struct interrupt_controller sc_ic;
> +#ifdef DDB
>   int  sc_ipi_reason[ICD_ICTR_CPU_M + 1];
> +#endif
>   int  sc_ipi_num[2];
>  };
>  struct ampintc_softc *ampintc;
> @@ -196,7 +198,9 @@ void   ampintc_intr_barrier(void *);
>  
>  int   ampintc_ipi_combined(void *);
>  int   ampintc_ipi_nop(void *);
> +#ifdef DDB
>  int   ampintc_ipi_ddb(void *);
> +#endif
>  void  ampintc_send_ipi(struct cpu_info *, int);
>  
>  struct cfattach  ampintc_ca = {
> @@ -347,15 +351,19 @@ ampintc_attach(struct device *parent, st
>   ampintc_intr_establish(ipiirq[0], IST_EDGE_RISING,
>   IPL_IPI|IPL_MPSAFE, NULL, ampintc_ipi_combined, sc, "ipi");
>   sc->sc_ipi_num[ARM_IPI_NOP] = ipiirq[0];
> +#ifdef DDB
>   sc->sc_ipi_num[ARM_IPI_DDB] = ipiirq[0];
> +#endif
>   break;
>   case 2:
>   ampintc_intr_establish(ipiirq[0], IST_EDGE_RISING,
>   

Re: Add more errnos to ober_read_elements

2020-09-03 Thread Theo de Raadt
Yep.

Martijn van Duren  wrote:

> Just reminded myself of this one.
> The manpage says nothing about ober_read_elements setting errno upon
> failure, yet it does in most cases. Furthermore, applications like
> snmpd use errno in ober_read_elements to determine if a read is
> incomplete (checking for ECANCELED), without initializing errno to
> 0.
> 
> The danger here is that since some stale errno might linger and a
> return from ober_read_elements with NULL could test against an old
> errno.
> 
> Diff below tries to remedy this.
> 
> OK?
> 
> martijn@
> 
> Index: ber.c
> ===
> RCS file: /cvs/src/lib/libutil/ber.c,v
> retrieving revision 1.15
> diff -u -p -r1.15 ber.c
> --- ber.c 24 Oct 2019 12:39:26 -  1.15
> +++ ber.c 3 Sep 2020 15:58:23 -
> @@ -1266,8 +1266,10 @@ ober_read_element(struct ber *ber, struc
>  
>   /* smallest number of contents octets only */
>   if ((i == 1 && last == 0 && (c & 0x80) == 0) ||
> - (i == 1 && last == 0xff && (c & 0x80) != 0))
> + (i == 1 && last == 0xff && (c & 0x80) != 0)) {
> + errno = EINVAL;
>   return -1;
> + }
>  
>   val <<= 8;
>   val |= c;
> @@ -1299,8 +1301,10 @@ ober_read_element(struct ber *ber, struc
>   ((u_char *)elm->be_val)[len] = '\0';
>   break;
>   case BER_TYPE_NULL: /* no payload */
> - if (len != 0)
> + if (len != 0) {
> + errno = EINVAL;
>   return -1;
> + }
>   break;
>   case BER_TYPE_SEQUENCE:
>   case BER_TYPE_SET:
> @@ -1346,8 +1350,10 @@ ober_read(struct ber *ber, void *buf, si
>  {
>   size_t  sz;
>  
> - if (ber->br_rbuf == NULL)
> + if (ber->br_rbuf == NULL) {
> + errno = ENOBUFS;
>   return -1;
> + }
>  
>   sz = ber->br_rend - ber->br_rptr;
>   if (len > sz) {
> 



Re: clang warning in vmctl

2020-09-02 Thread Theo de Raadt
I had this diff in my tree for a while, but haven't gotten around
to passing it up.

there are other warnings in the tree, found by newer clang, that
people should review.  Some of them are just noise, in particular
relating to missing {}.

Theo Buehler  wrote:

> clang 10 complains:
> 
> /usr/src/usr.sbin/vmctl/vmctl.c:792:35: warning: comparing a pointer to a 
> null character
>   constant; did you mean to compare to NULL? [-Wpointer-compare]
> '/')) == NULL || ++tty == '\0')
>   ^~~~
>   (void *)0
> The intent is probably rather this:
> 
> Index: vmctl.c
> ===
> RCS file: /var/cvs/src/usr.sbin/vmctl/vmctl.c,v
> retrieving revision 1.74
> diff -u -p -r1.74 vmctl.c
> --- vmctl.c   11 Mar 2020 12:47:49 -  1.74
> +++ vmctl.c   2 Sep 2020 18:06:47 -
> @@ -789,7 +789,7 @@ print_vm_info(struct vmop_info_result *l
>   tty = "-";
>   /* get tty - skip /dev/ path */
>   else if ((tty = strrchr(vmi->vir_ttyname,
> - '/')) == NULL || ++tty == '\0')
> + '/')) == NULL || *++tty == '\0')
>   tty = list[i].vir_ttyname;
>  
>   (void)fmt_scaled(vir->vir_used_size, curmem);
> 



Re: shrinking and growing reallocs: a theoretical? bad case for performance

2020-08-31 Thread Theo de Raadt
> Taking advantage of the sparse address space is smart and as 64-bit
> is now the norm, that space is even sparser.

Fundamentally this is moving various forms of pressure to the kernel,
which does not do the best job yet.

The pivot code in mmap for new mappings isn't entirely bug-free so we've
avoided it turning it on.  The idea of that code is be random as
neccessary -- creating "unknowable addresses", but in doing so avoid
fragmenting the address space excessively.  Excessive fragmentation in turn
fragmentations allocation in multi-level page-tables, and that in turn
results in excessive TLB pressure.  Which is difficult to gauge since things
keep working, but brings in a big performance cost.

Basically we were brave to do very high amounts of randomization early on.
At a cost.  But our work to improve the cost isn't finished.



Re: shrinking and growing reallocs: a theoretical? bad case for performance

2020-08-31 Thread Theo de Raadt
> If I am interpreting this correctly, realloc could be used to groom/shape
> the heap such that future allocations are less random and more predictable?

Traditionally the word "heap" refers to the zone of allocations in a
sbrk allocator, meaning things are packed tightly together in a known
space, and ordering of the objects inside that produces very low
variability.  I recommend against using the word heap, especially when
today we are using large-address space systems.

Additionally I think this phrasing forgets there are many many objects
in play, not just the ones being realloc'd.  Those objects disrupt the
available space by being allocated and freed.  Object allocation isn't
entirely controlled by the (small) malloc cache.

I guess the theory is that an attacker will succeed because a few realloc'd
objects don't 'relocate' as much as expected.

I don't believe this is likely.  I think we have placed a reasonable
number of hurdles at various levels with an eye on compute cost... we
recognize if we reject standard compsci "caching strategies" too much,
then perforance still stink.



Re: sensor value last change time not updated?

2020-08-25 Thread Theo de Raadt
Paul de Weerd  wrote:

> On Tue, Aug 25, 2020 at 09:27:20PM +0200, Mark Kettenis wrote:
> | > I've dug out my stash of weird usb devices and found another sensor (a
> | > uthum(4), with only temperature support).  I have a few other sensors
> | > in live machines too (acpitz(4), cpu(4), admtemp(4), it(4), maybe some
> | > more) that I could look into.
> | > 
> | > Is there any interest in adding support for setting the tv member for
> | > non-time sensitive sensors?  Or should I drop this quest?
> | 
> | I don't understand the point.  None of the sensor drivers set that
> | member except the timedelta sensors.  I don't think adding code to do
> | so to all sensor drivers makes sense.
> 
> I'm inspecting it to only register "new" samples (even if the value
> itself doesn't change).  My logic is that if the tv member has
> changed, then the sensor value has been updated, so there's new
> "data".  The fact that it's the same temperature / humidity / other
> sensed value can also be interesting.
> 
> But if that doesn't make sense, then I can drop the patches and just
> do periodic sampling at the same interval the kernel uses (which I've
> not found yet, it seems that at least ugold(4) just sends data
> periodically (every ~6 seconds) which the kernel then presents to
> userland through sysctl).

What you are doing wasn't the purpose of the time field.  It was added
only for time sensors, and it looks like someone added it to other
sensors.  And now it must suddenly be for all of them??

Collecting the microtime on every sensor read, on every time through
the processing loop, is not free.



Re: sensor value last change time not updated?

2020-08-25 Thread Theo de Raadt
Mark Kettenis  wrote:

> > Date: Tue, 25 Aug 2020 21:19:19 +0200
> > From: Paul de Weerd 
> > 
> > Hi all,
> > 
> > I've dug out my stash of weird usb devices and found another sensor (a
> > uthum(4), with only temperature support).  I have a few other sensors
> > in live machines too (acpitz(4), cpu(4), admtemp(4), it(4), maybe some
> > more) that I could look into.
> > 
> > Is there any interest in adding support for setting the tv member for
> > non-time sensitive sensors?  Or should I drop this quest?
> 
> I don't understand the point.  None of the sensor drivers set that
> member except the timedelta sensors.  I don't think adding code to do
> so to all sensor drivers makes sense.

That is correct.  Non-time drivers should probably report 0 in that field.

And to follow, it means the microtime() calls should be deleted from
other non-time-sensor drivers where it occurs.



Re: unbound(8): disable explicit port randomisation

2020-08-24 Thread Theo de Raadt
i've discusssed this offline with florian (many times, over the years)

It is quite possible there will be subtle behaviour changes, but in a
system configuration where "other programs on the machine are using also
ports quickly", we both expect unbound will behave *better* using kernel
support rather than doing the port probing itself.  Port probing to
detect what is available is simply a crazy workaround for systems which
don't have a way to perform the magic we do.

Florian Obser  wrote:

> With the update sthen@ just put in we can enable this:
> 
>   --disable-explicit-port-randomisation
>   disable explicit source port randomisation and rely
>   on the kernel to provide random source ports
> 
> OK?
> 
> diff --git Makefile.bsd-wrapper Makefile.bsd-wrapper
> index ff9bc927592..c4abf8dbb97 100644
> --- Makefile.bsd-wrapper
> +++ Makefile.bsd-wrapper
> @@ -17,7 +17,8 @@ CONFIGURE_OPTS_UNBOUND= --enable-allsymbols \
>   --with-rootkey-file=/var/unbound/db/root.key \
>   --with-conf-file=${CHROOTDIR}/etc/unbound.conf \
>   --with-username=_unbound \
> - --disable-shared
> + --disable-shared \
> + --disable-explicit-port-randomisation
>  
>  # do not remove, breaks unwind(8)
>  CONFIGURE_OPTS_UNBOUND+= --without-pthreads
> 
> 
> 
> -- 
> I'm not entirely sure you are real.
> 



Re: getitimer(2), setitimer(2): merge critical sections

2020-08-17 Thread Theo de Raadt
Scott Cheloha  wrote:

> There is one behavior change: in the setitimer(2) swap case it is now
> possible to EFAULT on copyout(9) *after* you have written the new
> timer value and (possibly) started the ITIMER_REAL timeout.
> 
> For example, the following code now yields EFAULT even though a new
> oneshot timer has been started successfully.
> 
>   struct itimerval new;
>   int error;
> 
>   new.it_value.tv_sec = 1;
>   new.it_value.tv_usec = 0;
>   timerclear(_interval);
>   error = setitimer(ITIMER_REAL, , 0xdeadbeef);
>   if (error)
>   warn("setitimer");
> 
> I don't think there is a way to avoid this without introducing a bunch
> of extra complexity.  The critical section is protected by a mutex and
> copyout(9) can sleep, so we have to wait until we leave the critical
> section to copyout(9).  If we leave the mutex to do the copyout(9)
> before writing the new timer value then the swap is no longer atomic.
> Of course, this is not an issue *now*, but when the syscalls are
> unlocked you will lose atomicity.
> 
> Personally I don't think this is a huge deal.  If you're getting
> EFAULT there is a bigger problem in your code.

Let's go back to this first mail.

I suspect it is OK to update the timout, even if the final copyout (and
syscall) then returns EFAULT.

It looks like historical 4.4BSD was "copyout then update".  FreeBSD is
"update them copyout".

I certainly don't think it is worthwhile creating a problem which
is somewhat similar to a TOCTOU.  Even your proposal to do the
address-space range check is a TOCTOU, it fixes nothing since the
address space can still be flipped).

What do other systems do?





Re: getitimer(2), setitimer(2): merge critical sections

2020-08-14 Thread Theo de Raadt
> It has occurred to me that we could do a trial copyout(9) in
> sys_setitimer() before entering the critical section.  This is a *bit*
> wasteful, but is relatively inexpensive and narrows the behavior
> change I mentioned down to truly improbable cases involving multiple
> threads and munmap(2).

That sounds scary.  You are touching userland memory twice, and I could
imagine a situation where it isn't the same memory because it gets
swapped out in a shared storage situation.

It sounds terribly wrong to do that.



Re: man find -exec -- a little bit more hand-holding

2020-08-14 Thread Theo de Raadt
Christian Weisgerber  wrote:

> On 2020-08-14, Jason McIntyre  wrote:
> 
> > - i cannot work out what is with the \! examples. i know we try to make
> >   entries work for both csh and sh style shells, but stuff like this
> >   works without escaping:
> >
> > $ find . ! -type f
> 
> Going through the CVS and SCCS history, I see that the examples
> came with a rewrite of find(1) at Berkeley 30 years ago.
> 
> csh's behavior that "for convenience, a `!' is passed unchanged
> when it is followed by a blank, tab, newline, `=' or `('" has been
> documented as such at least since the start of the CSRG repository
> in 1985.
> 
> bash, whose history substitution was modeled on csh, also shares
> this behavior.
> 
> I think it was never necessary to escape the '!' and the man page
> examples were written with an abundance of caution and a lack of
> understanding of csh's exact replacement mechanism.

Yep, so I think that particular escape should be deleted.



Re: Fwd: explicit_bzero vs. alternatives

2020-08-11 Thread Theo de Raadt
Philipp Klaus Krause  wrote:

> Am 11.08.20 um 02:48 schrieb Damien Miller:
> > 
> > We went with explict_bzero because our only use-case for this was
> > safe erasure that could not be elided by the compiler.
> > 
> > I don't see any need for explicit_memset() - if anything depends on
> > the overwritten value then simple memset() should be sufficient as
> > the compiler should detect the dependency and refuse to elide the
> > memset() to begin with.
> 
> However, for an explicit_memset-like function, a good C implementation
> would try to execute it as early as possible, while plain memset could
> be moved to a later point in the program by optimizations.

The goal is to to clear a transient object after last use, not ensure
the transient space is filled with a specific value.  Often times we
need the object initialized also, I can assure you zeroes are always
the correct value for initialization.  A memset with a special value is
no different than post-zeroing handling of the object.  I have never seen
a benefit to what you propose.

> > Hopefully C2X is taking a more broad approach to this problem than
> > considering new library calls. Over-eager optimisation (especially when
> > done at link-time over the whole program) is a major for anyone trying
> > to write safe C code.
> 
> I don't think a broader approach could work.

We need a broad approach to keep transport equipment not jumping off the
rails or falling from the sky.  It is likely people have or will
eventually die as a result of C's hand-wavy reduction in accurate
translation, and the continued justification of the error is
astounding. Changing the rules of execution of pre-existing code and
willfully dismissing and disclaiming all consequences is ethically
wrong.

> In general, the standard is
> only concerned with state observable in the C abstract machine.
> Everything else can only be hinted at (e.g. via volatile or something
> like bzero/memset_explicit, etc).

Again, wow.  volatile was gutted by the standard and didn't work.
bzero calls started being elided.



Re: cat(1): add more restrictive pledge(2)

2020-07-31 Thread Theo de Raadt
As a general rule, pledge uses isn't supposed to make programs
more complicated.

Stuart Henderson  wrote:

> On 2020/07/31 00:07, tempmai...@firemail.cc wrote:
> > I have to say I'm only a beginner to C but hopefully my patch is
> > good.
> > 
> > This patch adds a second and more restrictive pledge (only "stdio"
> > instead of "stdio rpath") after the getopt loop if there is no
> > input file or if the input file is "-" (stdin) or a sequence of
> > repeated instances of "-". It doesn't move argv past the last "-",
> > and doesn't pledge if it runs into an input file other than "-".
> > 
> > I've compiled it and tested it with ktrace(1) and kdump(1) and it
> > appears to work as expected and pledge correctly with at least
> > these invocations:
> > $ echo test | ./cat -uv # pledge("stdio", NULL);
> > $ echo test | ./cat -uv -   # pledge("stdio", NULL);
> > $ echo test | ./cat # pledge("stdio", NULL);
> > $ echo test | ./cat - - -   # pledge("stdio", NULL);
> > $ echo test | ./cat -   # pledge("stdio", NULL);
> > $ echo test | ./cat - cat.c # pledge("stdio rpath", NULL);
> > $ echo test | ./cat cat.c - # pledge("stdio rpath", NULL);
> > 
> > 
> > 
> > Index: bin/cat/cat.c
> > ===
> > RCS file: /cvs/src/bin/cat/cat.c,v
> > retrieving revision 1.27
> > diff -u -p -u -p -r1.27 cat.c
> > --- bin/cat/cat.c   28 Jun 2019 13:34:58 -  1.27
> > +++ bin/cat/cat.c   30 Jul 2020 23:21:14 -
> > @@ -94,7 +94,26 @@ main(int argc, char *argv[])
> > "usage: %s [-benstuv] [file ...]\n", __progname);
> > return 1;
> > }
> > +   argc -= optind;
> > argv += optind;
> > +
> > +   if (argc) {
> > +   if (!strcmp(*argv, "-")) {
> > +   do {
> > +   if (argc == 1) {
> > +   if (pledge("stdio", NULL) == -1)
> > +   err(1, "pledge");
> > +   argc--, argv++;
> > +   break;
> > +   } else
> > +   argc--, argv++;
> > +   } while (argc && !strcmp(*argv, "-"));
> > +   argc++, argv--;
> > +   }
> > +   } else {
> > +   if (pledge("stdio", NULL) == -1)
> > +   err(1, "pledge");
> > +   }
> > 
> > if (bflag || eflag || nflag || sflag || tflag || vflag)
> > cook_args(argv);
> > 
> 
> The improvement is fairly small; cat doesn't have network access or the
> ability to write files with the previous pledge. Is this worth the
> considerable extra complexity?
> 
> It's hard to get a feel for whether the argc/argv manipulation is correct.
> 



Re: ssh(1), getrrsetbyname(3), SSHFP and DNSSEC

2020-07-29 Thread Theo de Raadt
Florian Obser  wrote:

> On Wed, Jul 29, 2020 at 03:51:17PM +0200, Sebastian Benoit wrote:
> > If i remember correctly, the fallout was caused by EDNS but i might be
> > wrong. The unbound commit caused a developer some headscratching, because
> > his upstream internet did not work with such packets, which led to immediate
> > backout of the change, because a default config that does not work is not
> > good.
> 
> It was time. Running DNSSEC validation on a system without an RTC is
> not a good idea. NTP could not fix this because it depends on working
> DNS. This has since been addressed by Otto.

Addressed as well as possible, but perfection is not achievable.

ntpd (which is run by default) tries aggressively to repair the clock at
boot.  Later on, it is not so aggressive at correcting the clock.

For real computers, this is of no concern, it will work fine.

But on machines without a battery RTC, if internet operation is weak at
boot, but becomes better later on, there is still a window of time
(before ntpd slowly corrects the clock) where time is incoherent and
weird stuff can happen.

So my recommendation is to buy real computers.  And garbage can misbehave
like garbage will.



Re: hostname.if '!' commands and rdomains

2020-07-29 Thread Theo de Raadt
Claudio Jeker  wrote:

> On Wed, Jul 29, 2020 at 04:43:18PM +0200, Klemens Nanni wrote:
> > On Wed, Jul 29, 2020 at 05:33:14PM +0300, Kapetanakis Giannis wrote:
> > > Wouldn't this break those who already have
> > > !route -T2 
> > > 
> > > in their hostname.if files?
> > No,
> > 
> > $ route -T1 exec id -R
> > 1
> > $ route -T0 exec route -T1 exec id -R
> > 1
> 
> But:
>   $ route -T2 exec id -R
>   2
>   $ route -T2 exec route -T0 exec id -R
>   route: setrtable: Operation not permitted
> 
> Only root can change the rdomain if it is currently != 0.

That worry was stated in my email, but not so accurately, thank you.
So now you can't make a rdomain-0 !command in the global scope.

And if we start playing with "early !commands run in 0", that gets
even more messy.



Re: hostname.if '!' commands and rdomains

2020-07-29 Thread Theo de Raadt
You were already able to execute a !command inside the rdomain, either
by specifying the rdomain (on commands which can do that) or by using
route -T manually.

But now, you can't easily execute commands *outside the rdomain*, and
there are some things folk might want to do.

Also, there is an order of evaluation.  Commands before the rdomain
keywords are outside, but commands afterwards are in the rdomain.  That
troubles me a little, especially becuase it's another piece which will
be difficult to document.

Matthieu Herrb  wrote:

> Hi,
> 
> When I'm configuring an interface with a spécific rdomain, I'd assume
> that '!' commands (especially /sbin/route commands) are executed in
> the rdomain for this interface.
> 
> I know that parsing this file is complex and somehow fragile but still
> I tried to write a patch.
> 
> What do you think ?
> 
> Of course I'm ok with any enhancements / fixes to my shell foo.
> 
> --- netstart.orig Wed Jul 29 11:19:53 2020
> +++ netstart  Wed Jul 29 11:52:39 2020
> @@ -67,8 +67,16 @@
>   _cmds[${#_cmds[*]}]="ifconfig $_if ${_c[@]} up;dhclient $_if"
>   V4_DHCPCONF=true
>   ;;
> + rdomain) ((${#_c[*]} == 2)) || return
> + _cmds[${#_cmds[*]}]="ifconfig $_if rdomain ${_c[_name]}"
> + _rdomain=${_c[_name]}
> + ;;
>   '!'*)   _cmd=$(print -- "${_c[@]}" | sed 's/\$if/'$_if'/g')
> - _cmds[${#_cmds[*]}]="${_cmd#!}"
> + if [[ $_rdomain -ne 0 ]]; then
> +_cmds[${#_cmds[*]}]="/sbin/route -T$_rdomain exec 
> ${_cmd#!}"
> + else
> +_cmds[${#_cmds[*]}]="${_cmd#!}"
> + fi
>   ;;
>   *)  _cmds[${#_cmds[*]}]="ifconfig $_if ${_c[@]}"
>   ;;
> 
> -- 
> Matthieu Herrb
> 



Re: Add ability to set control values with video(1)

2020-07-25 Thread Theo de Raadt
Stuart Henderson  wrote:

> On 2020/07/25 09:20, Theo de Raadt wrote:
> > The normal idiom is when last-close happens in a driver, all modal-state
> > is lost and restored to default, and when you use the driver again --
> > the new open gets you a raw configuration which is then changed via
> > ioctl, before futher use.
> 
> Isn't this a bit like volume controls for audio which do exist outside
> of a particular application's opening of the device?

That is soft state, and operate in a global experience context.

Please also consider more than volume controls.  With audio, I think
it is the only major knob, but what's being proposed is a big collection
of knobs.

I'm not sure I see the set of camera ops the same way as volume control.

Also see my message relating to 'reset', back to the default state.
No such thing exists in audio.

I think this is the difference between subsystem (soft state), versus
driver state (much more hard state)





Re: Add ability to set control values with video(1)

2020-07-25 Thread Theo de Raadt
Matthieu Herrb  wrote:

> On Sat, Jul 25, 2020 at 09:17:24AM -0600, Theo de Raadt wrote:
> > Marcus Glocker  wrote:
> > 
> > > Instead of introducing the CLI parameter controls in video(1), we could
> > > introduce videoctl(1) in base, same as we have with audioctl(1).  This
> > > also gives us the possibility to only display the current video
> > > parameters.
> > 
> > I must have missed something.  Why does it need to be a seperate comment.
> > Won't this produce confusion?  Why can video do this?
> > 
> > Is it really correct for the video hardware to have persistant settings?
> > Would it not be better if the required mode was always commanded when
> > a video is being recorded?
> 
> I've not followed UVC hardware too closely; it seems that some of the
> cameras are always fully automatically adjusting their parameters,
> while others allow for manual setting that remains between device
> open().
> 
> And some of the controls are not available when video(4) is used from
> a browser (for conferencing).
> 
> On a 2nd thought having that integrated with video(1) allows to
> control the values with visual feedback so it makes sense to keep only
> one program. But ihmo it would be more user friendly if the command
> line syntax was more regular with audio or wscons control programs.

My primary concern is about a user changing settings which then persist
past close.

Upon re-open, how do they know what mode they are in?

I understand the default mode for a camera might not be nice.  But at
least it is a default.  If the previous use of the camera put it into
a nasty mode, does this mean all new users must first force default?

Now you don't know what mode it is in.  As a result, you must *always*
demand change into a specific mode.  Rather than making things simpler,
doesn't use of a camera become potentially more complicated?



Re: Add ability to set control values with video(1)

2020-07-25 Thread Theo de Raadt
  $ video -c white_balance_temperature=6500
  $ video
  $ video -c reset
  $ video

BTW, no other subsystem of ours has this "reset" type thing.

A few subsystems have persistance, but persistant modes are usually only
in the software layer, not in the hardware layer.

The normal idiom is when last-close happens in a driver, all modal-state
is lost and restored to default, and when you use the driver again --
the new open gets you a raw configuration which is then changed via
ioctl, before futher use.



Re: Add ability to set control values with video(1)

2020-07-25 Thread Theo de Raadt
Marcus Glocker  wrote:

> Instead of introducing the CLI parameter controls in video(1), we could
> introduce videoctl(1) in base, same as we have with audioctl(1).  This
> also gives us the possibility to only display the current video
> parameters.

I must have missed something.  Why does it need to be a seperate comment.
Won't this produce confusion?  Why can video do this?

Is it really correct for the video hardware to have persistant settings?
Would it not be better if the required mode was always commanded when
a video is being recorded?



Re: mailwrapper: hostsat and purgestat symlinks

2020-07-23 Thread Theo de Raadt
Since there are mailers which want these features, I think removing
this out of base is silly.  mailwrapper exists to abstract it, and
why shouldn't it remain as-is?

Todd C. Miller  wrote:

> On Thu, 23 Jul 2020 21:41:53 +0200, Klemens Nanni wrote:
> 
> > Ping.
> >
> > Feedback? Objections? OK?
> 
> Not OK as-is.
> 
> This will break hoststat/purgestat for anyone who uses the sendmail
> port.  See mail/sendmail/files/mailer.conf.sendmail in ports for
> how this is used.
> 
> However, since these are really sendmail-specific we could consider
> adding moving the links to the sendmail package and install them
> in /usr/local/bin.
> 
>  - todd
> 



Re: [Patch] httpd.h - delete unused field and enum

2020-07-21 Thread Theo de Raadt
Yeah, fine.

Sebastian Benoit  wrote:

> Ross L Richardson(open...@rlr.id.au) on 2020.07.15 14:49:23 +1000:
> > Field kv_type in struct kv is not used.  As that's the only use of
> > enum key_type, delete them both.
> 
> This is probably because its a copy from relayd.
> I dont think there is a reason to keep it.
> 
> i'd like to commit, anyone else to ok it?
> 
> 
> > 
> > Ross
> > 
> > 
> > Index: httpd.h
> > ===
> > RCS file: /cvs/src/usr.sbin/httpd/httpd.h,v
> > retrieving revision 1.146
> > diff -u -p -r1.146 httpd.h
> > --- httpd.h 9 Feb 2020 09:44:04 -   1.146
> > +++ httpd.h 15 Jul 2020 04:45:16 -
> > @@ -121,24 +121,12 @@ struct ctl_flags {
> > uint8_t  cf_tls_sid[TLS_MAX_SESSION_ID_LENGTH];
> >  };
> >  
> > -enum key_type {
> > -   KEY_TYPE_NONE   = 0,
> > -   KEY_TYPE_COOKIE,
> > -   KEY_TYPE_HEADER,
> > -   KEY_TYPE_PATH,
> > -   KEY_TYPE_QUERY,
> > -   KEY_TYPE_URL,
> > -   KEY_TYPE_MAX
> > -};
> > -
> >  TAILQ_HEAD(kvlist, kv);
> >  RB_HEAD(kvtree, kv);
> >  
> >  struct kv {
> > char*kv_key;
> > char*kv_value;
> > -
> > -   enum key_typekv_type;
> >  
> >  #define KV_FLAG_INVALID 0x01
> >  #define KV_FLAG_GLOBBING0x02
> > 
> 



Re: switch default MANPAGER from more(1) to less(1)

2020-07-19 Thread Theo de Raadt
> about -s: it's inclusion probably comes from a time when there was an
> annoying bug in nroff that made our man pages randomly display a number
> of blank lines in the middle of a page. -s mitigated that somewhat.

that is also what I recall.



Re: switch default MANPAGER from more(1) to less(1)

2020-07-19 Thread Theo de Raadt
Good.

The user-interface of less is slightly more refined, and definately
preferred.

Ingo Schwarze  wrote:

> Hi,
> 
> currently, if neither the MANPAGER nor the PAGER environment variable
> is set, man(1) uses "more -s" as the manual page pager.  I am quite
> sure that the only reason i did this is that i thought this behaviour
> was required by POSIX.
> 
> But it is not:
> 
>   https://pubs.opengroup.org/onlinepubs/9699919799/utilities/man.html
> 
>   "If the PAGER variable is null or not set,
>the command shall be either "more" or another
>paginator utility documented in the system documentation."
> 
> Right now, i even failed to find any indication that POSIX ever
> required more(1) as the default for this purpose.  I no longer
> understand where i got that idea from in the first place.
> 
> That said, on OpenBSD, the pager we provide is less(1).  In effect,
> it is less(1) even when called as more(1), albeit with minor
> differences in the default behaviour.  On top of that, our man(1)
> utility heavily relies on less(1) features that tradional more(1)
> wouldn't even have, in particular tagging support (:t).
> 
> So, i would find it logical to use less(1) as the default pager
> instead of more(1).
> 
> Same thing for the "-s" option: i thought it was required by POSIX,
> but it isn't.  I also provides little benefit, if any, and it may
> occasionally break output, in the rare case where two consecutive
> blank lines are syntacically significant (for example in an EXAMPLES
> section presenting a code sample).  Besides, if the author deliberately
> chose to put two consecutive blank lines, i don't see why man(1)
> should squeeze them and override the author's intent.
> 
> Is anybody concerned about the following patch, or would you agree
> with it?
> 
> Yours,
>   Ingo
> 
> 
> Index: apropos.1
> ===
> RCS file: /cvs/src/usr.bin/mandoc/apropos.1,v
> retrieving revision 1.41
> diff -u -p -r1.41 apropos.1
> --- apropos.1 22 Nov 2018 12:32:10 -  1.41
> +++ apropos.1 19 Jul 2020 15:18:30 -
> @@ -340,7 +340,7 @@ types appearing in function arguments in
>  Any non-empty value of the environment variable
>  .Ev MANPAGER
>  is used instead of the standard pagination program,
> -.Xr more 1 ;
> +.Xr less 1 ;
>  see
>  .Xr man 1
>  for details.
> @@ -363,7 +363,7 @@ Specifies the pagination program to use 
>  .Ev MANPAGER
>  is not defined.
>  If neither PAGER nor MANPAGER is defined,
> -.Xr more 1
> +.Xr less 1
>  .Fl s
>  is used.
>  Only used if
> Index: main.c
> ===
> RCS file: /cvs/src/usr.bin/mandoc/main.c,v
> retrieving revision 1.253
> diff -u -p -r1.253 main.c
> --- main.c15 Jun 2020 17:25:03 -  1.253
> +++ main.c19 Jul 2020 15:18:31 -
> @@ -1194,7 +1194,7 @@ spawn_pager(struct outstate *outst, char
>   if (pager == NULL || *pager == '\0')
>   pager = getenv("PAGER");
>   if (pager == NULL || *pager == '\0')
> - pager = "more -s";
> + pager = "less";
>   cp = mandoc_strdup(pager);
>  
>   /*
> Index: man.1
> ===
> RCS file: /cvs/src/usr.bin/mandoc/man.1,v
> retrieving revision 1.37
> diff -u -p -r1.37 man.1
> --- man.1 17 Jun 2020 19:41:25 -  1.37
> +++ man.1 19 Jul 2020 15:18:31 -
> @@ -275,7 +275,7 @@ is case insensitive.
>  Any non-empty value of the environment variable
>  .Ev MANPAGER
>  is used instead of the standard pagination program,
> -.Xr more 1 .
> +.Xr less 1 .
>  If
>  .Xr less 1
>  is used, the interactive
> @@ -329,7 +329,7 @@ Specifies the pagination program to use 
>  .Ev MANPAGER
>  is not defined.
>  If neither PAGER nor MANPAGER is defined,
> -.Xr more 1
> +.Xr less 1
>  .Fl s
>  is used.
>  .El
> Index: mandoc.1
> ===
> RCS file: /cvs/src/usr.bin/mandoc/mandoc.1,v
> retrieving revision 1.168
> diff -u -p -r1.168 mandoc.1
> --- mandoc.1  15 Jun 2020 18:05:25 -  1.168
> +++ mandoc.1  19 Jul 2020 15:18:31 -
> @@ -650,7 +650,7 @@ It never affects the interpretation of i
>  Any non-empty value of the environment variable
>  .Ev MANPAGER
>  is used instead of the standard pagination program,
> -.Xr more 1 ;
> +.Xr less 1 ;
>  see
>  .Xr man 1
>  for details.
> @@ -664,7 +664,7 @@ Specifies the pagination program to use 
>  .Ev MANPAGER
>  is not defined.
>  If neither PAGER nor MANPAGER is defined,
> -.Xr more 1
> +.Xr less 1
>  .Fl s
>  is used.
>  Only used if
> 



Re: timekeep: fixing large skews on amd64 with RDTSCP

2020-07-16 Thread Theo de Raadt
> Note the third sentence.
> 
> Given that, I reason that a serializing instruction before *and* after
> the RDTSC should freeze it in place.

I haven't seen anyone read it that way.



Re: Use VGA text mode palette RGB values in rasops(9)

2020-07-15 Thread Theo de Raadt
> So here is a new iteration taking feedback into account, using the
> #if WS_DEFAULT_BG == WSCOL_WHITE check for clarity, and also switching
> the foreground color of printed kernel messages to light cyan to improve
> contrast and readability.

I really dislike how two issues are being mixed into one diff.



Re: iked.conf.5: provide gre example

2020-07-15 Thread Theo de Raadt
It is extremely unwise to use DNS names at this level (or things which
look like DNS names).  The same problems that pf has with DNS, are
present here.  You really don't want people to get into this habit.

Klemens Nanni  wrote:

> Here's an addition to EXAMPLES for one of my frequent use cases that
> finally "just works".
> 
> First transport mode for child SAs was implemented, then a few
> interoperability issues have been identified with peers other than iked,
> now tobhe fixed pubkey (`rsa' ikeauth, default) usage based on this.
> 
> Feedback? OK?
> 
> Index: iked.conf.5
> ===
> RCS file: /cvs/src/sbin/iked/iked.conf.5,v
> retrieving revision 1.71
> diff -u -p -r1.71 iked.conf.5
> --- iked.conf.5   10 Jul 2020 21:23:47 -  1.71
> +++ iked.conf.5   12 Jul 2020 14:32:00 -
> @@ -1014,6 +1014,23 @@ ikev2 "subnet" esp from 10.0.3.0/24 to 1
>  ikev2 esp from 10.0.5.0/30 to 10.0.5.4/30 peer 192.168.1.2
>  ikev2 esp from 10.0.5.8/30 to 10.0.5.12/30 peer 192.168.1.3
>  .Ed
> +.Pp
> +This example encrypts a
> +.Xr gre 4
> +tunnel from local machine A to peer D using FQDN-based public key
> +authentication.
> +.Ar transport
> +mode is used to avoid duplicate encapsulation of GRE;
> +.Ar dstid
> +is set explicitly to the peer's FQDN such that its public key is looked up 
> even
> +if the peer does not send its FQDN as peer ID:
> +.Bd -literal -offset indent
> +ikev2 transport \e
> + proto gre \e
> + from A.example.com to D.example.com \e
> + peer D.example.com \e
> + dstid D.example.com
> +.Ed
>  .Sh SEE ALSO
>  .Xr enc 4 ,
>  .Xr ipsec 4 ,
> 



Re: no need for "extern optind" etc

2020-07-15 Thread Theo de Raadt
If I remember the story right, historically unistd.h did not declare
those two, so to encourage portable reuse of parts of our tree they
remained.

You touch the ssh directory, and I think if that was commited you would
break downstream users, unless -portable handled it.

Can a posix laywer pipe up?


Jan Stary  wrote:

> This is in the vein of
> https://marc.info/?l=openbsd-cvs=158170787221615=2
> 
>  declares "extern int optind" and friends,
> so there is no need to declare them again.
> Still builds on current/amd64.
> 
> Leaving out gnu, nsd, unbound (third party)
> and tic (is that third party)?
> 
> Also leaving out pr and rcs
> who do their own thing.
> 
>   Jan
> 
> 
> Index: sys/dev/microcode/aic7xxx/aicasm.c
> ===
> RCS file: /cvs/src/sys/dev/microcode/aic7xxx/aicasm.c,v
> retrieving revision 1.16
> diff -u -p -r1.16 aicasm.c
> --- sys/dev/microcode/aic7xxx/aicasm.c19 Dec 2014 22:44:58 -  
> 1.16
> +++ sys/dev/microcode/aic7xxx/aicasm.c15 Jul 2020 14:45:14 -
> @@ -112,8 +112,6 @@ int main(int argc, char *argv[]);
>  int
>  main(int argc, char *argv[])
>  {
> - extern char *optarg;
> - extern int optind;
>   int  ch;
>   int  retval;
>   char *inputfilename;
> Index: usr.bin/env/env.c
> ===
> RCS file: /cvs/src/usr.bin/env/env.c,v
> retrieving revision 1.17
> diff -u -p -r1.17 env.c
> --- usr.bin/env/env.c 28 Oct 2016 07:22:59 -  1.17
> +++ usr.bin/env/env.c 15 Jul 2020 14:45:14 -
> @@ -42,7 +42,6 @@ int
>  main(int argc, char *argv[])
>  {
>   extern char **environ;
> - extern int optind;
>   char **ep, *p;
>   int ch;
>  
> Index: usr.bin/finger/finger.c
> ===
> RCS file: /cvs/src/usr.bin/finger/finger.c,v
> retrieving revision 1.27
> diff -u -p -r1.27 finger.c
> --- usr.bin/finger/finger.c   26 Apr 2018 12:42:51 -  1.27
> +++ usr.bin/finger/finger.c   15 Jul 2020 14:45:14 -
> @@ -76,7 +76,6 @@ PERSON *phead, *ptail;
>  int
>  main(int argc, char *argv[])
>  {
> - extern int optind;
>   extern char *__progname;
>   int ch;
>   char domain[HOST_NAME_MAX+1];
> Index: usr.bin/ftp/cmds.c
> ===
> RCS file: /cvs/src/usr.bin/ftp/cmds.c,v
> retrieving revision 1.84
> diff -u -p -r1.84 cmds.c
> --- usr.bin/ftp/cmds.c18 Nov 2019 04:37:35 -  1.84
> +++ usr.bin/ftp/cmds.c15 Jul 2020 14:45:14 -
> @@ -217,7 +217,6 @@ usage:
>  void
>  mput(int argc, char *argv[])
>  {
> - extern int optind, optreset;
>   int ch, i, restartit = 0;
>   sig_t oldintr;
>   char *cmd, *tp, *xargv[] = { argv[0], NULL, NULL };
> Index: usr.bin/ftp/small.c
> ===
> RCS file: /cvs/src/usr.bin/ftp/small.c,v
> retrieving revision 1.12
> diff -u -p -r1.12 small.c
> --- usr.bin/ftp/small.c   18 Nov 2019 04:37:35 -  1.12
> +++ usr.bin/ftp/small.c   15 Jul 2020 14:45:14 -
> @@ -326,7 +326,6 @@ mabort(int signo)
>  void
>  mget(int argc, char *argv[])
>  {
> - extern int optind, optreset;
>   sig_t oldintr;
>   int xargc = 2;
>   char *cp, localcwd[PATH_MAX], *xargv[] = { argv[0], NULL, NULL };
> Index: usr.bin/getopt/getopt.c
> ===
> RCS file: /cvs/src/usr.bin/getopt/getopt.c,v
> retrieving revision 1.10
> diff -u -p -r1.10 getopt.c
> --- usr.bin/getopt/getopt.c   9 Oct 2015 01:37:07 -   1.10
> +++ usr.bin/getopt/getopt.c   15 Jul 2020 14:45:14 -
> @@ -13,8 +13,6 @@
>  int
>  main(int argc, char *argv[])
>  {
> - extern int optind;
> - extern char *optarg;
>   int c;
>   int status = 0;
>  
> Index: usr.bin/locate/code/locate.code.c
> ===
> RCS file: /cvs/src/usr.bin/locate/code/locate.code.c,v
> retrieving revision 1.21
> diff -u -p -r1.21 locate.code.c
> --- usr.bin/locate/code/locate.code.c 17 Jan 2019 06:15:44 -  1.21
> +++ usr.bin/locate/code/locate.code.c 15 Jul 2020 14:45:14 -
> @@ -106,8 +106,6 @@ int   bgindex(char *);
>  
>  
>  void usage(void);
> -extern int optind;
> -extern int optopt;
>  
>  int
>  main(int argc, char *argv[])
> Index: usr.bin/m4/main.c
> ===
> RCS file: /cvs/src/usr.bin/m4/main.c,v
> retrieving revision 1.87
> diff -u -p -r1.87 main.c
> --- usr.bin/m4/main.c 15 Jun 2017 13:48:42 -  1.87
> +++ usr.bin/m4/main.c 15 Jul 2020 14:45:14 -
> @@ -138,9 +138,6 @@ struct keyblk keywrds[] = {   /* m4 keywor
>  
>  #define MAXKEYS  (sizeof(keywrds)/sizeof(struct keyblk))
>  
> -extern int optind;
> -extern char *optarg;
> -
>  #define MAXRECORD 50
>  static struct 

Re: Initialize sis_ring_init() 'sis_rx_prod' and 'sis_rx_cons' to 0

2020-07-15 Thread Theo de Raadt
At driver attach, cd (inside M_ZERO softc) is 0.

But if a reset (or suspend/resume) happens, then sis_rx_prod/sis_rx_cons
will be within the middle of the ring, and it isn't clear if everything
else in the chip+driver are ready for that.

ok deraadt



Kevin Lo  wrote:

> ok?
> 
> Index: sys/dev/pci/if_sis.c
> ===
> RCS file: /cvs/src/sys/dev/pci/if_sis.c,v
> retrieving revision 1.137
> diff -u -p -u -p -r1.137 if_sis.c
> --- sys/dev/pci/if_sis.c  10 Jul 2020 13:26:38 -  1.137
> +++ sys/dev/pci/if_sis.c  15 Jul 2020 07:11:47 -
> @@ -1285,7 +1285,7 @@ sis_ring_init(struct sis_softc *sc)
>   ld->sis_rx_list[i].sis_ctl = 0;
>   }
>  
> - cd->sis_rx_prod = cd->sis_rx_cons;
> + cd->sis_rx_prod = cd->sis_rx_cons = 0;
>   if_rxr_init(>sis_rx_ring, 2, SIS_RX_LIST_CNT - 1);
>   sis_fill_rx_ring(sc);
>  
> 



Re: energy sensor

2020-07-14 Thread Theo de Raadt
No harm in adding it.


Mark Kettenis  wrote:

> The firmware on the POWER9 machines provides an energy sensor.  This
> sensor measures the energy on/in/at the chip.  I'm not sure what that
> actually means.  But it is different from the energy content of a
> battery which is measured in Watthours.
> 
> Should I add this?  Or should I just skip these sensors?
> 
> Index: sbin/sysctl/sysctl.c
> ===
> RCS file: /cvs/src/sbin/sysctl/sysctl.c,v
> retrieving revision 1.251
> diff -u -p -r1.251 sysctl.c
> --- sbin/sysctl/sysctl.c  29 May 2020 04:42:23 -  1.251
> +++ sbin/sysctl/sysctl.c  14 Jul 2020 17:30:43 -
> @@ -2759,6 +2759,9 @@ print_sensor(struct sensor *s)
>   case SENSOR_VELOCITY:
>   printf("%4.3f m/s", s->value / 100.0);
>   break;
> + case SENSOR_ENERGY:
> + printf("%.2f J", s->value / 100.0);
> + break;
>   default:
>   printf("unknown");
>   }
> Index: usr.bin/systat/sensors.c
> ===
> RCS file: /cvs/src/usr.bin/systat/sensors.c,v
> retrieving revision 1.31
> diff -u -p -r1.31 sensors.c
> --- usr.bin/systat/sensors.c  10 Dec 2018 13:35:54 -  1.31
> +++ usr.bin/systat/sensors.c  14 Jul 2020 17:30:43 -
> @@ -291,6 +291,9 @@ showsensor(struct sensinfo *s)
>   case SENSOR_VELOCITY:
>   tbprintf("%4.3f m/s", s->sn_value / 100.0);
>   break;
> + case SENSOR_ENERGY:
> + tbprintf("%.2f J", s->sn_value / 100.0);
> + break;
>   default:
>   tbprintf("%10lld", s->sn_value);
>   break;
> Index: sys/sys/sensors.h
> ===
> RCS file: /cvs/src/sys/sys/sensors.h,v
> retrieving revision 1.36
> diff -u -p -r1.36 sensors.h
> --- sys/sys/sensors.h 10 Dec 2018 13:35:54 -  1.36
> +++ sys/sys/sensors.h 14 Jul 2020 17:30:43 -
> @@ -53,6 +53,7 @@ enum sensor_type {
>   SENSOR_PRESSURE,/* pressure (mPa) */
>   SENSOR_ACCEL,   /* acceleration (u m/s^2) */
>   SENSOR_VELOCITY,/* velocity (u m/s) */
> + SENSOR_ENERGY,  /* energy (uJ) */
>   SENSOR_MAX_TYPES
>  };
>  
> @@ -80,6 +81,7 @@ static const char * const sensor_type_s[
>   "pressure",
>   "acceleration",
>   "velocity",
> + "energy",
>   "undefined"
>  };
>  #endif   /* !_KERNEL */
> 



Re: empty rc.firsttime when installing

2020-07-14 Thread Theo de Raadt
Denis Fondras  wrote:

> I was upgrading an EdgeRouter and it restarted multiple times instead of 
> booting
> /bsd

Try to produce a log of that problem, instead.



Re: disable libc sys wrappers?

2020-07-13 Thread Theo de Raadt
Fine with me.


Ted Unangst  wrote:

> On 2020-07-09, Theo de Raadt wrote:
> > > Added a -T option to ktrace for transparency. I got ambitious here and 
> > > made
> > > it take suboptions, anticipating that other transparency modifications may
> > > be desired.
> > 
> > Please don't do that.
> 
> Here is a simpler version.
> 
> 
> Index: lib/libc/dlfcn/init.c
> ===
> RCS file: /home/cvs/src/lib/libc/dlfcn/init.c,v
> retrieving revision 1.8
> diff -u -p -r1.8 init.c
> --- lib/libc/dlfcn/init.c 6 Jul 2020 13:33:05 -   1.8
> +++ lib/libc/dlfcn/init.c 13 Jul 2020 17:36:04 -
> @@ -114,6 +114,8 @@ _libc_preinit(int argc, char **argv, cha
>   _timekeep->tk_version != TK_VERSION)
>   _timekeep = NULL;
>   }
> + if (issetugid() == 0 && getenv("LIBC_NOUSERTC"))
> + _timekeep = NULL;
>   break;
>   }
>   }
> Index: usr.bin/ktrace/ktrace.1
> ===
> RCS file: /home/cvs/src/usr.bin/ktrace/ktrace.1,v
> retrieving revision 1.30
> diff -u -p -r1.30 ktrace.1
> --- usr.bin/ktrace/ktrace.1   15 May 2019 15:36:59 -  1.30
> +++ usr.bin/ktrace/ktrace.1   13 Jul 2020 17:38:22 -
> @@ -37,13 +37,13 @@
>  .Nd enable kernel process tracing
>  .Sh SYNOPSIS
>  .Nm ktrace
> -.Op Fl aBCcdi
> +.Op Fl aCcdi
>  .Op Fl f Ar trfile
>  .Op Fl g Ar pgid
>  .Op Fl p Ar pid
>  .Op Fl t Ar trstr
>  .Nm ktrace
> -.Op Fl adi
> +.Op Fl aBdiT
>  .Op Fl f Ar trfile
>  .Op Fl t Ar trstr
>  .Ar command
> @@ -109,6 +109,8 @@ processes.
>  Enable (disable) tracing on the indicated process ID (only one
>  .Fl p
>  flag is permitted).
> +.It Fl T
> +Disable userland timekeeping, making time related system calls more 
> prevalent.
>  .It Fl t Ar trstr
>  Select which information to put into the dump file.
>  The argument can contain one or more of the following letters.
> Index: usr.bin/ktrace/ktrace.c
> ===
> RCS file: /home/cvs/src/usr.bin/ktrace/ktrace.c,v
> retrieving revision 1.36
> diff -u -p -r1.36 ktrace.c
> --- usr.bin/ktrace/ktrace.c   28 Jun 2019 13:35:01 -  1.36
> +++ usr.bin/ktrace/ktrace.c   13 Jul 2020 17:37:06 -
> @@ -100,7 +100,7 @@ main(int argc, char *argv[])
>   usage();
>   }
>   } else {
> - while ((ch = getopt(argc, argv, "aBCcdf:g:ip:t:")) != -1)
> + while ((ch = getopt(argc, argv, "aBCcdf:g:ip:t:T")) != -1)
>   switch ((char)ch) {
>   case 'a':
>   append = 1;
> @@ -140,6 +140,9 @@ main(int argc, char *argv[])
>   usage();
>   }
>   break;
> + case 'T':
> + putenv("LIBC_NOUSERTC=");
> + break;
>   default:
>   usage();
>   }
> @@ -240,9 +243,9 @@ usage(void)
>   " [-u trspec] command\n",
>   __progname);
>   else
> - fprintf(stderr, "usage: %s [-aBCcdi] [-f trfile] [-g pgid]"
> + fprintf(stderr, "usage: %s [-aCcdi] [-f trfile] [-g pgid]"
>   " [-p pid] [-t trstr]\n"
> - "   %s [-adi] [-f trfile] [-t trstr] command\n",
> + "   %s [-aBdiT] [-f trfile] [-t trstr] command\n",
>   __progname, __progname);
>   exit(1);
>  }



Re: download site or stuff from sources with weak password

2020-07-13 Thread Theo de Raadt
The install script has never used all features of all commands hiding
inside it, intentionally, because we don't want to maintain all the
edge conditions.

*OUR* install scripts are meant to reach to *OUR* distribution
infrastructure, and *OUR* distribution infrastructure doesn't use
usernames or passwords options, which are fragile and scary and a
more complicated to handle than a simple regexp.


Also, a reminder to not be crass.  Trying to be crass with me gets
you nowhere, it simply reminds me of your past behaviour, and that
means are wasting your time.




sven falempin  wrote:

> On Mon, Jul 13, 2020 at 2:38 PM Theo de Raadt  wrote:
> 
> > I am sceptical of any need to support what you propose, especially
> > when it isn't documented, and secondly when it is shitty, and
> > outside the scope of the project.
> >
> 
> 
> FTP(1) General Commands ManualFTP(1)
> 
> NAME
>  ftp - Internet file transfer program
> 
> SYNOPSIS
>  ftp [-46AadEegiMmnptVv] [-D title] [-k seconds] [-P port] [-r seconds]
>  [-s srcaddr] [host [port]]
>  ftp [-C] [-o output] [-s srcaddr]
>  ftp://[user:password@]host[:port]/file[/]
> 
> documented here
> 
> 
> -- 
> --
> -
> Knowing is not enough; we must apply. Willing is not enough; we must do



Re: download site or stuff from sources with weak password

2020-07-13 Thread Theo de Raadt
I am sceptical of any need to support what you propose, especially
when it isn't documented, and secondly when it is shitty, and
outside the scope of the project.

sven falempin  wrote:

> ?(+([!@])@)
> 
> is not very smart for something:something@
> but i guess it is enough ?
> 
> ( tabulation should be present below )
> 
> Index: ./distrib/miniroot/install.sub
> ===
> RCS file: /cvs/src/distrib/miniroot/install.sub,v
> retrieving revision 1.1154
> diff -u -p -r1.1154 install.sub
> --- ./distrib/miniroot/install.sub  26 May 2020 16:21:00 -
> 1.1154
> +++ ./distrib/miniroot/install.sub  13 Jul 2020 18:26:42 -
> @@ -1775,7 +1775,7 @@ install_http() {
> HTTP_SERVER=${1%%/*}
> # Repeat loop to get user to confirm server address.
> ;;
> -   ?(http?(s)://)+([A-Za-z0-9:.\[\]_-]))
> +   ?(http?(s)://)?(+([!@])@)+([A-Za-z0-9:.\[\]_-]))
> case $resp in
> https://*)  _tls=force _http_proto=https;;
> http://*)   _tls=no_http_proto=http;;
> 
> --



Re: readlink "/etc/localtime" vs unveil

2020-07-11 Thread Theo de Raadt
Todd C. Miller  wrote:

> On Sat, 11 Jul 2020 13:35:57 -0700, Greg Steuck wrote:
> 
> > The problem seems to be due to Chromium doing a readlink (likely at
> > [1]) on /etc/localtime, which fails. There's a workaround that
> > happens to work for chromium (and firefox).  Just set TZ in your
> > environment.
> 
> You could just add a call to tzset() before unveil() is called.
> That will cause /etc/localtime to be read and its value cached.

I suspect it is trying to be clever for some reason, but clever is
actually stupid.  So tzset() might not help.  



Re: readlink "/etc/localtime" vs unveil

2020-07-11 Thread Theo de Raadt
Greg Steuck  wrote:

> The problem seems to be due to Chromium doing a readlink (likely at
> [1]) on /etc/localtime, which fails.

If that is what it is doing, then it should stop doing so.



Re: disable libc sys wrappers?

2020-07-10 Thread Theo de Raadt
Ingo Schwarze  wrote:

> Hi Theo,
> 
> Theo de Raadt wrote on Fri, Jul 10, 2020 at 10:02:46AM -0600:
> 
> > I also don't see the point of the leading _
> > 
> > Where does that come from?
> > 
> > This isn't a function namespace.  What does it signify, considering
> > no other environment variable uses _ prefixing.
> 
>$ man -kO Ev Ev~^_ | sed 's/ [^_][^ ]*//g'  # chicken scratches!
>   at, _
>   ksh, _
>   port-modules(5) _MODFOO_*
>   sudoers(5) _RLD*

Both of those are ports.  Neither are base.

> So, not absolutely accurate, but very close to the truth.  The
> bsd.port.mk(5) implementation, which does make extensive use of
> variable names with leading underscores in the sense intended by
> Ted (private variable, keep your fingers away!), is probably quite
> a special beast.

I don't agree at all, because this is not a programming namespace.



Re: disable libc sys wrappers?

2020-07-10 Thread Theo de Raadt
I also don't see the point of the leading _

Where does that come from?

This isn't a function namespace.  What does it signify, considering
no other environment variable uses _ prefixing.

I think you are mixing too much stuff in here...



Re: disable libc sys wrappers?

2020-07-09 Thread Theo de Raadt
> Added a -T option to ktrace for transparency. I got ambitious here and made
> it take suboptions, anticipating that other transparency modifications may
> be desired.

Please don't do that.

First off, I see no future in the open/openat idea.  Not just pledge,
not just unveil, not just reduction in ktrace clarity, I see no upside
and only downsides.

But secondly, I see no future in any other ideas of reducing system calls,
since they are the fundamental interface to do a request without magic.

This time thing is magic becuase it has to be (latency and accuracy),
but I see no future for anything else being magic.

If you make -T take options noone will use it, and the exercise is
pointless.  Noone is going to manually set the environment variable,
that much I can tell you also...




Re: disable libc sys wrappers?

2020-07-09 Thread Theo de Raadt
> Call getenv before issetugid.

Why??

issetugid is a syscall (yes, it has overhead) which (amongst other things)
whether you can trust the environment.

But getenv -- that's a call which trusts enough of the environment array's
structure, to do a walk, and find.  Why do the walk, at all, if you aren't
supposed to?

It might not just be the string in the environment, it might be some other
corruption (I can't see one, but WHY do the idiom in a less safe way?)

It is entirely BACKWARDS from what we do elsewhere.

Who suggested you do this?




Re: disable libc sys wrappers?

2020-07-09 Thread Theo de Raadt
Ingo Schwarze  wrote:

> Hi Theo,
> 
> Theo de Raadt wrote on Thu, Jul 09, 2020 at 11:08:38AM -0600:
> > Ingo Schwarze  wrote:
> 
> >> So, what about
> >>   LD_KTRACE_GETTIME
> >> or a similar environment variable name?
> 
> > That naming seems logical.
> > 
> > If it is mostly hidden behind a ktrace flag (-T ?) then I think
> > we're in good shape.
> 
> Technically, the implementation of the new ktrace(1) flag will be
> totally different from the implementation of the existing ktrace -t
> flags.  But from the user perspective, the purpose of the new flag
> is just to "put some more information into the dump file", so
> shouldn't it be just another -t letter?  Like,
> 
>   c   trace system calls
>   T   trace clock_gettime(2) and gettimeofday(2)
> 
> or similar?

Every single one of the -t sub-optionsd are flags passed to the kernel,
and the kernel evaluates what to export.   This is completely not like -t.

> Now that i look at that, and at what you said previously, is it even
> plausible that some user ever wants "-t c" ktracing but does
> specifically *not* want to see clock_gettime(2) and friends?

Sorry, that isn't how this works.

> Unless i'm mistaken, we don't provide a way either to say "i want
> to see system calls, except that i don't want to see chdir(2),
> chmod(2), getuid(2), and mprotect(2)."

And that is not what is happening here.  This is a totally
new condition where libc has a way to "skip" doing "calls to system
calls" but we need a way to expose them *BECAUSE THE DEVELOPMENT PROCESS
NEEDS THE VISIBILITY*.



Re: disable libc sys wrappers?

2020-07-09 Thread Theo de Raadt
Ingo Schwarze  wrote:

> Hi Theo,
> 
> Theo de Raadt wrote on Thu, Jul 09, 2020 at 09:47:26AM -0600:
> 
> > This time, they become invisible.
> [...]
> > There are many circumstances where ltrace is not suitable.
> [...]
> > We fiddle with programs all the time, to inspect them.
> 
> Fair enough, then.
> 
> The following variables already exist according to man -ak Ev~'\ 
>  * LD_PRELOAD
>  * LD_LIBRARY_PATH
>  * LD_BIND_NOW
>  * LD_TRACE_LOADED_OBJECTS
>  * LD_DEBUG
> 
> I susspect the name should:
> 
>  * start with "LD_"
>  * contain "TIME"
>  * contain "SYSCALL" or "KTRACE"

Actually the LD_ in those names refers to ld.so, but ld.so is not
making this decision.  libc is.

It also works for static binaries, because the checking position is
inside libc initialization.

That said, I don't like the originally proposed prefix STDIO_



Re: disable libc sys wrappers?

2020-07-09 Thread Theo de Raadt
Ingo Schwarze  wrote:

> Hi Theo,
> 
> Theo de Raadt wrote on Thu, Jul 09, 2020 at 09:47:26AM -0600:
> 
> > This time, they become invisible.
> [...]
> > There are many circumstances where ltrace is not suitable.
> [...]
> > We fiddle with programs all the time, to inspect them.
> 
> Fair enough, then.
> 
> The following variables already exist according to man -ak Ev~'\ 
>  * LD_PRELOAD
>  * LD_LIBRARY_PATH
>  * LD_BIND_NOW
>  * LD_TRACE_LOADED_OBJECTS
>  * LD_DEBUG
> 
> I susspect the name should:
> 
>  * start with "LD_"
>  * contain "TIME"
>  * contain "SYSCALL" or "KTRACE"
> 
> If i understand correctly, what the user really wants is that
> time-related API calls appear in ktrace; that they cause system
> calls is merely how that is implemented.  Consequently, from the
> user perspective, "KTRACE" is more relevant than "SYSCALL".
> It is also clearer what it does; i assume using the word "SYSCALL"
> would require a lengthy wording like "FORCE_SYSCALL".
> 
> So, what about
> 
>   LD_KTRACE_GETTIME
> 
> or a similar environment variable name?

That naming seems logical.

If it is mostly hidden behind a ktrace flag (-T ?) then I think
we're in good shape.



Re: silicom X710 ixl, unable to query phy types, no sff

2020-07-09 Thread Theo de Raadt
David Gwynne  wrote:

> so the problem is the older api doenst support the "get phy types"
> command, or the sff commands. should we silence the "get phy types"
> error output? is there a better errno to use when the sff command
> isn't supported? should we add something to the manpage? should that
> something be "i'm not angry, just disappointed"?

I don't think it is the errno that matters, it is this:

> ifconfig: ixl0 transceiver: Input/output error

Way I read the code, there's no other cause for EIO in that place
in ifconfig.  Why print the errno translation.  Why not just something
vague like this:

ixl3: flags=8802 mtu 1500
lladdr 00:e0:ed:75:a5:5f
index 6 priority 0 llprio 3
media: Ethernet autoselect (10GbaseLR full-duplex)
status: active
transceiver: cannot determine

Or something like that.  Consider it a soft error.




Re: disable libc sys wrappers?

2020-07-09 Thread Theo de Raadt
Ingo Schwarze  wrote:

> Hi,
> 
> i wonder whether there is a layering violation going on here.
> 
> From the user perspective, whether an API call is a syscall or a
> mere library function doesn't make any difference, it's an
> implementation detail.

>From the developer's perspective it is a HUGE DEAL, because ktrace
now doesn't show calls to gettimeofday / clock_gettime calls in the
code most often traced (event loops).

> API calls have often moved from being library
> functions to syscall and vice versa, witnessed by the blurry line
> between sections 2 and 3 in the manual (which is not a problem at
> all).

When that happened, they remained visible.

This time, they become invisible.

o you say the user wants to see whether their program calls
> some time-related API functions, and in the past, ktrace was useful
> for that.  On a machine where syscalls for the purpose are no longer
> needed due to pirofti@'s optimization, does it really make sense to
> force useless syscalls just to be able to see the API calls in ktrace?
> Wouldn't it make more sense to instead simply use ltrace(1), which IIUC
> is intended for just that purpose, tracing API calls?

There are many circumstances where ltrace is not suitable.  It exposes
the wrong interface, and fails to show the parameters in the expected
fashion.

> Someone said setting up ltrace(1) is kind of harder, while ktrace(1)
> is very easy to use (i can confirm the latter, i use ktrace regularly).
> So maybe the real problem is to make sure ltrace(1) is about as easy
> to use as ktrace(1)?  (Not sure what exactly the problem(s) with ltrace
> usability are, if any, i didn't use it much so far.)

I don't want to see the layers of functions, which means I don't want
to use ltrace.  I want ktrace to be useful.

> It feels odd that a user would fiddle with the internal way how API
> calls are implemented, in particular using something as scary as
> environment variables, just to *inspect* what their program is doing...

We fiddle with programs all the time, to inspect them.  We add debug printf's!
How can doing this in a more subtle way be harmful?



Re: pshared semaphores

2020-07-09 Thread Theo de Raadt
Mark Kettenis  wrote:

> > From: "Ted Unangst" 
> > Date: Thu, 09 Jul 2020 11:07:02 -0400
> > 
> > On 2020-07-08, Mark Kettenis wrote:
> > > > From: "Ted Unangst" 
> > > > Date: Wed, 08 Jul 2020 05:20:23 -0400
> > > > 
> > > > On 2020-07-08, Ted Unangst wrote:
> > > > > I think this makes sem_init(pshared) work.
> > > > 
> > > > Whoops, need more context to include the header file changes.
> > > 
> > > It is a bit of a pity that we have to expose the internals here, but I
> > > don't see an easy way to avoid that, especially since hppa requires
> > > 16-byte alignment.  At least  doesn't do any
> > > namespace pollution as I believe a single underscore is enough here as
> > > everything is in file scope?
> > > 
> > > This will require a libpthread major bump, and those are really
> > > painful!  So I'm not sure we should do this just for pshared
> > > semaphores which hardly anybody uses.
> > > 
> > > I wonder if we do this, should we include some additional padding in
> > > the struct for future expansion?
> > 
> > We can also expose only a padded struct sem { int _pad[4]; } or so.
> > That's a bit more cumbersome, and we need to be careful. But certainly
> > possible.
> 
> Tricky since _atomic_lock is actually an array on hppa and need
> 16-byte alignment.  And the lock is actually used on hppa.

Yes that's a good point:

If you pad for the future, use the maximum-sized types, thereby trying to
anticipate the future.

It might not be enough for the bizzare hppa case, but it might help other
architectures...



Re: disable libc sys wrappers?

2020-07-09 Thread Theo de Raadt
Ted Unangst  wrote:

> On 2020-07-08, Theo de Raadt wrote:
> > I think we need something like this.
> > 
> > Documenting it will be a challenge.
> > 
> > I really don't like the name as is too generic, when the control is only
> > for a narrow set of "current time" system calls.
> 
> I thought I'd start with something, but lots of questions.
> 
> Should it be per wrapper?

Perhaps not named per syscall, but named per grouping.  This causes
direct syscalls for time retrieval.  Can you find a name for that?

> I know in the past we've had some similar
> conversations about eliminating syscalls (open/openat)

I don't think we'll be doing that, because it doesn't really make the
world any better and, as is evident with time handling mangles ktrace
too much.

> Initial thought is it's easier to make one button, and then document
> it in ktrace perhaps?

Probably in kdump also.

Hang on.  Do people ever want to force time calls, outside of ktrace?
I doubt it.  Should ktrace maybe have a flag, similar to -B with LD_BIND_NOW,
which sets the new environment variable?  Maybe -T?  The problem smells very
similar to the root cause for LD_BIND_NOW setting..



Re: disable libc sys wrappers?

2020-07-08 Thread Theo de Raadt
Ted's new experimental mailer is incompatible with the real world.



Re: disable libc sys wrappers?

2020-07-08 Thread Theo de Raadt
Mark Kettenis  wrote:

> > From: "Theo de Raadt" 
> > Date: Wed, 08 Jul 2020 09:42:41 -0600
> > 
> > I think we need something like this.
> > 
> > Documenting it will be a challenge.
> > 
> > I really don't like the name as is too generic, when the control is only
> > for a narrow set of "current time" system calls.
> 
> I'm not sure we should be using getenv() in this early initialization
> function though.

Ah, you worry about the static "#ifndef PIC / early_static_init" versus
"PIC ld.so" environ setup, and this very early getenv() call might not be
looking at the environ global.

Ted, did you check static and dynamic binaries?



Re: disable libc sys wrappers?

2020-07-08 Thread Theo de Raadt
I think we need something like this.

Documenting it will be a challenge.

I really don't like the name as is too generic, when the control is only
for a narrow set of "current time" system calls.

Ted Unangst  wrote:

> Not sure how useful this will be, but I think it could be helpful to still
> see section (2) functions in ktrace, even if there's magic to avoid that.
> 
> As proof of concept, if env LIBC_NOSYSWRAPPERS is set, the libc timecounters
> are turned off. Now I see lots of gettimeofday syscalls in ktrace again.
> 
> Is this better than switching to ltrace? Combined ktrace and ltrace output
> is fairly messy, but it seems to work. Setting it up to trace just a few
> functions and all the system calls is a bit more involved.
> 
> 
> Index: init.c
> ===
> RCS file: /home/cvs/src/lib/libc/dlfcn/init.c,v
> retrieving revision 1.8
> diff -u -p -r1.8 init.c
> --- init.c6 Jul 2020 13:33:05 -   1.8
> +++ init.c8 Jul 2020 08:13:07 -
> @@ -114,6 +114,8 @@ _libc_preinit(int argc, char **argv, cha
>   _timekeep->tk_version != TK_VERSION)
>   _timekeep = NULL;
>   }
> + if (issetugid() == 0 && getenv("LIBC_NOSYSWRAPPERS"))
> + _timekeep = NULL;
>   break;
>   }
>   }
> 



Re: userland clock_gettime proof of concept

2020-07-08 Thread Theo de Raadt
Christian Weisgerber  wrote:

> On 2020-06-26, George Koehler  wrote:
> 
> > Here's macppc again.  My macppc isn't using your newest diff but does
> > now need to define TC_TB in .
> 
> Crucial question: How confident are we that TB is in sync on
> multiprocessor machines?

Ugly test program (not showing it) seems to show syncronized clocks on
powerpc, or at least closely sycronized.  It might be one register.



Re: userland clock_gettime proof of concept

2020-07-08 Thread Theo de Raadt
Christian Weisgerber  wrote:

> On 2020-06-26, George Koehler  wrote:
> 
> > Here's macppc again.  My macppc isn't using your newest diff but does
> > now need to define TC_TB in .
> 
> Crucial question: How confident are we that TB is in sync on
> multiprocessor machines?

A few small test programs exposed the problem on alpha for me.

This one identified the problem in a glaring fashion.  The upper
word of the register is per-cpu, and it toggles back and forth
upon each nanosleep call, as we return to a different cpu.  (Quite
obviously the scheduler has zero afinity)

A slightly different test program was able to identify occasions
when returning to the other cpu would return a clock value which was
earlier.  A variation of test_time_skew.c, I don't have it handy.

Maybe this can be quickly convered for macppc, to test?

I don't understand yet how to fix alpha.  It looks like we need a
mechanism to know what cpu# we are on, and a table for the variation.
(The upper word of the alpha clock register assists with profiling
it seems, so I don't think we can put a delta there)

#include 
#include 
#include 

main()
{
struct timespec s = {0, 0};
long times[40];
volatile unsigned long t;
int i;

for (i = 0; i < sizeof times / sizeof times[0]; i++) {
__asm volatile("rpcc %0" : "=r" (t));
times[i] = t;
nanosleep(, NULL);
}
for (i = 0; i < sizeof times / sizeof times[0]; i++) {
t = times[i];
printf("%lx %x\n", (t >> 32) & 0x, t & 0x);
}
}




Re: user tc for alpha

2020-07-08 Thread Theo de Raadt
Scott Cheloha  wrote:

> > -   __asm volatile("rd %%sys_tick, %0" : "=r" (tick) :);
> > +   __asm volatile("rd %%sys_tick, %0" : "=r" (tick));
> >  
> > return (tick & ~0u);
> >  }
> 
> The only thing that gives me pause is the inline assembly.
> 
> I know it isn't a lot of assembly, but would it be a layer violation to
> put, e.g.
> 
> uint64_t
> rd_systick(void)
> {
>   uint64_t val;
> 
>   __asm volatile("rd %%sys_tick, %0" : "=r" (val));
> 
>   return val;
> }
> 
> into something like sys/arch/sparc64/include/cpufunc.h and use it
> in both the kernel and libc?

I don't see anything which precludes libc doing __asm calls, itself,
and I don't think we need to use cpp or inline declarations to cheat.
It is not impossible for userland and kernel use of such "macros" to
change, and would it be OK for libc to "abi change" (note little abi)
because an include file changed?  I don't think that's nice.

> In general isn't it nicer to write it once?

In general yes.  But in the specific, I don't think so.

> We could do the same thing on other platforms too for select
> instructions that are useful in both libc and the kernel.

Until the general rule breaks down, in a variety of ways.  At which
point, why replace a simple thing with a spread out abstraction?

It is already impossible for the abstraction to MI and invisible, it
must be MD (because some architectures have more than 1 counter to look
at)



Re: userland clock_gettime proof of concept

2020-07-07 Thread Theo de Raadt
> The /sys/arch/powerpc/include/timetc.h in your diff never gets used,
> because there is no #include .  On macppc,

I am fixing this issue for all the architectures, just being careful
by doing builds first.



Re: Use VGA text mode palette RGB values in rasops(9)

2020-07-07 Thread Theo de Raadt
Mark Kettenis  wrote:

> Unless there is an overwhelming majority of users/developers who thing
> the colors should indeed be changed.  If there is a significant
> minority that thinks the current colormap severely impacts the
> usability of the framebuffer console, then maybe an interface to
> change the colormap from userland is needed.

Such an interface already exists, it is known as X.

I'm not being ironic.

The console code is sufficient for minor use, or controlled environment
use, and after that X has the fancy features rather than replicating
additional support code in the kernel.  It satisfies needs, not fan clubs.




Re: Use VGA text mode palette RGB values in rasops(9)

2020-07-07 Thread Theo de Raadt
 wrote:

> Related, I looked through the xterm code to see how hard it would
> be to use some of it directly in the wscons system for accurate
> emulation, and it looked pretty challenging to me.  Fixing problems
> piecemeal is probably easier than trying to reuse code.

It was obvious 25-30 years ago to choose vt220 as the what-to-emulate.

In those days, there were still a lot of proprietary unix systems, and
their termcaps were not being updated, so the "pccons" hackjob was
highly inconvient when ssh'ing between machines.  So vt220 was an easier
choice.  Highly compatible with the various termcap generations on those
proprietary operating systems; most of their termcaps were quite
restricted in what they said a vt220 was, so it worked well.

Most of the architectures therefore got vt220 emulation, but since
Torek's sparc code had early "sun" terminal emulation, the code also
grew the option of servicing that emulation instead.  It was convenient.
Likewise, the "sun" termcap entry was high-quality on proprietary
operating systems.

Nowadays, if xterm emulation was the target (instead), it would be more
suitable than either choice.  Highly desireable in my mind.  Imagine if
/etc/ttys was "xterm" on all systems.

I believe it only needs to be a fairly minimal emulation, with tty size
issues are handled TIOCGWINSZ.

After all, the vt220 emulation we have is already a hackjob.  It has
misbehaving features, the 24 vs 25 line issue, enhancements which
are incompatible with a real vt220, gaps in emulating obscure features
noone wants, and colour support which isn't really standard.

>From a risk perspective, unless something is 100% compatible, and
accepts nothing else as input, it isn't compatible and there are risks
introduced because it behaves different from the real thing.  We never
seem to get close to perfect emulation.  Just perfect enough to match
termcap...



Re: Use VGA text mode palette RGB values in rasops(9)

2020-07-07 Thread Theo de Raadt
Frederic Cambus  wrote:

> One can test in frame buffer consoles by doing:
> 
> export TERM=wsvt25
> 
> And run either vim or colorls -G.
> 

Furthermore the situation is just beyond ridiculous.

On the one hand your diff is talking about minimizing the differences
between terminals, but your example immediately heads in the opposite
direction by SELECTING for divergent configuration -- which I suspect
noone except you uses.

This termcap mess should never have happened.  Up until about 10 years
ago, pointless and accidental divergence in console emulators ended up
being reflected in termcap entries *which to a large degree noone used,
except for the people making the termcap entries*, and the situation is
so retarded, because is ENTRENCHES differences rather than recognizing
the differences shouldn't have existed.

The console emulators SHOULD HAVE become more mundane and less
featureful, they should have become closer to a clean blend of vt220 +
xterm.  But no!  People focused on extremely narrow details and hid them
behind TERM feature flags, and avoided focusing on the big picture of
improving ease of utilization and compatibility.

It is very sad.



Re: Use VGA text mode palette RGB values in rasops(9)

2020-07-07 Thread Theo de Raadt
> One can test in frame buffer consoles by doing:
> 
> export TERM=wsvt25
> 
> And run either vim or colorls -G.

Why not set it to wyse30?

Come on.  We chose a default.  If you change it to some arbitrary value,
then you get to reap the upsides and downsides.  Our emulation is *NOT*
'NetBSD "wscons" emulator in vt220 mode.'.







Re: Use VGA text mode palette RGB values in rasops(9)

2020-07-07 Thread Theo de Raadt
Jonathan Gray  wrote:

> On Tue, Jul 07, 2020 at 03:16:33PM +0200, Frederic Cambus wrote:
> > Hi tech@,
> > 
> > The recent spike of interest around framebuffer consoles has prompted
> > me to revisit a proposal I sent back in early 2017 [1].
> > 
> > Aesthetics considerations aside, kettenis@ raised the concern that colors
> > from the original rasops palette carefully matched what OpenFirmware
> > uses for the console on sparc64.
> > 
> > Therefore, I propose to default on using the proper VGA text mode palette
> > RGB values, and to keep the original rasops color palette on sparc64.
> > 
> > The differences between the two palettes can be seen here [2].
> > 
> > Comments? OK?
> 
> Why is it important to match VGA colours?
> We don't try to match video modes or fonts.

I also don't understand the fuss, because during bootup we are effectively
flipping between a series of views (without retaining pre-existing text,
just simply clearing the screen and starting all over again).  Since that
is the situation, why does any of the look matter, as long as each new load
is similar (or better), isn't that good enough?  I think this might matter
if the pre-existing text was *retained*, rather than cleared away..



  1   2   3   4   5   6   7   8   9   10   >