Re: Removing syscall(2) from libc and kernel
Here's the newest version of all 3 diffs:
- kernel diff, can be tested alone
- userland diff, can be tested alone
- syscalls.master cleanup, would happen afterwards, and conflicts a bit.
the ports team has repaired a majority of the syscall fallout in non-go
programs. The effort for go is still ongoing, I do not think it will take
very long. Fingers crossed.
go is the worst because the unix-hater club at the root of their
development team (this is my interpretation of what has happened) showed their
hate to ioctl() -- it is obviously such an evil type-unsafe call that people
need to be shown the even more evil type-unsafe syscall(). the application
community became aware of syscall() and embraced it for this purpose, then
when glibc refused to add new syscall stubs, a related community picked up
syscall() use there also, for all sorts of crazy seperate reasions. On linux,
syscall() is way more of a shitshow [go check their manual page for a subset
of the details that must be kept in mind] than on *BSD operating systems.
*BSD trees handled the 64-bit transition in a cleaner way: first side-stepping
the stdarg problem using a seperate __syscall() function and using pads, later
solving it by changing the ABI so the pads were not needed and the special
casesn
are decomposed in the kernel. This was also easier because we were able to
stop userland from calling __syscall() initially and syscall() as a later step.
Index: sys/arch/alpha/alpha/trap.c
===
RCS file: /cvs/src/sys/arch/alpha/alpha/trap.c,v
diff -u -p -u -r1.108 trap.c
--- sys/arch/alpha/alpha/trap.c 8 Mar 2023 04:43:07 - 1.108
+++ sys/arch/alpha/alpha/trap.c 28 Oct 2023 15:11:05 -
@@ -497,17 +497,15 @@ dopanic:
* a3, and v0 from the frame before returning to the user process.
*/
void
-syscall(code, framep)
- u_int64_t code;
- struct trapframe *framep;
+syscall(u_int64_t code, struct trapframe *framep)
{
- const struct sysent *callp;
+ const struct sysent *callp = sysent;
struct proc *p;
- int error, indirect = -1;
+ int error = ENOSYS;
u_int64_t opc;
u_long rval[2];
u_long args[10];/* XXX */
- u_int hidden, nargs;
+ u_int nargs;
atomic_add_int(, 1);
p = curproc;
@@ -515,24 +513,11 @@ syscall(code, framep)
framep->tf_regs[FRAME_SP] = alpha_pal_rdusp();
opc = framep->tf_regs[FRAME_PC] - 4;
- switch(code) {
- case SYS_syscall:
- indirect = code;
- code = framep->tf_regs[FRAME_A0];
- hidden = 1;
- break;
- default:
- hidden = 0;
- }
-
- error = 0;
- callp = sysent;
- if (code >= SYS_MAXSYSCALL)
- callp += SYS_syscall;
- else
- callp += code;
+ if (code <= 0 || code >= SYS_MAXSYSCALL)
+ goto bad;
+ callp += code;
- nargs = callp->sy_narg + hidden;
+ nargs = callp->sy_narg;
switch (nargs) {
default:
if (nargs > 10) /* XXX */
@@ -559,7 +544,7 @@ syscall(code, framep)
rval[0] = 0;
rval[1] = 0;
- error = mi_syscall(p, code, indirect, callp, args + hidden, rval);
+ error = mi_syscall(p, code, callp, args, rval);
switch (error) {
case 0:
Index: sys/arch/amd64/amd64/locore.S
===
RCS file: /cvs/src/sys/arch/amd64/amd64/locore.S,v
diff -u -p -u -r1.141 locore.S
--- sys/arch/amd64/amd64/locore.S 24 Oct 2023 13:20:09 - 1.141
+++ sys/arch/amd64/amd64/locore.S 28 Oct 2023 15:11:05 -
@@ -508,6 +508,7 @@ ENTRY(savectx)
lfence
END(savectx)
+// XXX this should not behave like a nop
IDTVEC(syscall32)
sysret /* go away please */
END(Xsyscall32)
Index: sys/arch/amd64/amd64/trap.c
===
RCS file: /cvs/src/sys/arch/amd64/amd64/trap.c,v
diff -u -p -u -r1.101 trap.c
--- sys/arch/amd64/amd64/trap.c 5 Jul 2023 12:58:55 - 1.101
+++ sys/arch/amd64/amd64/trap.c 28 Oct 2023 15:11:05 -
@@ -553,7 +553,7 @@ syscall(struct trapframe *frame)
caddr_t params;
const struct sysent *callp;
struct proc *p;
- int error, indirect = -1;
+ int error = ENOSYS;
size_t argsize, argoff;
register_t code, args[9], rval[2], *argp;
@@ -570,26 +570,9 @@ syscall(struct trapframe *frame)
argp = [0];
argoff = 0;
- switch (code) {
- case SYS_syscall:
- /*
-* Code is first argument, followed by actual args.
-*/
- indirect = code;
- code = frame->tf_rdi;
- argp = [1];
- argoff = 1;
- break;
- default:
-
Re: Removing syscall(2) from libc and kernel
Theo de Raadt wrote:
> Piece by piece, I've been trying to remove the easiest of the
> terminal-actions that exploit code uses (ie. getting to execve, or performing
> other system calls, etc).
> Snapshots for some architectures now contain kernel diffs which reject
> syscall(2). The symbol still remains libc.
>
> I'm including a piece of this diff.
Replacement diff that works on more architectures. (The story here
is that a kernel-only diff without the userland diff requires a different
kind of range check, because syscalls.master cannot be changed to make
the 0th entry enosys, because the generated .h files will break libc
build. So I refactored rapidly, and forgot 'error = ENOSYS' on a few
architectures..)
Index: sys/arch/alpha/alpha/trap.c
===
RCS file: /cvs/src/sys/arch/alpha/alpha/trap.c,v
diff -u -p -u -r1.108 trap.c
--- sys/arch/alpha/alpha/trap.c 8 Mar 2023 04:43:07 - 1.108
+++ sys/arch/alpha/alpha/trap.c 27 Oct 2023 16:16:57 -
@@ -497,17 +497,15 @@ dopanic:
* a3, and v0 from the frame before returning to the user process.
*/
void
-syscall(code, framep)
- u_int64_t code;
- struct trapframe *framep;
+syscall(u_int64_t code, struct trapframe *framep)
{
- const struct sysent *callp;
+ const struct sysent *callp = sysent;
struct proc *p;
- int error, indirect = -1;
+ int error = ENOSYS;
u_int64_t opc;
u_long rval[2];
u_long args[10];/* XXX */
- u_int hidden, nargs;
+ u_int nargs;
atomic_add_int(, 1);
p = curproc;
@@ -515,24 +513,11 @@ syscall(code, framep)
framep->tf_regs[FRAME_SP] = alpha_pal_rdusp();
opc = framep->tf_regs[FRAME_PC] - 4;
- switch(code) {
- case SYS_syscall:
- indirect = code;
- code = framep->tf_regs[FRAME_A0];
- hidden = 1;
- break;
- default:
- hidden = 0;
- }
-
- error = 0;
- callp = sysent;
- if (code >= SYS_MAXSYSCALL)
- callp += SYS_syscall;
- else
- callp += code;
+ if (code <= 0 || code >= SYS_MAXSYSCALL)
+ goto bad;
+ callp += code;
- nargs = callp->sy_narg + hidden;
+ nargs = callp->sy_narg;
switch (nargs) {
default:
if (nargs > 10) /* XXX */
@@ -559,7 +544,7 @@ syscall(code, framep)
rval[0] = 0;
rval[1] = 0;
- error = mi_syscall(p, code, indirect, callp, args + hidden, rval);
+ error = mi_syscall(p, code, callp, args, rval);
switch (error) {
case 0:
Index: sys/arch/amd64/amd64/locore.S
===
RCS file: /cvs/src/sys/arch/amd64/amd64/locore.S,v
diff -u -p -u -r1.141 locore.S
--- sys/arch/amd64/amd64/locore.S 24 Oct 2023 13:20:09 - 1.141
+++ sys/arch/amd64/amd64/locore.S 27 Oct 2023 03:26:49 -
@@ -508,6 +508,7 @@ ENTRY(savectx)
lfence
END(savectx)
+// XXX this should not behave like a nop
IDTVEC(syscall32)
sysret /* go away please */
END(Xsyscall32)
Index: sys/arch/amd64/amd64/trap.c
===
RCS file: /cvs/src/sys/arch/amd64/amd64/trap.c,v
diff -u -p -u -r1.101 trap.c
--- sys/arch/amd64/amd64/trap.c 5 Jul 2023 12:58:55 - 1.101
+++ sys/arch/amd64/amd64/trap.c 27 Oct 2023 03:26:49 -
@@ -553,7 +553,7 @@ syscall(struct trapframe *frame)
caddr_t params;
const struct sysent *callp;
struct proc *p;
- int error, indirect = -1;
+ int error = ENOSYS;
size_t argsize, argoff;
register_t code, args[9], rval[2], *argp;
@@ -570,26 +570,9 @@ syscall(struct trapframe *frame)
argp = [0];
argoff = 0;
- switch (code) {
- case SYS_syscall:
- /*
-* Code is first argument, followed by actual args.
-*/
- indirect = code;
- code = frame->tf_rdi;
- argp = [1];
- argoff = 1;
- break;
- default:
- break;
- }
-
- callp = sysent;
- if (code < 0 || code >= SYS_MAXSYSCALL)
- callp += SYS_syscall;
- else
- callp += code;
-
+ if (code <= 0 || code >= SYS_MAXSYSCALL)
+ goto bad;
+ callp = sysent + code;
argsize = (callp->sy_argsize >> 3) + argoff;
if (argsize) {
switch (MIN(argsize, 6)) {
@@ -620,7 +603,7 @@ syscall(struct trapframe *frame)
rval[0] = 0;
rval[1] = 0;
- error = mi_syscall(p, code, indirect, callp, argp, rval);
+ error = mi_syscall(p, code, callp, argp, rval);
Re: Removing syscall(2) from libc and kernel
Theo de Raadt wrote:
> Piece by piece, I've been trying to remove the easiest of the
> terminal-actions that exploit code uses (ie. getting to execve, or performing
> other system calls, etc).
> So in this next step, I'm going to take away the ability to perform syscall #0
> (SYS_syscall), with the first argument being the real system call.
>
> This library interface, and all the pieces below it, will be going away:
>
> https://man.openbsd.org/syscall.2
>
> There's going to be some fallout which takes time to fix, especially in the
> "go" ecosystem.
>
> Snapshots for some architectures now contain kernel diffs which reject
> syscall(2). The symbol still remains libc.
Here is the larger diff which removes syscall(2) from everywhere:
- kernel
- libc prototypes and stub
- manuals
- a few programs that can see this
Index: include/unistd.h
===
RCS file: /cvs/src/include/unistd.h,v
diff -u -p -u -r1.107 unistd.h
--- include/unistd.h7 Jan 2023 05:24:58 - 1.107
+++ include/unistd.h23 Oct 2023 21:10:08 -
@@ -522,7 +522,6 @@ int setthrname(pid_t, const char *);
voidsetusershell(void);
int strtofflags(char **, u_int32_t *, u_int32_t *);
int swapctl(int cmd, const void *arg, int misc);
-int syscall(int, ...);
int getentropy(void *, size_t);
int pledge(const char *, const char *);
int unveil(const char *, const char *);
Index: lib/libc/Symbols.list
===
RCS file: /cvs/src/lib/libc/Symbols.list,v
diff -u -p -u -r1.83 Symbols.list
--- lib/libc/Symbols.list 20 Aug 2023 15:17:53 - 1.83
+++ lib/libc/Symbols.list 23 Oct 2023 21:10:08 -
@@ -434,7 +434,6 @@ symlink
symlinkat
sync
sysarch
-syscall
timer_create
timer_delete
timer_getoverrun
Index: lib/libc/hidden/unistd.h
===
RCS file: /cvs/src/lib/libc/hidden/unistd.h,v
diff -u -p -u -r1.12 unistd.h
--- lib/libc/hidden/unistd.h18 May 2023 16:11:09 - 1.12
+++ lib/libc/hidden/unistd.h23 Oct 2023 21:10:08 -
@@ -157,7 +157,6 @@ PROTO_NORMAL(swapctl);
PROTO_NORMAL(symlink);
PROTO_NORMAL(symlinkat);
PROTO_NORMAL(sync);
-PROTO_NORMAL(syscall);
PROTO_NORMAL(sysconf);
PROTO_DEPRECATED(tcgetpgrp);
PROTO_DEPRECATED(tcsetpgrp);
Index: lib/libc/sys/Makefile.inc
===
RCS file: /cvs/src/lib/libc/sys/Makefile.inc,v
diff -u -p -u -r1.174 Makefile.inc
--- lib/libc/sys/Makefile.inc 20 Aug 2023 15:17:53 - 1.174
+++ lib/libc/sys/Makefile.inc 23 Oct 2023 21:10:08 -
@@ -8,7 +8,7 @@
# modules with non-default implementations on at least one architecture:
SRCS+= Ovfork.S brk.S ${CERROR} \
sbrk.S sigpending.S sigprocmask.S \
- sigsuspend.S syscall.S tfork_thread.S
+ sigsuspend.S tfork_thread.S
# glue to offer userland wrappers for some syscalls
SRCS+= posix_madvise.c pthread_sigmask.c \
@@ -216,7 +216,7 @@ MAN+= __get_tcb.2 __thrsigdivert.2 __thr
shmctl.2 shmget.2 shutdown.2 sigaction.2 sigaltstack.2 sigpending.2 \
sigprocmask.2 sigreturn.2 sigsuspend.2 socket.2 \
socketpair.2 stat.2 statfs.2 swapctl.2 symlink.2 \
- sync.2 sysarch.2 syscall.2 sysctl.2 thrkill.2 truncate.2 \
+ sync.2 sysarch.2 sysctl.2 thrkill.2 truncate.2 \
umask.2 unlink.2 unveil.2 utimes.2 utrace.2 vfork.2 \
wait.2 waitid.2 write.2 \
ypconnect.2
Index: lib/libc/sys/syscall.2
===
RCS file: /cvs/src/lib/libc/sys/syscall.2,v
diff -u -p -u -r1.16 syscall.2
--- lib/libc/sys/syscall.2 22 Feb 2023 07:04:50 - 1.16
+++ lib/libc/sys/syscall.2 23 Oct 2023 21:10:08 -
@@ -1,68 +0,0 @@
-.\"$OpenBSD: syscall.2,v 1.16 2023/02/22 07:04:50 jmc Exp $
-.\"$NetBSD: syscall.2,v 1.4 1995/02/27 12:38:53 cgd Exp $
-.\"
-.\" Copyright (c) 1980, 1991, 1993
-.\"The Regents of the University of California. All rights reserved.
-.\"
-.\" Redistribution and use in source and binary forms, with or without
-.\" modification, are permitted provided that the following conditions
-.\" are met:
-.\" 1. Redistributions of source code must retain the above copyright
-.\"notice, this list of conditions and the following disclaimer.
-.\" 2. Redistributions in binary form must reproduce the above copyright
-.\"notice, this list of conditions and the following disclaimer in the
-.\"documentation and/or other materials provided with the distribution.
-.\" 3. Neither the name of the University nor the names of its contributors
-.\"may be used to endorse or promote products derived from this software
-.\"without specific prior written per
Removing syscall(2) from libc and kernel
Piece by piece, I've been trying to remove the easiest of the
terminal-actions that exploit code uses (ie. getting to execve, or performing
other system calls, etc).
I recognize we can never completely remove all mechanisms they
use. However, I hope I am forcing attack coders into using increasingly
more complicated methods. Same time, it means fewer methods are
available. Other methods make exploitation more fragile. This is
pushing success rates into "low-percent statistical" success. If we
teach more software stacks to "fail hard, don't try to recover", that is
an improvement in security.
I already made it difficult to call execve() directly in a few ways.
The kernel must be entered via the exact syscall instruction, inside the
libc syscall stub. Immediately before that syscall instruction, the
SYS_execve instruction is loaded into a register. On some
architectures, the PLT-reachable stub performs a retguard check, which
can be triggered by a few methods. Stack pivots are also mostly
prevented because of other checks. It is not possible to enter via
the SYS_syscall (syscall register = 0) case either.
Attack code can try to do perform other system calls, to create
filesystem damage or network communication. They could still load other
syscall numbers and jump to a found syscall instruction, if they are
able to cheat the retguard epilogue (It is a bit unfortunate that libc
syscall stubs tend to use the same save register, but at least the
compare offset is chosen random at compile time). Or, they could know
where all the system calls are from a pre-read libc, which requires them
to be on the machine before performing an online or offline attack (libc
is random relinked, but still readable in the filesystem). It's
difficult to discover code-locations online only, because most
architectures also have xonly code now. Some methods can use PLT
entries (which also vary based upon random relink), but I've not seem
much methodology using PLT entry + offset.
Anyways, everyone of these things I mention, and the ones I don't mention,
tend to be more difficult than the previous methods. I'm trying to remove
simple methods, and force attackers into more and more complex methods.
I promise that I will circle back and damage the more complex methods in
the future.
So in this next step, I'm going to take away the ability to perform syscall #0
(SYS_syscall), with the first argument being the real system call.
This library interface, and all the pieces below it, will be going away:
https://man.openbsd.org/syscall.2
There's going to be some fallout which takes time to fix, especially in the
"go" ecosystem.
Snapshots for some architectures now contain kernel diffs which reject
syscall(2). The symbol still remains libc.
I'm including a piece of this diff.
Index: sys/arch/alpha/alpha/trap.c
===
RCS file: /cvs/src/sys/arch/alpha/alpha/trap.c,v
diff -u -p -u -r1.108 trap.c
--- sys/arch/alpha/alpha/trap.c 8 Mar 2023 04:43:07 - 1.108
+++ sys/arch/alpha/alpha/trap.c 27 Oct 2023 03:26:49 -
@@ -497,17 +497,15 @@ dopanic:
* a3, and v0 from the frame before returning to the user process.
*/
void
-syscall(code, framep)
- u_int64_t code;
- struct trapframe *framep;
+syscall(u_int64_t code, struct trapframe *framep)
{
- const struct sysent *callp;
+ const struct sysent *callp = sysent;
struct proc *p;
- int error, indirect = -1;
+ int error;
u_int64_t opc;
u_long rval[2];
u_long args[10];/* XXX */
- u_int hidden, nargs;
+ u_int nargs;
atomic_add_int(, 1);
p = curproc;
@@ -515,24 +513,11 @@ syscall(code, framep)
framep->tf_regs[FRAME_SP] = alpha_pal_rdusp();
opc = framep->tf_regs[FRAME_PC] - 4;
- switch(code) {
- case SYS_syscall:
- indirect = code;
- code = framep->tf_regs[FRAME_A0];
- hidden = 1;
- break;
- default:
- hidden = 0;
- }
-
- error = 0;
- callp = sysent;
- if (code >= SYS_MAXSYSCALL)
- callp += SYS_syscall;
- else
- callp += code;
+ if (code <= 0 || code >= SYS_MAXSYSCALL)
+ goto bad;
+ callp += code;
- nargs = callp->sy_narg + hidden;
+ nargs = callp->sy_narg;
switch (nargs) {
default:
if (nargs > 10) /* XXX */
@@ -559,7 +544,7 @@ syscall(code, framep)
rval[0] = 0;
rval[1] = 0;
- error = mi_syscall(p, code, indirect, callp, args + hidden, rval);
+ error = mi_syscall(p, code, callp, args, rval);
switch (error) {
case 0:
Index: sys/arch/amd64/amd64/locore.S
===
RCS file: /cvs/src/sys/arch/amd64/amd64/locore.S,v
diff -u -p -u
disruptive amd64 snapshot coming
There is a pretty disruptive amd64 snapshot coming, so anyone who is using snapshots for critical stuff should take a pause. (This warning about a development step is unusual, I won't make it common practice).
Re: boot loaders: softraid volumes must be on RAID partitions
Crystal Kolipe wrote: `> Hi Theo, it's a long time since we last conversed. > > On Mon, Oct 23, 2023 at 03:44:17PM -0600, Theo de Raadt wrote: > > What user without OpenBSD experience is booting from 'd'? > > > > Which also poses the question -- what user with OpenBSD experience > > is booting from 'd'? > > > > Why? > > Some disklabel partitions have traditionally had specific meanings: > > a - root fs > b - swap > c - whole disk > d - on non-OpenBSD systems is 'whole disk' where c is not > i - often used for non FFS partitions on OpenBSD > > Why would you automatically make a RAID as parition 'a'? > > It's not a root fs. > > I don't see any logic behind RAID partition = 'a'. > > What if you want more than one? > > Booting from non-'a' softraids has never been discouraged on amd64. > > It's been noted that on other archs it doesn't work, but there has > never been a general advisory that booting from softraid volumes should > only be done from 'a'. > > > You say "quite possible that people have machines deployed that boot > > from other RAID partitions" > > After 10+ years of it working, do you really think that nobody has > ever done it? The way you write, it sounds personal.
Re: boot loaders: softraid volumes must be on RAID partitions
Crystal Kolipe wrote:
> On Mon, Oct 23, 2023 at 11:04:07AM +, Klemens Nanni wrote:
> > 10/16/23 04:02, Klemens Nanni ??:
> > > The current check implies one could use, e.g. SWAP or MSDOS partitions
> > > as softraid(4) chunks, but sys/dev/softraid.c always expects FS_RAID,
> > > thus using chunks with different partition types is not possible:
> > >
> > > # vmctl create -s100M disk.img
> > > # vnd=`vnconfig disk.img`
> > > # echo 'swap *' | disklabel -wAT- vnd0
> > >
> > > # disklabel $vnd | grep swap
> > > a: 2048000swap
> > > # bioctl -c c -l ${vnd}a softraid0
> > > softraid0: invalid metadata format
> > >
> > > Correct the check.
> > > I don't expect this to break anything.
> > > amd64 biosboot boots off standard RAID 'a' as before.
> > >
> > > Feedback? Objection? OK?
> >
> > Ping.
>
> This breaks booting off of a RAID that is not on partition 'a', on amd64.
>
> Was this intentional?
>
> For example, if you have a RAID on 'd', with no 'a' partition at all, then
> with your patch the machine becomes unbootable.
>
> The second stage bootloader doesn't automatically find the softraid volume.
> Manually booting the kernel from it results in a kernel panic when the
> kernel can't find the root filesystem.
>
> Although booting from a RAID on a non-'a' partition is not supported on all
> archs, it has worked fine on amd64 for a long time, so it's quite possible
> that people have machines deployed that boot from other RAID partitions.
>
> This change would unexpectedly break them, and it would potentially be quite
> painful for any users who upgrade to 7.5 and find out afterwards that their
> machine doesn't boot, because the work-around would likely be to boot the
> ramdisk kernel, and unpack mdec/boot from the base package of the previous
> release then re-run installboot specifying the old mdec/boot.
>
> That wouldn't be at all obvious to users without a lot of OpenBSD experience.
What user without OpenBSD experience is booting from 'd'?
Which also poses the question -- what user with OpenBSD experience
is booting from 'd'?
Why?
You say "quite possible that people have machines deployed that boot
from other RAID partitions"
Who? Just you?
Re: relayd does not delete control socket on shutdown
Otto Moerbeek wrote: > On Sat, Oct 21, 2023 at 10:40:45PM +0300, Kapetanakis Giannis wrote: > > > On 21/10/2023 20:39, Florian Obser wrote: > > > Which was 8 years ago. I don't understand why you see a change in 7.4. > > > > > > Anyway, we decided to not clean up control sockets in any of our > > > privsep daemons because leaving them behind does not cause any issues. > > > > I just noticed it today when I tried to use the socket in a script and > > noticed that it stayed there even after shutdown and though it was after 7.4 > > but I was wrong about that. > > > > Your commit made it that clear. > > > > Agree it's not a big case if it stays there. > > > > Would the unlink succeed if the socket was owned by _relayd? > > > > G > > Unlinking somthing requires write permissions to the directory it is > in. Which means an attacker who gains control, but otherwise can't do a bunch of other things becuase of the privsep design -- could still fill the directory and filesystem. So a few years ago we asked ourselves -- is the tradeoff worth it?
Re: HAMMER2 filesystem for OpenBSD
What will come of this discussion of opinions? Chris Narkiewicz wrote: > Hi, > > Tomohiro Kusumi is currently working on HAMMER2 implementation > for OpenBSD, FreeBSD and NetBSD. > > The repository is here: > https://github.com/kusumi/openbsd_hammer2 > > > He maintains repositories for NetBSD, FreeBSD and OpenBSD, which > suggests that the implementation is portable. He also > provides a patch for OpenBSD 7.3: > > https://github.com/kusumi/openbsd_hammer2/blob/master/patch/openbsd73.patch > > The patch looks very minimal to me, with no deeper changes to the > kernel. > > I haven't found any discussion about HAMMER2 in list archives, so I'd > like to bring it to devs attention, kindly asking for your opinion. > Does it look like it's worth bringing in? Does it require more work? > > I'd appreciate any opinions from more knowledgable crowd. > > Cheers, > Chris >
Re: timeout(1): align execvp(3) failure statuses with GNU timeout
This manual page provides details for the EXIT STATUS.
This should be added.
Scott Cheloha wrote:
> Align timeout(1)'s execvp(3) failure statuses with those of GNU
> timeout. 127 for ENOENT, 126 for everything else.
>
> $ cd /tmp
> $ touch script.sh
> $ ls -l script.sh
> -rw-r--r-- 1 ssc wheel 0 Oct 15 13:43 script.sh
> $ gtimeout 1.0 ./script.sh ; echo $?
> gtimeout: failed to run command './script.sh': Permission denied
> 126
> $ timeout 1.0 ./script.sh ; echo $?
> timeout: ./script.sh: Permission denied
> 1
> $ gtimeout 1.0 ./not-a-script.sh ; echo $?
> gtimeout: failed to run command './not-a-script.sh': No such file or directory
> 127
> $ timeout 1.0 ./not-a-script.sh ; echo $?
> timeout: ./not-a-script.sh: No such file or directory
> 1
>
> While here, _exit(2) from the child process instead of exit(3).
>
> ok?
>
> Index: timeout.c
> ===
> RCS file: /cvs/src/usr.bin/timeout/timeout.c,v
> retrieving revision 1.25
> diff -u -p -r1.25 timeout.c
> --- timeout.c 13 Jan 2023 06:53:04 - 1.25
> +++ timeout.c 15 Oct 2023 18:47:04 -
> @@ -260,7 +260,8 @@ main(int argc, char **argv)
> signal(SIGTTOU, SIG_DFL);
>
> execvp(argv[0], argv);
> - err(1, "%s", argv[0]);
> + warn("%s", argv[0]);
> + _exit(errno == ENOENT ? 127 : 126);
> }
>
> /* parent continues here */
>
Re: Remove hardcoded ${HOSTCC} calls in games?
I think this is correct support for cross-compilation, and what you
are trying to do is less important.
Frederic Cambus wrote:
> Hi tech@,
>
> When trying the GCC 11 static analyzer on games, I noticed that some of
> them (adventure, boggle, fortune, hack, monop, phantasia) have hardcoded
> calls to ${HOSTCC}. They would obviously not compile when passed the GCC's
> "-fanalyzer" flag through CFLAGS as it is not recognized by Clang.
>
> Most of those calls were added in 1996 by the following commit:
>
> https://github.com/openbsd/src/commit/da34e3c3d40263be91967714eaa1a2c4390ea117
>
> And the remaining ones in early 1997:
>
> https://github.com/openbsd/src/commit/bdcd13e0503c5cfa23706bc22449229ca71dddaf
> https://github.com/openbsd/src/commit/06f1ba87d9fbb136e239d3f8fa0b8a7c8c825687
>
> Unless I'm mistaken, I don't see any reason why we need to keep them.
>
> The following diff removes them.
>
> Comments? OK?
>
> Index: games/adventure/Makefile
> ===
> RCS file: /cvs/src/games/adventure/Makefile,v
> retrieving revision 1.5
> diff -u -p -r1.5 Makefile
> --- games/adventure/Makefile 23 May 2002 18:42:59 - 1.5
> +++ games/adventure/Makefile 12 Oct 2023 07:39:02 -
> @@ -9,6 +9,6 @@ data.c: glorkz setup
> ./setup ${.CURDIR}/glorkz > data.c
>
> setup: setup.c hdr.h
> - ${HOSTCC} -o setup ${.CURDIR}/setup.c
> + ${CC} -o setup ${.CURDIR}/setup.c
>
> .include
> Index: games/boggle/mkdict/Makefile
> ===
> RCS file: /cvs/src/games/boggle/mkdict/Makefile,v
> retrieving revision 1.4
> diff -u -p -r1.4 Makefile
> --- games/boggle/mkdict/Makefile 7 Jan 2016 16:00:31 - 1.4
> +++ games/boggle/mkdict/Makefile 12 Oct 2023 07:39:02 -
> @@ -5,7 +5,6 @@
> PROG=mkdict
> CFLAGS+=-I${.CURDIR}/../boggle
> NOMAN=noman
> -CC=${HOSTCC}
>
> install:
>
> Index: games/boggle/mkindex/Makefile
> ===
> RCS file: /cvs/src/games/boggle/mkindex/Makefile,v
> retrieving revision 1.4
> diff -u -p -r1.4 Makefile
> --- games/boggle/mkindex/Makefile 7 Jan 2016 16:00:31 - 1.4
> +++ games/boggle/mkindex/Makefile 12 Oct 2023 07:39:02 -
> @@ -5,7 +5,6 @@
> PROG=mkindex
> CFLAGS+=-I${.CURDIR}/../boggle
> NOMAN=noman
> -CC=${HOSTCC}
>
> install:
>
> Index: games/fortune/strfile/Makefile
> ===
> RCS file: /cvs/src/games/fortune/strfile/Makefile,v
> retrieving revision 1.4
> diff -u -p -r1.4 Makefile
> --- games/fortune/strfile/Makefile9 Feb 1997 13:52:40 - 1.4
> +++ games/fortune/strfile/Makefile12 Oct 2023 07:39:02 -
> @@ -4,6 +4,5 @@
>
> PROG=strfile
> MAN= strfile.8
> -CC= ${HOSTCC}
>
> .include
> Index: games/hack/Makefile
> ===
> RCS file: /cvs/src/games/hack/Makefile,v
> retrieving revision 1.17
> diff -u -p -r1.17 Makefile
> --- games/hack/Makefile 5 Apr 2019 09:02:27 - 1.17
> +++ games/hack/Makefile 12 Oct 2023 07:39:02 -
> @@ -24,7 +24,7 @@ hack.onames.h: makedefs def.objects.h
> ${.OBJDIR}/makedefs ${.CURDIR}/def.objects.h > hack.onames.h
>
> makedefs: makedefs.c
> - ${HOSTCC} ${CFLAGS} ${LDFLAGS} ${LDSTATIC} -o ${.TARGET}
> ${.CURDIR}/${.PREFIX}.c ${LDADD}
> + ${CC} ${CFLAGS} ${LDFLAGS} ${LDSTATIC} -o ${.TARGET}
> ${.CURDIR}/${.PREFIX}.c ${LDADD}
>
> beforeinstall:
> ${INSTALL} ${INSTALL_COPY} -o ${BINOWN} -g ${BINGRP} -m 444
> ${.CURDIR}/help \
> Index: games/monop/Makefile
> ===
> RCS file: /cvs/src/games/monop/Makefile,v
> retrieving revision 1.7
> diff -u -p -r1.7 Makefile
> --- games/monop/Makefile 23 May 2002 18:43:00 - 1.7
> +++ games/monop/Makefile 12 Oct 2023 07:39:02 -
> @@ -12,7 +12,7 @@ cards.pck: initdeck
> ${.OBJDIR}/initdeck ${.CURDIR}/cards.inp
>
> initdeck: initdeck.c
> - ${HOSTCC} ${CFLAGS} ${LDFLAGS} ${LDSTATIC} -o ${.TARGET}
> ${.CURDIR}/initdeck.c ${LDADD}
> + ${CC} ${CFLAGS} ${LDFLAGS} ${LDSTATIC} -o ${.TARGET}
> ${.CURDIR}/initdeck.c ${LDADD}
>
> beforeinstall:
> ${INSTALL} ${INSTALL_COPY} -o ${BINOWN} -g ${BINGRP} -m 444 cards.pck \
> Index: games/phantasia/Makefile
> ===
> RCS file: /cvs/src/games/phantasia/Makefile,v
> retrieving revision 1.19
> diff -u -p -r1.19 Makefile
> --- games/phantasia/Makefile 11 Jul 2022 03:11:49 - 1.19
> +++ games/phantasia/Makefile 12 Oct 2023 07:39:02 -
> @@ -11,13 +11,13 @@ CLEANFILES+=map setup setup.o phantglobs
> all: setup phantasia
>
> setup.o: setup.c
> - ${HOSTCC} -c ${CFLAGS} -o ${.TARGET} ${.CURDIR}/setup.c
> + ${CC} -c ${CFLAGS} -o ${.TARGET}
Re: Some bwfm(4) diffs
Peter J. Philipp wrote: > On Tue, Oct 10, 2023 at 06:25:44AM +0200, Peter J. Philipp wrote: > > > > Thanks, I actually have one of these myself. So I'm going to > > > > investigate (and probably drop one of the diffs). > > > > > > I don't see any problems on my machine. Firmware loads and WiFi works > > > just fine on my MacBookPro12,1. > > > > > > > Hmm odd. I'll have to check again. Thanks Mark! > > Hi, > > Just to confirm it works now. I updated to a latest snapshot and the > installer may have done something as it attached late in the install but > first complained about no firmware. That diff is not in snapshots.
Re: ifq_start_task(): purge queue before exit when IFF_RUNNING flag is not set
Alexander Bluhm wrote: > During configuration there is usually some kind of lock, it happens > rarely, and bugs are hard to trigger, especially on amd64 which > guarantees some consistency. That's why it works. > > > Some times ago, I privately pointed this and proposed to modify if_flags > > atomically. May be it's time to rethink this. > > Atomic operations are not the answer to everything. They are still > slow and don't guarantee consistency with the logic around them. > If writes to if_flags happen rarely and are protected by exclusive > netlock anyway, we can leave them as they are. > > More problematic is reading. Many people believe that reading an > integer, that another CPU can modify, is fine. To make the integer > value consistent for the compiler, you need READ_ONCE() or > atomic_load_int(). And then consider whether reordering it with > all the instructions around it, is a problem. Getting that right > with memory barriers is very hard. It should not be done in regular > code, better use primitives that have better semantics. Right the problem is other fields in the structure which are married to each other. A flag indicates that some other field should be looked at. But the reading point, in some code, does not indicate this semantic, and the compiler feels free to pre-read the wrong part from the structure out of order. There are many ways this can play out. It is better to lock the entire sub-block in some way, than just the single field.
Re: any work on port of rtw89? (realtek 8852 AE/BE/CE)
Mikhail Pchelin wrote: > On Sun, Oct 01, 2023 at 11:17:32AM -0600, Theo de Raadt wrote: > > >I'm interested if anyone has already done any research or code regarding > > >these drivers to save double effort. > > > > Over in freebsd land won't you just use the linux drivers, to "save double > > effort"? And how's that going. > > Linux80211 is a wrong approach to anyone who has ever cared about > software stability, but I don't care about those decisions, this layer > is sponsored work with specially appointed person, I can only wish a > good luck to him. For decades I've been hearing that FreeBSD wireless is better.
Re: any work on port of rtw89? (realtek 8852 AE/BE/CE)
>I'm interested if anyone has already done any research or code regarding >these drivers to save double effort. Over in freebsd land won't you just use the linux drivers, to "save double effort"? And how's that going.
Re: sysupgrade: omit default sets answer
It does not seem crucial to commit this just before a release.
Klemens Nanni wrote:
> On Fri, Sep 29, 2023 at 05:28:46PM +0200, Florian Obser wrote:
> > On 2023-09-29 14:41 UTC, Klemens Nanni wrote:
> > > The response file contains only to non-defaults, except for
> > > Set name(s)? (or 'abort' or 'done') [done] done
> > >
> > > which is the hardcoded default since 2009:
> > > ask "Set name(s)? (or 'abort' or 'done')" done
> > >
> > > We pass it since r1.23 in 2019
> > > Let sysupgrade(8) create auto_upgrade.conf file in preparation of
> > > moving the functionality out of install.sub.
> > > OK deraadt
> > >
> > > I see no need for explicit defaults.
> > > sysupgrade(8) and installer work the same with this diff.
> >
> > I have a very vague recollection that there was a reason to have this
> > but I can't put my finger on it. Something like the installer would spin
> > on selecting the sets while we were developing this inside of
> > install.sub. It was probably some other mistake that got fixed and this
> > is a left-over.
>
> I don't recall that, but will certainly check on it, thanks.
>
> >
> > Anyway, soon a lot of people are going to try sysupgrade in all kinds of
> > weird setups, now is a good time to put this in, IFF you have the time
> > to keep an eye on that it doesn't break. You also want deraadt@ on board
> > for this.
> >
> > With all those constraints met, OK florian (I *don't* have time to keep an
> > eye on it.)
>
> No need to push this before release and create potential problems.
> I just sent the diff now so as not to forget about it (again).
>
> >
> >
> > >
> > > Index: sysupgrade.sh
> > > ===jk
> > > RCS file: /cvs/src/usr.sbin/sysupgrade/sysupgrade.sh,v
> > > retrieving revision 1.48
> > > diff -u -p -r1.48 sysupgrade.sh
> > > --- sysupgrade.sh 8 Jun 2022 09:03:11 - 1.48
> > > +++ sysupgrade.sh 29 Sep 2023 13:48:18 -
> > > @@ -185,7 +185,6 @@ fi
> > > cat <<__EOT >/auto_upgrade.conf
> > > Location of sets = disk
> > > Pathname to the sets = ${SETSDIR}/
> > > -Set name(s) = done
> > > Directory does not contain SHA256.sig. Continue without verification =
> > > yes
> > > __EOT
> > >
> > >
> >
> > --
> > In my defence, I have been left unsupervised.
> >
>
Re: Significance of MALLOC_OPTIONS=G
Our kernel also has the concept of guard-pages, meaning it will try to keep a gap of 1 page between mmap() allocations. The way it is coded, it isn't perfect, but it tends to work and catch some issues.
Re: [newvers] sysupgrade(8) -release to -beta narrow of sets version
I stopped reading as soon as your pride showed. Eponymous Pseudonym wrote: > Right, that's exactly it. > > I also found out moments after I posted a white space nuisance, and > the logic details that are not enough in what I proposed as it's not > factoring the snapshot independently of -release, and also -current > and -stable cases are incomplete too, and BEFORE that, there is > missing information from, and changes needed too, on the master > "release" preparation side. > > So can't solve it as simple as I proposed, and was trying to make it a > recurring set of state table changes and various "edge" case fixes by > iterative incremental sequence of "improvements" that would be always > correct regardless of the failure state, even when the index / > checksum has been mixed and incompletely overlapped.. but you're > relieving this for now and that's fine with me. > > It took me exactly 3 min to solve it in multiple variants my end and I > am not being inconvenienced with a tweak per "newvers" change, after > all the extensive routes of precise tracking of these in my custom > auto-upgrade routines for many years maybe even more than a decade > prior to sysupgrade(8) here.. > > My main critical point is the omission to enumerate the sets in the > logic of the sysupgrade(8) script and the gratuitous version detection > based on kernel string and checksum/index rather than normalised > (tracked) by newvers state published on the mirror (or in the > signatures) or carried in the sets versioning information in a > specialised format like BUILDINFO, whatever you like. > > Anyway, it needs a bit of a discussion to which I have not been yet > invited, so will defer that to your schedule and preferred solution, > needless to say. Thanks for your qualified feedback as usual. > > On 9/26/23, Theo de Raadt wrote: > > Stuart Henderson wrote: > > > >> > This results in failure to upgrade to a valid snapshot on the mirrors, > >> > and users having to wait a new snapshot fanout without mixed sets and > >> > checksum files containing both versions (incompletely the older). > >> > >> I do not agree with "be lenient on what you expect thing that > >> used to be popular belief. So IMHO it is better if sysupgrade fails > >> when the files in the ftp directory do not match its expectations. > > > > There are a few different back-end failure modes going on here. It is > > more than a mirroring problem. The hash files contain a mix of files. > > I think stuart is right -- we are better off just plain failing. One > > day I will find time to improve the backend, but that's not now. > > >
Re: [newvers] sysupgrade(8) -release to -beta narrow of sets version
Stuart Henderson wrote: > > This results in failure to upgrade to a valid snapshot on the mirrors, > > and users having to wait a new snapshot fanout without mixed sets and > > checksum files containing both versions (incompletely the older). > > I do not agree with "be lenient on what you expect thing that > used to be popular belief. So IMHO it is better if sysupgrade fails > when the files in the ftp directory do not match its expectations. There are a few different back-end failure modes going on here. It is more than a mirroring problem. The hash files contain a mix of files. I think stuart is right -- we are better off just plain failing. One day I will find time to improve the backend, but that's not now.
Re: prevent re-upgrade in powerpc64 boot loader
Klemens Nanni wrote: > Are there more reasons or benefits to this approach than a) less intrusive > chmod() and b) more shared, overall less code? > > I obviously don't see the full picture (yet). A normal bootloader only reads from disk. No write occur. The chmod hack is a write. In the libsa case, it narrowly modifies just the block that is required, because libsa's filesystem code is not a buffering filesystem implimentation. But these octeon / powerpc64 bootloaders are real kernels, running the real ffs. When the "allow a block write" operation is activated, the root filesystem is *REALLY MOUNTED*, which means all the ffs read, read-ahead, inode update, and other write operations come into play. There is no complete "only do what libsa does" scheme. So the worry is that doing mount rw, chmod, mount ro, can create (or make worse) existing filesystem damage, to the point where maybe you cannot boot at all anymore.
Re: prevent re-upgrade in powerpc64 boot loader
Visa Hankala wrote:
> On Sat, Sep 23, 2023 at 02:26:18PM +, Klemens Nanni wrote:
> > On Sat, Sep 23, 2023 at 01:11:32PM +0200, Mark Kettenis wrote:
> > > > Date: Thu, 21 Sep 2023 22:30:01 +
> > > > From: Klemens Nanni
> > > >
> > > > In comparison to MI boot which only cares about /bsd.upgrade's x bit,
> > > > powerpc64 rdboot just wants a regular file.
> > > >
> > > > Require and strip u+x before execution to prevent sysupgrade(8) loop.
> > > > I'm new to powerpc64 and can't think of a reason to be different.
> > > >
> > > > Feedback? Objection? OK?
> > >
> > > So there is a problem with this approach. Calling chmod() will mean
> > > the bootloader will change the filesystem. What happens if you're
> > > booting with a root filesystem that was not cleanly unmounted?
> > > Modifying a forcibly mounted filesystem may not be without risk.
> >
> > Isn't that already the case with chmo() /etc/random.seed?
> >
> > Can you explain how that is not a problem in other non-kernel boot loaders
> > using libsa's ufs2_open() instead of mount(2)?
>
> chmod() through libsa's filesystem code modifies only the inode.
Yes, it is very minimal. I studied the code, and as far as I could see one
block is written back.
> Doing a mount-chmod-unmount cycle using the kernel triggers multiple
> writes to the filesystem.
For ffs, around 20 blocks if I studied it right. For libsa, I think 3
blocks are written back.
Majority of those are rewrites of recently read blocks.
I think the risk is overplayed, and we haven't heard of real damage have
we?
> In the past, I have pondered if octeon (and powerpc64) bootcode should
> use libsa instead of replicating the MI boot code. I did not use libsa
> initially because the libsa and libc APIs differ, and I did not want
> to use custom system call stubs. The original octboot/kexec interface
> was not suitable for libsa use, either.
Correct, I remember this discussion. It would be complicated interfacing
to the stupid libsa interface from anywhere else. It's not just libsa that
is the problem, but a new independent consumer. It will be fragile.
> However, the libsa and libc worlds can be reconciled with a trick.
> The libsa code is first compiled in standalone mode into a relinkable
> object ("saboot"). This object is then tweaked to avoid name conflicts
> with libc. Finally, the object is linked with glue code and libc into
> a proper rdboot executable that the kernel can run.
>
> Some have seen this before.
Yes. And I still worry about it. You still have a new application linked
against crappy libsa.
Next, someone changes libsa for amd64, the only architecture as far as they
are concerned, ok maybe they check arm64 also. But they don't ensure that
you get the memo. (Whoever does that change doesn't even *consider* the
chance that their code will affect another architecture, it's bad -- they
don't even mentally process that there are other architectures in the tree).
saboot continues to compile, but the rules of libsa have changed, and the
consequences are undefined.
But if you stick with ffs, those kind of yahoo operations are more rare.
So I'm not sure of the tradeoff.
Viable ROP-free roadmap for i386/armv8/riscv64/alpha/sparc64
20-some years ago I was very much involved in the integration of the
stack-protector into OpenBSD. This subsystem was developed as a gcc
patch by Hiroaki Etoh. Many years later it adopted and substantially
rewritten to incorporate it into mainline gcc. Thus, OpenBSD for a few
years was the first & only system with the stack protector.
Miod Vallat, Dale Rahn, (some forgotten), and I incorporated the code
into OpenBSD, fixed many problems with Etoh, and made some decisions
along the way.
One of these decisions was that the original stack protector protected
all functions. But this was too expensive. gcc at that time did not
know the types of various local variables were (to for instance, look
for character arrays). It only knew the total size. So it was I who
chose the value of 16, which has infected the ecosystem for 2 decades.
Only functions with 16 or more bytes of local storage are protected.
Another decision was where the stack protector check function was
located. We placed it into libc. And then we re-wrote it very
carefully to be reentrant and safe in all calling conditions; the
original proposal was not clean.
Originally there was only one cookie per program, but Matthew Dempsky
made changes so that every DSO (shared library object) had a different
cookie. So a program like ssh would have 6 cookies, and far more than
that in a dynamically linked browser. If you crash in one DSO and get
visibility of your own cookie, that doesn't help you do function calls
in another DSO (for example libc.so).
Via Matthew Dempsky and others, I provided ideas for better heuristic
to select functions to protect rather than the 16-byte heuristic, and
eventually some people at google wrote the -fstack-protector-strong
diffs for gcc and clang, because modern compilers keep better track of
the types and format of their local variables.
Years later, Todd Mortimer and I developed RETGUARD. At the start of
that initiative he proposed we protect all functions, to try to guard
all the RET instructions, and therefore achieve a state we call
"ROP-free". I felt this was impossible, but after a couple hurdles the
RETGUARD performance was vastly better than the stack protector and we
were able to protect all functions and get to ROP-free (on fixed-sized
instruction architecures). Performance was acceptable to trade against
improved security.
On variable-sized instruction architectures, polymorphic RET and other
control flow instructions can and will surface, but the available RET
gadgets are seriously reduced and exploitation may not be possible.
Other methods attempt to reduce the impact of the poly-gadgets. Still
the effort has value on all architectures. So amd64 isn't as good as
arm64, riscv64, mips64, powerpc, or powerpc64.
RETGUARD provides up to 4096 cookies per DSO, per-function, but limited to
avoid excessive bloat. It is difficult to do on architectures with very
few registers. Code was only written for clang, there is no gcc codebase
doing it. clang code for some architectures was never written (riscv64).
I hope that sets the stage for what is coming next.
We were able to enable RETGUARD on all functions because it was fast.
Why is RETGUARD fast, and the stack protector slow? In pseudo-code, the
OpenBSD stack-protector model creates function prologue and epilogue which
look like this:
{
local local_saved_cookie = per_DSO_cookie;
if (per_DSO_cookie != local_saved_cooke)
__stack_smash_handler(name_of_function)
}
This issues a warning to stdout, then crashes a manual SIGABRT, and you
can use the debugger on the core file in the wrong frame (you are inside
__stack_smash_handler, not at the momemt of the fault).
RETGUARD made a choice to not use a smash-reporting function, and instead
does:
{
local
if (retguard_matches>
illegal-instruction
}
So a detected-corruption causes an immediate crash, generally with a
SIGABRT, you get no detailed report. But you can use the debugger on the
core file in exactly the correct frame (an improvement).
At first glance RETGUARD is faster because it has less instructions.
But remember we are now living in a speculative-execution universe. A
few years ago some speculation reseachers I talked to pointed out that
the stack-protector generated instructions to do the call into
__stack_smash_handler(), and even many instructions inside the function
itself, are fetched, decoded, issued, and their results are discarded.
That's a waste of cpu resources. It might be a slowdown because those
execution slots are not used exclusively for straight-line speculation
following the RET. Modern cpus also have complicated branch-target
caches which may not be heuristically tuned to the stack protector approach.
On the other hand the RETGUARD approach uses an illegal instruction (of
some sort), which is a speculation barrier. That prevents the cpu from
heading off into an
Re: powerpc64 BOOT kernel question
Mark Kettenis wrote: > > Date: Fri, 22 Sep 2023 23:19:30 + > > From: Klemens Nanni > > > > Does the tiny kexec kernel actually need network, bio(4) or HID devices? > > octeon/BOOT does not have any of this. > > Well, we do need the USB keyboard stuff to allow users to type at the > bootloader prompt no? The USB mouse is clearly not needed, even on a > RAMDISK kernel. That's an oversight. Right. > I'm not sure about trimming the pseudo-device stuff. The fact that > you need that etherip line suggests that something is fishy. I think > I would keep the loop line instead since that is what we do on the > floppy kernels for i386 and amd64. The wsmux needs to be kept as well > of course. It suggests you are choosing some random network-device, to pull in the network support. When you delete stuff out of the kernel configuration, you are heading towards configurations that *noone has ever tested*. Do you really wish to be the first person to go there? For a configuration where it is almost impossible to collect meaningful debug information? That doesn't seem sane.
Re: powerpc64 BOOT kernel question
Klemens Nanni wrote: > Does the tiny kexec kernel actually need network, bio(4) or HID devices? I think you are playing with fire when you remove potential console devices.
Re: man 9 intro - improvements [and learning for me]?
Ingo, that's a bit cynical. As long as the process is slow, step by step, adding one or two manuals at a time, and focusing on being ACCURATE, then it will be good. It would be wrong to add inaccurate pages. A lack of documentation is slightly better than inaccurate documentation. So if you really want to do this, I suggest you start with the simplest ones first, get them correct, listen to the feedback you receive and figure out who has knowledge in each area, and adapt your review process along the way. Ingo is very familiar with this process, but perhaps also a bit jaded because he has done some very big documentation uplifts (and in each case, had the big plan to update a big area). Done in small bits, small but valuable improvements are not that difficult. Ingo Schwarze wrote: > Hi Christoff, > > of course you are free to work on whatever interests you, but if you are > looking for advice, i'd respectfully recommend that you try to work on > specifics rather than on generalities first, in particular when you > feel as if your experience in contributing isn't above average. > That applies even in the domain of documentation. > > Christoff Humphries wrote on Mon, Sep 18, 2023 at 12:21:48PM +: > > > I went searching for documentation about the kernel internals and was > > used to the intro(9) man page from NetBSD > > https://man.netbsd.org/intro.9 that had a lot more details. Would it > > be a worthwhile project to attempt to do the same for OpenBSD? > > Doing something like that may *sound* simple at first, but actually, > i can think of few documentation changes that would be harder to get > right and harder to get committed. > > Unless i'm totally off track, getting that right requires > > 1) a solid understanding of all areas of the kernel and > 2) a good understanding of what our kernel developers > actually need to know for their everyday work. > > If you have that knowledge, it's *still* much easier to improve > individual pages that are lacking in quality than improving the top-level > overview. Note that item 2 above tells you which of the pages are > probably most worthy of attention. > > Besides, the NetBSD intro(9) page does not strike me as particularly > convincing. *If* the lines in that page agree with the .Nd one-line > descriptions in the indivual pages, then it provides almost nothing > of value - but causes a maintenance burden because it needs to be > edited whenever any .Nd line changes. If the lines mismatch, then > the .Nd lines in the indivifual pages can likely be improved. > I did not check which is the case - possibly both are. > > We did have a few pages like that in the past, but most of those > got deleted because they provided little value. For example, > compare these two: > > https://man.openbsd.org/OpenBSD-5.6/string.3 > https://man.openbsd.org/OpenBSD-current/string.3 > > A very small number still exists, perhaps most notably > > https://man.openbsd.org/crypto.3 and > https://man.openbsd.org/ssl.3 > > The benefit of those is *not* that they exhaustively list everything - > indeed, apropos(1) is better at that job than a manually maintained > table of contents - but that they provide some high-level information > how the libraries as a whole are intended to be used that is hard to > figure out from individual pages. Also, these pages do not duplicate > .Nd lines. > > > I understand the annoyance of folks talking about what they intend or > > are going to do with no actual fruit, but wanted to check that the > > intro(9) is lacking and the information is not just stored somewhere > > else (other than "apropos -s 9 ."). > > Sorry, i lack the experience in kernel development that would be > required to judge whether any information could usefully be added > to intro(9). > > Yours, > Ingo > > > > I want to learn the internals of the OpenBSD kernel, too, and will do > > as mulander (and friends) did by learning a bit of code daily > > https://blog.tintagel.pl/2017/09/10/openbsd-daily-recap.html. > > > > Seeking the learn and contribute, especially in the uvm, vmd/vmm, and > > possibly filesystem subsystems. >
Re: [patch] Sort of fix for game "phantasia"
No way. I do not think that is good advice, either, because that user can then fill /var. S V wrote: > Awesome, You and William Ahern really showed me that is "security > mindset". I simply didn't think about some of this points. Thanks for > that. > > Can I propose as my "last" attempt on this topic to add more > clarification "how to fix it by user on his machine" with this patch > > 2023-09-16 5:14 GMT+03:00, Theo de Raadt : > > > > revision 1.18 > > date: 2015/11/24 03:10:10; author: deraadt; state: Exp; lines: +1 -2; > > commitid: NZfzN0SfUUHBE4HF; > > In 1995, all of the games were setuid games. At end of 1996, I took them > > all > > to setgid games, and we started wittling them down. Nearly 10 years later > > I > > am removing all setgid from the games. If any of these have score files > > they > > are now broken, and I hope various folk repair them. I have argued for > > years > > (and received pushback...) that the score file features must be removed, or > > rewritten to use private files, because setgid is the wrong tool. > > ok tedu > > > > We will not bring back setgid-supported scorefiles. > > > > > > S V wrote: > > > >> Why don't drop it in this case? > >> > >> сб, 16 сент. 2023 г., 05:03 Theo de Raadt : > >> > >> Nope, 'setgid games' is intentionally dead. > >> > >> We will not be bringing it back. > >> > >> S V wrote: > >> > >> > It is pretty to not have this game running, but included so > >> > > >> > I just added chmod ugo+rw "game files" after chown root:games in > >> Makefile > >> > > >> > and add notice about it in man file > >> > > >> > It is unbroke game and place it to playable state. > >> > > >> > Thanks in advance! > >> > > >> > -- Slava Voronzoff > >> > > > > > -- > Nerfur Dragon > -==(UDIC)==-
Re: [patch] Sort of fix for game "phantasia"
It is a poor trade. Giving a user an additional gid or uid (for program lifetime), in programs which have not been reviewed (or -- cannot be reviewed and fixed), is not good. Before, setgid games could be used along with another bug to fill /var. Now, you can just fill /var because you made it writeable to all. You are making this program give the user an ability that didn't exist before. The ability to fill /var. Shit breaks pretty badly when /var is full. Score files in this garbage program are not important enough to create that risk for users.
Re: [patch] Sort of fix for game "phantasia"
revision 1.18 date: 2015/11/24 03:10:10; author: deraadt; state: Exp; lines: +1 -2; commitid: NZfzN0SfUUHBE4HF; In 1995, all of the games were setuid games. At end of 1996, I took them all to setgid games, and we started wittling them down. Nearly 10 years later I am removing all setgid from the games. If any of these have score files they are now broken, and I hope various folk repair them. I have argued for years (and received pushback...) that the score file features must be removed, or rewritten to use private files, because setgid is the wrong tool. ok tedu We will not bring back setgid-supported scorefiles. S V wrote: > Why don't drop it in this case? > > сб, 16 сент. 2023 г., 05:03 Theo de Raadt : > > Nope, 'setgid games' is intentionally dead. > > We will not be bringing it back. > > S V wrote: > > > It is pretty to not have this game running, but included so > > > > I just added chmod ugo+rw "game files" after chown root:games in Makefile > > > > and add notice about it in man file > > > > It is unbroke game and place it to playable state. > > > > Thanks in advance! > > > > -- Slava Voronzoff >
Re: [patch] Sort of fix for game "phantasia"
Nope, 'setgid games' is intentionally dead. We will not be bringing it back. S V wrote: > It is pretty to not have this game running, but included so > > I just added chmod ugo+rw "game files" after chown root:games in Makefile > > and add notice about it in man file > > It is unbroke game and place it to playable state. > > Thanks in advance! > > -- Slava Voronzoff
Re: coping with large shell commands in make(1)
I did not point you in this direction, not at all.
I said make execve failure report an accurate error. Rather than
requiring a special hook which will report the problem. Meaning
if the problem areas were known, they could be coped with. What
I'm seeing here appears to be a thing like system(), which will
punt the problem further along, to a shell, which will also probably
report the error poorly -- I suspect the more layers you pile on
top of ARG_MAX and/or E2BIG, the worse this will become.
Marc Espie wrote:
> Theo pointed me in another direction
>
> Proof of concept that appears to work just fine
> (very minimal error checking so far)
>
> Of course, if we end up running a command on somethin too large,
> thi will fail as usual, but much to my surprise,
>
> make show=_FULL_FETCH_LIST
> in sysutils/telegraf worked with this.
>
> (and then I remembered that echo is a built-in, so there's no long exec
> involved)
>
> Index: cmd_exec.c
> ===
> RCS file: /cvs/src/usr.bin/make/cmd_exec.c,v
> retrieving revision 1.11
> diff -u -p -r1.11 cmd_exec.c
> --- cmd_exec.c16 Jan 2020 16:07:18 - 1.11
> +++ cmd_exec.c26 Aug 2023 20:06:45 -
> @@ -36,6 +36,7 @@
> #include "memory.h"
> #include "pathnames.h"
> #include "job.h"
> +#include "engine.h"
>
> char *
> Cmd_Exec(const char *cmd, char **err)
> @@ -54,7 +55,7 @@ Cmd_Exec(const char *cmd, char **err)
> *err = NULL;
>
> /* Set up arguments for the shell. */
> - args[0] = "sh";
> + args[0] = _PATH_BSHELL;
> args[1] = "-c";
> args[2] = (char *)cmd;
> args[3] = NULL;
> @@ -82,7 +83,9 @@ Cmd_Exec(const char *cmd, char **err)
> (void)close(fds[1]);
> }
>
> - (void)execv(_PATH_BSHELL, args);
> + (void)execv(args[0], args);
> + if (errno == E2BIG)
> + retry_with_temp_file(false, args);
> _exit(1);
> /*NOTREACHED*/
>
> Index: engine.c
> ===
> RCS file: /cvs/src/usr.bin/make/engine.c,v
> retrieving revision 1.71
> diff -u -p -r1.71 engine.c
> --- engine.c 30 May 2023 04:42:21 - 1.71
> +++ engine.c 26 Aug 2023 20:06:45 -
> @@ -549,6 +549,30 @@ recheck_command_for_shell(char **av)
> return av;
> }
>
> +void
> +retry_with_temp_file(bool errCheck, char **args)
> +{
> + char template[] = "/tmp/shell.XX";
> + int fd;
> + char buf[50];
> + int i = 1;
> +
> + fd = mkstemp(template);
> + snprintf(buf, sizeof(buf), "/dev/fd/%d", fd);
> +
> + if (fd == -1)
> + return;
> + unlink(template);
> + write(fd, args[2], strlen(args[2]));
> + lseek(fd, 0, SEEK_SET);
> + if (errCheck)
> + args[i++] = "-e";
> + args[i++] = buf;
> + args[i++] = NULL;
> + execv(args[0], args);
> +}
> +
> +
> static void
> run_command(const char *cmd, bool errCheck)
> {
> @@ -583,6 +607,8 @@ run_command(const char *cmd, bool errChe
> todo = av;
> }
> execvp(todo[0], todo);
> + if (errno == E2BIG && todo == shargv)
> + retry_with_temp_file(errCheck, todo);
>
> if (errno == ENOENT)
> fprintf(stderr, "%s: not found\n", todo[0]);
> Index: engine.h
> ===
> RCS file: /cvs/src/usr.bin/make/engine.h,v
> retrieving revision 1.17
> diff -u -p -r1.17 engine.h
> --- engine.h 13 Jan 2020 15:12:58 - 1.17
> +++ engine.h 26 Aug 2023 20:06:45 -
> @@ -138,4 +138,5 @@ extern bool job_run_next(Job *);
> */
> extern void handle_job_status(Job *, int);
>
> +extern void retry_with_temp_file(bool, char **);
> #endif
>
Re: [patch] netcat: support --crlf
Pietro Cerutti wrote: > The motivation is that several network protocols are line oriented > with CRLF as line terminators. SMTP and HTTP are among the most > popular. Yet, all servers of those protocols and and will accept the simpler 1-byte line terminator. > FWIW, it works on RHEL 7.9 That isn't based on our nc. It is a replacement tool with other options. There is no standard, instead there are 4-5 tools with the same name and an increasing number of incompatible options. People just can't resist adding incompatible changes and extensions. We ourselves added a bunch of new TLS-related options because "openssl s_client" is COMPLETELY UNSUITABLE FOR ANY USE BECAUSE OF the "line begins with R or Q" behaviour. I'm not sure where all of this will go in the long term, but noone seems to want act like they desire convergence. I'll predict that option/capability convergence is less likely than one of them eventually shipping with a built-in extension langauge (like maybe a lisp, or one of the many incompatible BPF extensions that are all the rage).
Re: Have sysupgrade run fw_update -vv
Stuart Henderson wrote: > On 2023/08/13 11:44, Andrew Hewus Fresh wrote: > > My laptop doesn't have the fastest wifi and sysupgrade already uses a > > progress bar to show what it's doing, so I'd really like to provide more > > feedback on what it is doing: > > Does a single -v give enough feedback? As shown below, -vv is ridiculously verbose. I think -v is also too verbose. If you are going to do a progress bar, I think it should be one progress bar. I am sure there is a machine out there where -vv will do more than 24 lines of output. If not today, eventually. So I don't think we want -vv or -v in in sysupgrade or the installer, unless the output is changed to be less verbose. The IMPORTANT information from sysupgrade or the installer... is not this wall of text. > It's a fair bit quieter (it > doesn't include the time estimates from ftp, but does still prints > what it's doing before it starts downloading anything potentially > large, so at least you aren't sat there downloading 12Mb of intel- > or iwx-firmware or 25Mb of amdgpu-firmware with zero idea about > what it's doing). > > # fw_update -vv > Detect firmware ... found. > Get/Verify SHA256.sig 100% |**| 2371 00:00 > > Get/Verify intel-firmware-2023080... 100% |*| 12155 KB00:07 > > Install intel-firmware-2023080... 100% || 12155 KB00:00 > > Get/Verify iwx-firmware-20230330.tgz 100% |*| 12890 KB00:48 > > Install iwx-firmware-20230330.tgz 100% || 12890 KB00:00 > > Get/Verify vmm-firmware-1.14.0p4.tgz 100% |*| 42927 00:00 > > Install vmm-firmware-1.14.0p4.tgz 100% || 42927 00:00 > > fw_update: added intel,iwx,vmm; updated none; kept inteldrm,uvideo > > vs. > > # fw_update -v > Get/Verify intel-firmware-20230808v0.tgz ... installed. > Get/Verify iwx-firmware-20230330.tgz ... installed. > Get/Verify vmm-firmware-1.14.0p4.tgz ... installed. > fw_update: added intel,iwx,vmm; updated none; kept inteldrm,uvideo > > > $ time doas fw_update > > fw_update: added intel; updated none; kept inteldrm,iwm,uvideo,vmm > > 0m58.45s real 0m00.51s user 0m00.35s system > > firmware.openbsd.org is handled entirely by DNS round-robin with no > geographical awareness, so even with good local network and internet > connection, it can sometimes still take a very long time. For example, > times from two consecutive runs fetching intel-firmware on a 550M > download connection: > > 0m10.11s real 0m00.71s user 0m00.77s system > 1m17.47s real 0m01.28s user 0m01.22s system >
Re: smr_grace_wait(): Skip halted CPUs
Visa Hankala wrote: > On Sat, Aug 12, 2023 at 01:29:10PM +0200, Martin Pieuchot wrote: > > On 12/08/23(Sat) 10:57, Visa Hankala wrote: > > > On Fri, Aug 11, 2023 at 09:52:15PM +0200, Martin Pieuchot wrote: > > > > When stopping a machine, with "halt -p" for example, secondary CPUs are > > > > removed from the scheduler before smr_flush() is called. So there's no > > > > need for the SMR thread to peg itself to such CPUs. This currently > > > > isn't a problem because we use per-CPU runqueues but it doesn't work > > > > with a global one. So the diff below skip halted CPUs. It should also > > > > speed up rebooting/halting on machine with a huge number of CPUs. > > > > > > Because SPCF_HALTED does not (?) imply that the CPU has stopped > > > processing interrupts, this skipping is not safe as is. Interrupt > > > handlers might access SMR-protected data. > > > > Interesting. This is worse than I expected. It seems we completely > > forgot about suspend/resume and rebooting when we started pinning > > interrupts on secondary CPUs, no? Previously sched_stop_secondary_cpus() > > was enough to ensure no more code would be executed on secondary CPUs, > > no? Wouldn't it be better to remap interrupts to the primary CPU in > > those cases? Is it easily doable? > > I think device interrupt stopping already happens through > config_suspend_all(). Right. DVACT_QUIESCE is supposed to stop the device's transaction flow, which implies it stops scheduling more things which will cause interrupts. DVACT_SUSPEND is supposed to stop the device even further, so that it can save a hardware state into memory, which is viable for recovery later on. For hibernate, this is even more strict, because memory side effects must not occur after these phases. I suppose there is nothing which says interrupts must be stopped for DVACT_SUSPEND. A driver could ACK the interrupts, but create no software side effects. Anyways, for the halt/reboot case, config_suspend_all() is not done. We have architectures that don't suspend/resume, but must halt/reboot. We have not reviewed their driver stack to check if config_suspend_all() would do the right thing. This is not trivial, it isn't just in the drivers. Some of our non-suspending architectures could have incorrectly ordered device trees...
Re: installer: disk crypto: crank KDF rounds to hardware based default
Mark Kettenis wrote: > > Date: Fri, 11 Aug 2023 11:13:23 + > > From: Klemens Nanni > > > > On Mon, May 08, 2023 at 11:00:27AM +, Klemens Nanni wrote: > > > On Sun, Apr 23, 2023 at 05:07:30PM +, Klemens Nanni wrote: > > > > For new installs, it seems adequate to base the number on the actual > > > > hardware, > > > > assuming the CRYPTO volume will stay in that hardware for a while. > > > > > > > > The current default of 16 is from old PKCS5 PBKDF2 times and changing > > > > it in > > > > bioctl(8) is a more invasive change (for later, perhaps). > > > > > > > > Thoughts? Feedback from the crypto folks appreciated. > > > > > > > > On X230 and T14, 16 feels pretty instant, whereas 'auto' takes about a > > > > second > > > > on a T14. > > > > > > Ping. > > > > Anyone? > > > > I consider a hardware based value a saner default for new installations > > (root disk volumes are most likely to stick around on the same machine) > > than a decade old constant. > > See the recent discussion about _bcrypt_autorounds() in libc. > > System performance varies, and even on modern hardware it can provide > varying results. The ramdisk environment is considerably different > from the mult-user environment in this respect. > > Using a fixed value is way more predictable. If 16 no longer is a > safe default it should be raised. I think this case is different, because the ramdisk has no process contention. The code still sticks to minimum 16: if (r < 16) r = 16; On faster machines, it will increase the rounds. For that machine, for that disk configuration. This is processed early on to bring the disk up, when there is little or no contention. So it will not have a regressive performance impact. It will be very rare to use this in a non-ramdisk circumstance. It is different then the user 'password change' operation.
Re: ldd: check read return value to avoid unitialized struct fields
Greg Steuck wrote: > Thanks for the patch. > > I could see some value in tightening the conditions to always check > `!= expected`. I don't see enough improvement from separating the error > case of -1 from the incomplete read case considering the otherwise > identical behavior. Hmm, that is a valid point. We want this code to be as simple as possible, and the effect of these errors is identical.
Re: Bringing in OpenBSD
enh wrote: > (fwiw, Android has someone working on adding this header to upstream > LLVM right now, so hopefully it should come "for free" in llvm 18. > what, if anything, to _do_ with it is another question though :-) ) Will stdckdint.h come into common usage before or after 32-bit time_t wraps? That's what I want to know.
Re: Bringing in OpenBSD
Lucian Popescu wrote: > I noticed that some parts of OpenBSD use awkward techniques to detect > undefined behavior in arithmetic operations, for example: ... > The snippet was taken from lib/libexpat/lib/xmlparse.c libexpat is outside source, which we incorporate We have no influence over what the upstream libexpat does, but I can *ASSURE YOU* there is zero change they will be switching to C23-only support tomorrow, and breaking compatibility with the entire universe. So that is such a terrible example. You couldn't find a better example?
Re: ldd: check read return value to avoid unitialized struct fields
That looks better.
Lucas wrote:
> Theo Buehler wrote:
> > > - if (pread(fd, phdr, size, ehdr.e_phoff) != size) {
> > > + if ((nr = pread(fd, phdr, size, ehdr.e_phoff)) != -1) {
> >
> > did you intend to check for == -1?
> >
> > > warn("read(%s)", name);
> >
> > should that not say pread?
>
> Indeed, thanks for spotting both things.
>
>
> ---
> commit 92f58b2a1cd576c3e72303004388ab1e9709e327 (ldd-read-rv)
> from: Lucas
> date: Sat Aug 5 16:34:16 2023 UTC
>
> Check {,p}read return values consistently
>
> Check that read performs a full header read. Explicitly check against -1
> for failure instead of < 0. Split pread error message between error
> handling and short reads. Promote size from int to size_t.
>
> M libexec/ld.so/ldd/ldd.c
>
> diff 194ff02fb6be247e27fe964e901d891d99ec0b74
> 92f58b2a1cd576c3e72303004388ab1e9709e327
> commit - 194ff02fb6be247e27fe964e901d891d99ec0b74
> commit + 92f58b2a1cd576c3e72303004388ab1e9709e327
> blob - 9e8c5065cd843ff36d91efcb868b94ffd4c98365
> blob + 532feb9855a03480c289fa2188768657a4f82e7c
> --- libexec/ld.so/ldd/ldd.c
> +++ libexec/ld.so/ldd/ldd.c
> @@ -96,7 +96,9 @@ doit(char *name)
> {
> Elf_Ehdr ehdr;
> Elf_Phdr *phdr;
> - int fd, i, size, status, interp=0;
> + size_t size;
> + ssize_t nr;
> + int fd, i, status, interp=0;
> char buf[PATH_MAX];
> struct stat st;
> void * dlhandle;
> @@ -118,11 +120,16 @@ doit(char *name)
> return 1;
> }
>
> - if (read(fd, , sizeof(ehdr)) < 0) {
> + if ((nr = read(fd, , sizeof(ehdr))) == -1) {
> warn("read(%s)", name);
> close(fd);
> return 1;
> }
> + if (nr != sizeof(ehdr)) {
> + warnx("%s: incomplete ELF header", name);
> + close(fd);
> + return 1;
> + }
>
> if (!IS_ELF(ehdr) || ehdr.e_machine != ELF_TARG_MACH) {
> warnx("%s: not an ELF executable", name);
> @@ -140,12 +147,18 @@ doit(char *name)
> err(1, "reallocarray");
> size = ehdr.e_phnum * sizeof(Elf_Phdr);
>
> - if (pread(fd, phdr, size, ehdr.e_phoff) != size) {
> - warn("read(%s)", name);
> + if ((nr = pread(fd, phdr, size, ehdr.e_phoff)) == -1) {
> + warn("pread(%s)", name);
> close(fd);
> free(phdr);
> return 1;
> }
> + if (nr != size) {
> + warnx("%s: incomplete program header", name);
> + close(fd);
> + free(phdr);
> + return 1;
> + }
> close(fd);
>
> for (i = 0; i < ehdr.e_phnum; i++)
>
Re: ldd: check read return value to avoid unitialized struct fields
Lucas wrote:
> "Theo de Raadt" wrote:
> > What errno is being printed here?
>
> """Everything is alright""" error,
>
> $ : >empty && ./obj/ldd empty
> ldd: read(empty): Undefined error: 0
>
> which would be the same as a short read in the pread below.
Nope, that is not correct.
errno is not being cleared. It just happens to be zero. Future
code changes could insert another operation above which would set
errno, and then this would print a report about that error.
No, your diff is still wrong.
errno is only updated when a system call returns -1.
So your diff is looking at an old, unrelated, errno.
And instead of the previous diff which did this once, you are now
doing it 5 times.
> ---
> commit f1255c0035aa37752a298b752fd20215a1d7adef (ldd-read-rv)
> from: Lucas
> date: Sat Aug 5 14:36:58 2023 UTC
>
> Check {,p}read return values consistently
>
> Check that read performs a full header read. Explicitly check against -1
> for failure instead of < 0. Split pread error message between error
> handling and short reads. Promote size from int to size_t.
>
> M libexec/ld.so/ldd/ldd.c
>
> diff 7b0c383483702d9a26856c2b4754abb44950ed82
> f1255c0035aa37752a298b752fd20215a1d7adef
> commit - 7b0c383483702d9a26856c2b4754abb44950ed82
> commit + f1255c0035aa37752a298b752fd20215a1d7adef
> blob - 9e8c5065cd843ff36d91efcb868b94ffd4c98365
> blob + 12777f2420a6a74f9f456f080c207bf47760b258
> --- libexec/ld.so/ldd/ldd.c
> +++ libexec/ld.so/ldd/ldd.c
> @@ -96,7 +96,9 @@ doit(char *name)
> {
> Elf_Ehdr ehdr;
> Elf_Phdr *phdr;
> - int fd, i, size, status, interp=0;
> + size_t size;
> + ssize_t nr;
> + int fd, i, status, interp=0;
> char buf[PATH_MAX];
> struct stat st;
> void * dlhandle;
> @@ -118,11 +120,16 @@ doit(char *name)
> return 1;
> }
>
> - if (read(fd, , sizeof(ehdr)) < 0) {
> + if ((nr = read(fd, , sizeof(ehdr))) == -1) {
> warn("read(%s)", name);
> close(fd);
> return 1;
> }
> + if (nr != sizeof(ehdr)) {
> + warnx("%s: incomplete ELF header", name);
> + close(fd);
> + return 1;
> + }
>
> if (!IS_ELF(ehdr) || ehdr.e_machine != ELF_TARG_MACH) {
> warnx("%s: not an ELF executable", name);
> @@ -140,12 +147,18 @@ doit(char *name)
> err(1, "reallocarray");
> size = ehdr.e_phnum * sizeof(Elf_Phdr);
>
> - if (pread(fd, phdr, size, ehdr.e_phoff) != size) {
> + if ((nr = pread(fd, phdr, size, ehdr.e_phoff)) != -1) {
> warn("read(%s)", name);
> close(fd);
> free(phdr);
> return 1;
> }
> + if (nr != size) {
> + warnx("%s: incomplete program header", name);
> + close(fd);
> + free(phdr);
> + return 1;
> + }
> close(fd);
>
> for (i = 0; i < ehdr.e_phnum; i++)
>
Re: ldd: check read return value to avoid unitialized struct fields [was "ldd: check {,p}read return
Lucas wrote:
> Bump.
>
> Lucas wrote:
> > Now with a better subject.
> >
> > I was also wondering about the lack of pledge() other than the newly
> > added one. That goes because dlopen() can do anything?
> >
> > Lucas wrote:
> > > Hi,
> > >
> > > I wanted to understand how the pledge execpromises commit worked in ldd
> > > and went to read it, and noticed that there is both a
> > >
> > > if (read(fd, , sizeof(ehdr)) < 0) {
> > >
> > > and a
> > >
> > > if (pread(fd, phdr, size, ehdr.e_phoff) != size) {
> > >
> > > In particular, the "read < 0" confused me quite a lot, but the manpage
> > > states that, if the file descriptor _is a regular file_ and there are
> > > enough bytes, it reads to completion. The check for a being a regular
> > > file is already in place, but there is nothing guarding against a short
> > > file, so check instead if read == sizeof(ehdr).
> > >
> > > -Lucas
>
>
> diff refs/heads/master refs/heads/ldd-read-rv
> commit - 7b0c383483702d9a26856c2b4754abb44950ed82
> commit + 862cbcc132ebcd92cb4b44eb1b453ea9ada0bbc3
> blob - 9e8c5065cd843ff36d91efcb868b94ffd4c98365
> blob + ad624d9cd0e72944b93e951de9b31f57a6258601
> --- libexec/ld.so/ldd/ldd.c
> +++ libexec/ld.so/ldd/ldd.c
> @@ -118,7 +118,7 @@ doit(char *name)
> return 1;
> }
>
> - if (read(fd, , sizeof(ehdr)) < 0) {
> + if (read(fd, , sizeof(ehdr)) != sizeof(ehdr)) {
> warn("read(%s)", name);
What errno is being printed here?
> close(fd);
> return 1;
>
Re: bgpd, be more carefule with shutdown reason
Theo Buehler wrote: > On Fri, Aug 04, 2023 at 11:40:36AM +0200, Claudio Jeker wrote: > > When copying the shutdown reason from ctl_neighbor into the peer struct > > the strlcpy needs a NUL terminated string input. This may not be the case > > so we should be more careful here. > > I see two ways to fix this. > > a) force in a NUL before callin strlcpy() as done below. > > b) use memcpy() and then force terminate p->conf.reason. > > I think either approach is fine. A third option would be > > c) snprintf with "%.*s" To me that always smells like "it isn't a string, we'll remember that, and handle the situation later". But.. it... isn't... a string. Gross. So I would always lean towards code that insists on passing the 0 byte at every stage, even if it has to also pass strlen+1 for a block region size. And if you find one of these to copy, always convert it to a real string ASAP.
Re: uvm_loadav: don't recompute schedstate_percpu.spc_nrun
Scott Cheloha wrote: > On Thu, Aug 03, 2023 at 09:09:30AM -0600, Theo de Raadt wrote: > > Scott Cheloha wrote: > > > > > > > How about this. Kill the spc_ldavg calculation. Its use is more then > > > > > questionable. The cpu selection code using this is not wroking well > > > > > and > > > > > process stealing will do the rest. > > > > > > This is more or less what I said yesterday. The per-CPU load average > > > is not useful for deciding where to put a thread. > > > > I guess you have not been reading mpi's scheduler diff. The entire idea > > of "placing a thread" is 1980's single-processor flawed. > > Dude, I'm not talking about mpi's patch, I'm talking about what's in > the tree. > > Given the current state of the scheduler, we can throw out spc_ldavg. > It isn't necessary. > > > > Of the variables we > > > have available to consider, only the current length of the runqueue is > > > useful. > > > > No, that concept is also broken. > > Again, I am talking about the current scheduler. > > Said another way: the current approach can limp along just fine using > only the runqueue length. We can get rid of spc_ldavg without > worrying about it. I'm just saying all of us need to recognize that it is impossible to defend the in-tree code. Anways, you said "the current length of the runqueue is useful". It is only useful in choosing a different stupid runqueue.
Re: uvm_loadav: don't recompute schedstate_percpu.spc_nrun
Scott Cheloha wrote: > > > How about this. Kill the spc_ldavg calculation. Its use is more then > > > questionable. The cpu selection code using this is not wroking well and > > > process stealing will do the rest. > > This is more or less what I said yesterday. The per-CPU load average > is not useful for deciding where to put a thread. I guess you have not been reading mpi's scheduler diff. The entire idea of "placing a thread" is 1980's single-processor flawed. > Of the variables we > have available to consider, only the current length of the runqueue is > useful. No, that concept is also broken. On your 8-cpu laptop, the runqueue does not work at all. Typically, the number of available cpu's exceeds the ready-to-run processes. For workloads where the ready process count exceeds the cpus, the processes get put onto the wrong cpu's queues -- and because scheduler code runs so rarely, this is all a waste of time. What actually happens is pretty much all process selection happen based upon a process on a cpu going to sleep, and that cpu finds it's runqueue is empty because other cpu's have stolen it empty, so that cpu proceeds to steal out of another cpu's runqueue. All process progress really depends upon stealing processes, fixing the other cpu's runqueue with locks, and thus ignoring any pre-calculation by the 'scheduler code'. All of this stealing requires big locks, to protect the scheduler code which is occasionally (let's be honest -- rarely?) re-organizing these stupid runqueues, which then get ignored in the typical case. Those locks are so crazy weird, we've been confused for decades about how to improve it. It appears there are no small steps, and we probably need a "Briandead / Dead Alive" lawnmower procedure, and then rebuild afterwards. I think you will soon join the club of people who believe this code from 1980 is so completely unsuitable that not one line of it can be reused.
[no subject]
S V wrote: > 2023-07-27 17:24 GMT+03:00, Theo de Raadt : > > You don't explain why you are trying to enable floating point register > > use in the kernel. > > I just have CPU with it (Cortex-a57 with NEON), so was toying with it > trying to look if I get more performance. > Got this error and decided to post, thinking - maybe I got some "not > yet discovered" problem. > > I'm not good with kernel development (sent only simple patches) so was > curious about it. What you are trying to do won't work at all. If the kernel uses floating point registers, it must save/restore them at multiple types of context switching points, which is why kernels normally do not do this, except in very controlled fashion. One doesn't set a compile option, and get this. You are in way over your head
Re:
You don't explain why you are trying to enable floating point register use in the kernel. S V wrote: > I was trying (as an experiment) to build aarch64 current kernel with > -march=armv8-a+simd and stumble upon error > > Interesting to notice that armv8-a+nofp+simd compiles and runs OK > > part of output with error: > > cc -g -Werror -Wall -Wimplicit-function-declaration -Wno-pointer-sign > -Wno-constant-conversion -Wno-address-of-packed-member > -Wno-unused-but-set-variable -Wno-gnu-folding-constant > -Wframe-larger-than=2047 -Wno-deprecated-non-prototype > -Wno-unknown-warning-option -march=armv8-a+fp+simd > -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer -ffixed-x18 > -ffreestanding -fno-pie -mbranch-protection=bti -O2 -pipe -nostdinc > -I/usr/src/sys -I/usr/src/sys/arch/arm64/compile/CUSTOM.MP/obj > -I/usr/src/sys/arch -I/usr/src/sys/dev/pci/drm/include > -I/usr/src/sys/dev/pci/drm/include/uapi > -I/usr/src/sys/dev/pci/drm/amd/include/asic_reg > -I/usr/src/sys/dev/pci/drm/amd/include > -I/usr/src/sys/dev/pci/drm/amd/amdgpu > -I/usr/src/sys/dev/pci/drm/amd/display > -I/usr/src/sys/dev/pci/drm/amd/display/include > -I/usr/src/sys/dev/pci/drm/amd/display/dc > -I/usr/src/sys/dev/pci/drm/amd/display/amdgpu_dm > -I/usr/src/sys/dev/pci/drm/amd/pm/inc > -I/usr/src/sys/dev/pci/drm/amd/pm/legacy-dpm > -I/usr/src/sys/dev/pci/drm/amd/pm/swsmu > -I/usr/src/sys/dev/pci/drm/amd/pm/swsmu/inc > -I/usr/src/sys/dev/pci/drm/amd/pm/swsmu/smu11 > -I/usr/src/sys/dev/pci/drm/amd/pm/swsmu/smu12 > -I/usr/src/sys/dev/pci/drm/amd/pm/swsmu/smu13 > -I/usr/src/sys/dev/pci/drm/amd/pm/powerplay/inc > -I/usr/src/sys/dev/pci/drm/amd/pm/powerplay/hwmgr > -I/usr/src/sys/dev/pci/drm/amd/pm/powerplay/smumgr > -I/usr/src/sys/dev/pci/drm/amd/pm/swsmu/inc > -I/usr/src/sys/dev/pci/drm/amd/pm/swsmu/inc/pmfw_if > -I/usr/src/sys/dev/pci/drm/amd/display/dc/inc > -I/usr/src/sys/dev/pci/drm/amd/display/dc/inc/hw > -I/usr/src/sys/dev/pci/drm/amd/display/dc/clk_mgr > -I/usr/src/sys/dev/pci/drm/amd/display/modules/inc > -I/usr/src/sys/dev/pci/drm/amd/display/modules/hdcp > -I/usr/src/sys/dev/pci/drm/amd/display/dmub/inc -DDDB -DDIAGNOSTIC > -DKTRACE -DACCOUNTING -DKMEMSTATS -DPTRACE -DPOOL_DEBUG -DCRYPTO > -DSYSVMSG -DSYSVSEM -DSYSVSHM -DUVM_SWAP_ENCRYPT -DFFS -DFFS2 > -DFFS_SOFTUPDATES -DUFS_DIRHASH -DQUOTA -DEXT2FS -DMFS -DNFSCLIENT > -DNFSSERVER -DCD9660 -DUDF -DMSDOSFS -DFIFO -DFUSE -DSOCKET_SPLICE > -DTCP_ECN -DTCP_SIGNATURE -DINET6 -DIPSEC -DPPP_BSDCOMP -DPPP_DEFLATE > -DPIPEX -DMROUTING -DMPLS -DBOOT_CONFIG -DPCIVERBOSE -DUSER_PCICONF > -DUSBVERBOSE -DSUSPEND -DWSDISPLAY_COMPAT_USL > -DWSDISPLAY_COMPAT_RAWKBD -DWSDISPLAY_DEFAULTSCREENS="6" > -DONEWIREVERBOSE -DMULTIPROCESSOR -DMAXUSERS=80 -D_KERNEL -D__arm64__ > -MD -MP -c /usr/src/sys/dev/usb/ohci.c > /usr/src/sys/dev/usb/ohci.c:708:1: error: stack frame size (2192) > exceeds limit (2047) in function 'ohci_init' > [-Werror,-Wframe-larger-than] > ohci_init(struct ohci_softc *sc) > ^ > 1 error generated. > *** Error 1 in /usr/src/sys/arch/arm64/compile/CUSTOM.MP > (Makefile:1562 'ohci.o') > > > -- > Nerfur Dragon > -==(UDIC)==- >
Re: Zenbleed
Manawyrm wrote: > (Hetzner engineer here, but speaking as a private individual) > > Hetzner Cloud is using regular mainline QEMU on Linux as the hypervisor, > so while I'd agree that faulting when the MSR is set isn't ideal, this > behaviour will probably also occur on a lot of other machines/setups. > > Another solution might be preferrable. Linux has started to do a very > similar thing and I haven't yet checked if it runs into a similar > situation/fault. > > At least on Hetzner Cloud, there is a lot of indications that the > machine is running as a virtual machine (ACPI/SMBIOS, etc.). If > possible, maybe skip setting the flag when running virtualized? Indeed. We now check the cpuid ecx HV flag before performing this specific wrmsr. This kind of thing seems to keep coming up. In our codebase, we have a way to handle a faulting rdmsr. We don't have a maching way to handle a faulting wrmsr. We sort of don't want the latter..
Re: arm64 BTI support for mpg123
Mark Kettenis wrote: > I'm not sure to what extent this makes IBT less effective. Can the > retpolines be used as gadgets to bypass IBT? Should we stop enabling > retpolines by default? > > What *is* obvious is that retpolines are incompatible wuth shadow > stacks. Is there an alternative that doesn't replace the indirect > branch with a return instruction? It is clear however that both AMD and Intel have seperate (yet compatible) strategies to encourage you to buy newer chips.
Re: Zenbleed
Would like to know if this patch helps anyone with this type of
problem.
Index: sys/arch/amd64/amd64/cpu.c
===
RCS file: /cvs/src/sys/arch/amd64/amd64/cpu.c,v
retrieving revision 1.172
diff -u -p -u -r1.172 cpu.c
--- sys/arch/amd64/amd64/cpu.c 24 Jul 2023 14:53:58 - 1.172
+++ sys/arch/amd64/amd64/cpu.c 25 Jul 2023 03:28:35 -
@@ -1216,7 +1216,8 @@ cpu_fix_msrs(struct cpu_info *ci)
if (msr != nmsr)
wrmsr(MSR_DE_CFG, nmsr);
}
- if (family == 0x17 && ci->ci_model >= 0x31) {
+ if (family == 0x17 && ci->ci_model >= 0x31 &&
+ (cpu_ecxfeature & CPUIDECX_HV) == 0) {
nmsr = msr = rdmsr(MSR_DE_CFG);
nmsr |= DE_CFG_SERIALIZE_9;
if (msr != nmsr)
Index: sys/arch/i386/i386/machdep.c
===
RCS file: /cvs/src/sys/arch/i386/i386/machdep.c,v
retrieving revision 1.664
diff -u -p -u -r1.664 machdep.c
--- sys/arch/i386/i386/machdep.c24 Jul 2023 14:54:00 - 1.664
+++ sys/arch/i386/i386/machdep.c25 Jul 2023 03:28:29 -
@@ -1993,7 +1993,8 @@ identifycpu(struct cpu_info *ci)
if (msr != nmsr)
wrmsr(MSR_DE_CFG, nmsr);
}
- if (family == 0x17 && ci->ci_model >= 0x31) {
+ if (family == 0x17 && ci->ci_model >= 0x31 &&
+ (cpu_ecxfeature & CPUIDECX_HV) == 0) {
nmsr = msr = rdmsr(MSR_DE_CFG);
nmsr |= DE_CFG_SERIALIZE_9;
if (msr != nmsr)
Re: Zenbleed
I am not aware of a feature flag we can check for if we can
write to MSR_DE_CFG, and my guess is their VM needs to silently
ignore the bits we modify, and not generate a fault.
I'm not sure what we can do here, but I suspect some developers will
think about it.
Anyways, they decided to fault. That seems wrong. I think they will
get other complaints in the near future. I think you should let them
know. It is an easy fix on their side.
Lucas wrote:
> Hey,
>
> I'm running this on a Hetzner VPS, with the dmesg below. I ran syspatch
> followed by installboot -v sd0. After each boot, 100% I get
>
> cpu0 at mainbus0: apid 0 (boot processor)
> cpu0: AMD EPYC Processor, 2445.60 MHz, 17-31-00
> cpu0: FPU,... (proc capabilities)
> cpu0: 32KB ... (cache info)
> cpu0: smt 0, core 0, package 0
> kernel: protection fault trap, code=0
> Stopped at cpu_fix_msrs+0x1a4: wrmsr
> ddb{0}> show panic
> the kernel did not panic
> ddb{0}> show reg
> rdi 0x8243dff0cpu_info_full_primary+0x1ff0
> rsi0x2
> rbp 0x829277f0end+0x3277f0
> rbx 0x80043880
> rdx 0
> rcx 0xc0011029
> rax 0x202
> r8 0x101010101010101
> r9 0x8080808080808080
> r10 0xf74eb8da9a13c7be
> r11 0x4299783767f1ac05
> r12 0x800438a4
> r13 0x800020a11000
> r14 0x8243dff0cpu_info_full_primary+0x1ff0
> r15 0x17
> rip 0x811b50f4cpu_fix_msrs+0x1a4
> cs 0x8
> rflags 0x10202__ALIGN_SIZE+0xf202
> rsp 0x829277c0end+0x3277c0
> ss0x10
> cpu_fix_msrs+0x1a4: wrmsr
> ddb{0}> bt
> cpu_fix_msrs(...) at cpu_fix_msrs+0x1a4
> cpu_attach(...) at cpu_attach+0x3fd
> config_attach(...) at config_attach+0x1f4
> acpimadt_attach(...) at acpimadt_attach+0x349
> config_attach(...) at config_attach+0x1f4
> acpi_attach_common(...) at acpi_attach_common+0x5db
> config_attach(...) at config_attach+0x1f4
> bios_attach(...) at bios_attach+0x77e
> config_attach(...) at config_attach+0x1f4
> mainbus_attach(...) at mainbus_attach+0x778
> config_attach(...) at config_attach+0x1f4
> cpu_configure(...) at cpu_configure+0x33
> main(0,0,0,0,0,1) at main+0x3d3
> end trace fram: 0x0, count: 13
> ddb{0}> mach ddbcpu 1
> Invalid cpu 1
> ddb{0}>
>
>
> I've screenshots of the backtrace, should the function parameters be
> required.
>
> If other people are lucky like, remember that you can `boot /obsd` at
> the boot prompt to boot an older kernel.
>
> After booting the old kernel, I successfully ran fw_update and fetched
> the amd firmware package. Nevertheless, the problem persists. I tried
> rerunning `syspatch` and `installboot -v sd0` again, and the problem
> still persists. In case it's relevant, the installboot output is
>
> Using / as root
> installing bootstrap on /dev/rsd0c
> using first-stage /usr/mdec/biosboot, second-stage /usr/mdec/boot
> copying /usr/mdec/boot to //boot
> looking for superblock at 65536
> found valid ffs2 superblock
> //boot is 6 blocks x 16384 bytes
> fs block shift 2; part offset 64; inode block 56, offset 5232
> expecting 64-bit fs blocks (incr 4)
> master boot record (MBR) at sector 0
> partition 3: type 0xA6 offset 64 size 80003008
> /usr/mdec/biosboot will be written at sector 64
>
>
> OpenBSD 7.3 (GENERIC.MP) #0: Wed Jul 12 05:09:49 MDT 2023
>
> r...@syspatch-73-amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC.MP
> real mem = 2080227328 (1983MB)
> avail mem = 1997848576 (1905MB)
> random: good seed from bootblocks
> mpath0 at root
> scsibus0 at mpath0: 256 targets
> mainbus0 at root
> bios0 at mainbus0: SMBIOS rev. 2.8 @ 0xf59d0 (10 entries)
> bios0: vendor Hetzner version "2017" date 11/11/2017
> bios0: Hetzner vServer
> acpi0 at bios0: ACPI 3.0
> acpi0: sleep states S5
> acpi0: tables DSDT FACP APIC HPET MCFG WAET
> acpi0: wakeup devices
> acpitimer0 at acpi0: 3579545 Hz, 24 bits
> acpimadt0 at acpi0 addr 0xfee0: PC-AT compat
> cpu0 at mainbus0: apid 0 (boot processor)
> cpu0: AMD EPYC Processor, 2445.58 MHz, 17-31-00
> cpu0:
> FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,MMX,FXSR,SSE,SSE2,HTT,SSE3,PCLMUL,SSSE3,FMA3,CX16,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,AES,XSAVE,AVX,F16C,RDRAND,HV,NXE,MMXX,FFXSR,PAGE1GB,RDTSCP,LONG,LAHF,CMPLEG,AMCR8,ABM,SSE4A,MASSE,3DNOWP,OSVW,TOPEXT,CPCTR,FSGSBASE,BMI1,AVX2,SMEP,BMI2,RDSEED,ADX,SMAP,CLFLUSHOPT,CLWB,SHA,UMIP,IBPB,IBRS,STIBP,SSBD,XSAVEOPT,XSAVEC,XGETBV1,XSAVES
> cpu0: 32KB 64b/line 8-way D-cache, 32KB 64b/line 8-way I-cache, 512KB
> 64b/line 8-way L2 cache, 16MB 64b/line 16-way L3 cache
> cpu0: smt 0, core 0, package 0
> mtrr: Pentium Pro MTRR support, 8 var ranges, 88 fixed ranges
> cpu0: apic clock running at
Zenbleed
Zenbleed errata for 7.2 and 7.3 will come out soon. sysupgrade of the -current snapshot already contains a fix. I wanted to share some notes on impact: OpenBSD does not use the AVX instructions to the same extent that Linux and Microsoft do, so this is not as important. On Linux, glibc has AVX-based optimizations for simple functions (string and memory copies) which will store secrets into the register file which can be extracted trivially, so the impact on glibc-based systems is HUGE. While working on our fixes, I ran the test programs for quite a while and I never saw anything resembling a 'text' string. However when I ran a browser I saw streams of what was probably graphics-related fragments flowing past. The base system clearly uses AVX very rarely by itself. In summary: in OpenBSD, this isn't a big deal today. However, attacks built upon primitives always get better over time, so I urge everyone to install these workarounds as soon as our errata ship. -- ps. If you use syspatch for these new errata, you must install the bootblocks yourself! syspatch cannot install them for you. So you must run this yourself, before the last reboot: installboot -v sd0 or installboot -v wd0 Our cpu firmware update mechanism uses the bootblocks to load the firmware from disk and provides it to the kernel, so if you don't have new bootblocks you won't be protected.
Re: Stop using direct syscall(2) from perl(1)
Andrew Hewus Fresh wrote:
> One thing to note, on the advice of miod@, I adjusted the mapping of
> size_t from u_int to u_long, but that means we no longer recognize
> getlogin_r. When I asked, miod@ suggested that syscalls.master was
> confused.
>
> $ grep getlogin_r /usr/src/sys/kern/syscalls.master /usr/include/unistd.h
> /usr/src/sys/kern/syscalls.master:141 STD { int
> sys_getlogin_r(char *namebuf, u_int namelen); }
> /usr/include/unistd.h:intgetlogin_r(char *, size_t)
That's quite a surprise.
syscalls.master will need to be changed, which will change
struct sys_getlogin_r_args, which is used inside sys_getlogin_r() in
kern_prot.c. Don't get fooled by the advisory /* */ block you see
there, it is using the generated struct. But the offset to load the
field isn't changing, and luckily the incorrect field is smaller than
the correct field, so I *think* there is no major ABI crank needed.
Re: bgpd: adjust ctl_neighbor usage
Theo Buehler wrote: > On Thu, Jul 20, 2023 at 05:22:25PM +0200, Theo Buehler wrote: > > On Thu, Jul 20, 2023 at 05:06:00PM +0200, Claudio Jeker wrote: > > > I think it is better to use a safe ideom when matching against a peer name > > > instead of forcefully NUL terminate the string somewhere unrelated. > > > By default all these string buffers use the same size so strncmp() will > > > not clip since the peer description is enforced by bgpd to be smaller. > > > > > > Another option would be to move > > > neighbor->descr[PEER_DESCR_LEN - 1] = 0; > > > into the match functions. At least then it is certainly done. > > > > I prefer strncpy(). So this diff is ok. > > Ugh. strncmp... Yes, that's fine :)
Re: bgpd: adjust ctl_neighbor usage
Theo Buehler wrote: > On Thu, Jul 20, 2023 at 05:06:00PM +0200, Claudio Jeker wrote: > > I think it is better to use a safe ideom when matching against a peer name > > instead of forcefully NUL terminate the string somewhere unrelated. > > By default all these string buffers use the same size so strncmp() will > > not clip since the peer description is enforced by bgpd to be smaller. > > > > Another option would be to move > > neighbor->descr[PEER_DESCR_LEN - 1] = 0; > > into the match functions. At least then it is certainly done. > > I prefer strncpy(). So this diff is ok. The problem with strncpy() is increasing number of people eyeing it with prejudice and therefore a temptation to "fix it", and move the NUL. It could happen in the future, I would entertain making 20 year bets...
Re: Stop using direct syscall(2) from perl(1)
-my $sb = "\0\0\0\0"; +my $sb = "\0" x 4096; That's pretty terrible. Does this language not have types?
Re: [PATCH] When appropriate, replace the waitpid calls with wait in sbin/init/init.c
rhl120 wrote: > Hello, this commit fixes nothing. To me it is just an instance of using > the right tool for the right job. Sorry, you are incorrect. > I could also argue that there is a > performance improvement (because the call passes less arguments) but > that is probably negligible. That is not correct. The kernel only has a wait4() system call. waitpid() and wait() are both wrappers on top of it. It is half style. The other half is that this program is intentionally written at a lower level of calls, so your proposal is not right.
Re: [PATCH] When appropriate, replace the waitpid calls with wait in sbin/init/init.c
What are you fixing by making this less precise?
rhl120 wrote:
> Hello, while browsing the source code of init, I found a couple of calls to
> waitpid which, I believe, could be replaced with wait(NULL). As far as I can
> tell lib/libc/gen/wait.c and lib/libc/gen/waitpid.c backup my belief but on
> the other hand I am very new to this stuff so I may be wrong so sorry if this
> is a waste of your time. The FAQ says that I should send the patch inline but
> the mailing lists page says that I can send it as an attachment so I did both.
> Thanks for checking out my commit!
> Here is the patch:
>
> diff --git a/sbin/init/init.c b/sbin/init/init.c
> index cf7ed60afe9..1456f9508f7 100644
> --- a/sbin/init/init.c
> +++ b/sbin/init/init.c
> @@ -1176,7 +1176,7 @@ f_multi_user(void)
> }
>
> while (!requested_transition)
> - if ((pid = waitpid(-1, NULL, 0)) != -1)
> + if ((pid = wait(NULL)) != -1)
> collect_child(pid);
>
> return requested_transition;
> @@ -1360,7 +1360,7 @@ f_nice_death(void)
> clang = 0;
> alarm(DEATH_WATCH);
> do {
> - if ((pid = waitpid(-1, NULL, 0)) != -1)
> + if ((pid = wait(NULL)) != -1)
> collect_child(pid);
> } while (clang == 0 && errno != ECHILD);
>
> @@ -1404,7 +1404,7 @@ f_death(void)
> clang = 0;
> alarm(DEATH_WATCH);
> do {
> - if ((pid = waitpid(-1, NULL, 0)) != -1)
> + if ((pid = wait(NULL)) != -1)
> collect_child(pid);
> } while (clang == 0 && errno != ECHILD);
>
Re: GPROF: sleep_state: disable _mcount() across suspend/resume
> ok kettenis@ ok deraadt also
Re: GPROF: sleep_state: disable _mcount() across suspend/resume
Mark Kettenis wrote: > It is still a bit scary to have cpu_hatch() call _mcount() but I guess > adding __attribute__((no_profile)) to all of the functions called by > cpu_hatch() isn't really workable either. It now immediately returns. > > + int gmon_state = gmoninit; > > No variable declarations in the middle of functions please. Yep. Put it at the top. > > + gmoninit = 0; > > + membar_producer(); > > Why are you messing with memory barriers here? I have asked the same question. This has potential to make things worse.
Re: GPROF: sleep_state: disable _mcount() across suspend/resume
I dare you to write the simplest fix for this, instead of a diff that scrolls by.
Re: GPROF: sleep_state: disable _mcount() across suspend/resume
Scott Cheloha wrote: > Secondary CPUs are still running at the top of sleep_state(). To > disable _mcount with gmoninit we would need to wait until after > secondary CPUs have halted to toggle it off, which is way further into > sleep_state(). I suspect you are exaggerating the window of time when _mcount is dangerous to call. Disable it early, re-enable it late, and avoid trying to write a complicated solution.
Re: GPROF: sleep_state: disable _mcount() across suspend/resume
Mark Kettenis wrote: > So isn't the real problem that some of the lower-level code involved > in the resume path isn't properly marked to not do the > instrumentation? Traditionally that was assembly code and we'd use > NENTRY() (in amd64) or ENTRY_NP() (on some other architectures) to > prevent thise functions from calling _mcount(). But that was only > ever done for code used during early bootstrap of the kernel. And > these days there may be C code that needs this as well. > > With your diff, functions in the suspend/resume path will still call > _mcount() which may not be safe. I guess you can make critical functions not do _PROF_PROLOGUE or you can make __mcount or _mcount aware that they should "do nothing", or "nothing scary". Hell, save & toggle the 'gmoninit' variable during the suspend/resume sequence, and then adjust one comment: /* * Do not profile execution if memory for the current CPU * descriptor and profiling buffers has not yet been allocated * or if the CPU we are running on has not yet set its trap -* handler +* handler, or disabled during a suspend/resume sequence */ if (gmoninit == 0) return; Does this really need another variable? It feels like this can be 4 1-line diffs.
Re: m2: add suspend keyboard shortcut
Tobias Heider wrote: > +#ifdef SUSPEND > + if (ksym == KS_Cmd_Sleep) > + return request_sleep(SLEEP_SUSPEND); > +#endif Oh wait, it is consumed. Is that a problem
Re: m2: add suspend keyboard shortcut
Tobias Heider wrote: > +#ifdef SUSPEND > + if (ksym == KS_Cmd_Sleep) > + return request_sleep(SLEEP_SUSPEND); > +#endif This key is not absorbed when it performs this action. Is that OK?
Re: OpenBSD perl 5.36.1 - Call for Testing
Alexander Bluhm wrote: > After that we have only the syscall Perl diff floating around and > can concentrate on that. It is important to make some sort of progress on that before the release cycle, because immediately after release I want to start burning syscall down.
Re: acpi: move acpiioctl to x86
Sure.
Tobias Heider wrote:
> I am planning to restructure the APM/sleep APIs to make it easier to suspend
> from more places like as a suspend keyboard shortcut.
>
> The acpiioctl handler is x86 specific code which is currently built on all
> platforms but only hooked up on i386 and amd64. It is also in the way of
> my plans, so I'd prefer if we move it to acpi_x86.c where all the other
> x86-only acpi code lives.
>
> ok?
>
> Index: dev/acpi//acpi.c
> ===
> RCS file: /mount/openbsd/cvs/src/sys/dev/acpi/acpi.c,v
> retrieving revision 1.421
> diff -u -p -r1.421 acpi.c
> --- dev/acpi//acpi.c 29 Jun 2023 20:58:08 - 1.421
> +++ dev/acpi//acpi.c 5 Jul 2023 13:37:18 -
> @@ -3439,58 +3439,6 @@ acpiclose(dev_t dev, int flag, int mode,
> return (error);
> }
>
> -int
> -acpiioctl(dev_t dev, u_long cmd, caddr_t data, int flag, struct proc *p)
> -{
> - int error = 0;
> - struct acpi_softc *sc;
> - struct apm_power_info *pi = (struct apm_power_info *)data;
> - int s;
> -
> - if (!acpi_cd.cd_ndevs || APMUNIT(dev) != 0 ||
> - !(sc = acpi_cd.cd_devs[APMUNIT(dev)]))
> - return (ENXIO);
> -
> - s = splbio();
> - /* fake APM */
> - switch (cmd) {
> - case APM_IOC_SUSPEND:
> - case APM_IOC_STANDBY:
> - if ((flag & FWRITE) == 0) {
> - error = EBADF;
> - break;
> - }
> - acpi_addtask(sc, acpi_sleep_task, sc, SLEEP_SUSPEND);
> - acpi_wakeup(sc);
> - break;
> -#ifdef HIBERNATE
> - case APM_IOC_HIBERNATE:
> - if ((error = suser(p)) != 0)
> - break;
> - if ((flag & FWRITE) == 0) {
> - error = EBADF;
> - break;
> - }
> - if (get_hibernate_io_function(swdevt[0].sw_dev) == NULL) {
> - error = EOPNOTSUPP;
> - break;
> - }
> - acpi_addtask(sc, acpi_sleep_task, sc, SLEEP_HIBERNATE);
> - acpi_wakeup(sc);
> - break;
> -#endif
> - case APM_IOC_GETPOWER:
> - error = acpi_apminfo(pi);
> - break;
> -
> - default:
> - error = ENOTTY;
> - }
> -
> - splx(s);
> - return (error);
> -}
> -
> void acpi_filtdetach(struct knote *);
> int acpi_filtread(struct knote *, long);
>
> @@ -3571,12 +3519,6 @@ acpiopen(dev_t dev, int flag, int mode,
>
> int
> acpiclose(dev_t dev, int flag, int mode, struct proc *p)
> -{
> - return (ENXIO);
> -}
> -
> -int
> -acpiioctl(dev_t dev, u_long cmd, caddr_t data, int flag, struct proc *p)
> {
> return (ENXIO);
> }
> Index: dev/acpi//acpi_x86.c
> ===
> RCS file: /mount/openbsd/cvs/src/sys/dev/acpi/acpi_x86.c,v
> retrieving revision 1.15
> diff -u -p -r1.15 acpi_x86.c
> --- dev/acpi//acpi_x86.c 6 Mar 2022 15:12:00 - 1.15
> +++ dev/acpi//acpi_x86.c 5 Jul 2023 14:33:40 -
> @@ -17,15 +17,86 @@
> */
>
> #include
> +#include
> #include
> #include
>
> +#ifdef HIBERNATE
> +#include
> +#endif
> +
> +#include
> +#include
> +
> #include
> #include
> #include
> #include
>
> #include
> +#define APMUNIT(dev) (minor(dev)&0xf0)
> +
> +#ifndef SMALL_KERNEL
> +extern struct cfdriver acpi_cd;
> +
> +int
> +acpiioctl(dev_t dev, u_long cmd, caddr_t data, int flag, struct proc *p)
> +{
> + int error = 0;
> + struct acpi_softc *sc;
> + struct apm_power_info *pi = (struct apm_power_info *)data;
> + int s;
> +
> + if (!acpi_cd.cd_ndevs || APMUNIT(dev) != 0 ||
> + !(sc = acpi_cd.cd_devs[APMUNIT(dev)]))
> + return (ENXIO);
> +
> + s = splbio();
> + /* fake APM */
> + switch (cmd) {
> + case APM_IOC_SUSPEND:
> + case APM_IOC_STANDBY:
> + if ((flag & FWRITE) == 0) {
> + error = EBADF;
> + break;
> + }
> + acpi_addtask(sc, acpi_sleep_task, sc, SLEEP_SUSPEND);
> + acpi_wakeup(sc);
> + break;
> +#ifdef HIBERNATE
> + case APM_IOC_HIBERNATE:
> + if ((error = suser(p)) != 0)
> + break;
> + if ((flag & FWRITE) == 0) {
> + error = EBADF;
> + break;
> + }
> + if (get_hibernate_io_function(swdevt[0].sw_dev) == NULL) {
> + error = EOPNOTSUPP;
> + break;
> + }
> + acpi_addtask(sc, acpi_sleep_task, sc, SLEEP_HIBERNATE);
> + acpi_wakeup(sc);
> + break;
> +#endif
> + case APM_IOC_GETPOWER:
> + error = acpi_apminfo(pi);
> + break;
> +
> + default:
> + error = ENOTTY;
> + }
> +
> + splx(s);
> + return (error);
> +}
>
Re: pf.os database /p0f
Lee, Jonathan D wrote: > Hello, the empty section yes, I agree would still need to be populated. > Thanks for adding some fresh visibility to this problem as I noticed OpenBSD > has p0f as well as FreeBSD the FreeBSD is being used as an example with > PfSense. > > The p0f database is starting to show its age. That sentence is the key. If it is old, you can stop using it. Or, you can be a partner in fixing it, and validating the changes, entirely. Not in a small way, but completely ensuring that what you propose has no downside. You in? > I am just researching a way to compartmentalize containers because their > abilities to perform data marshaling over the host's NIC. Furthermore, there > is many different vendors of containers from bsdJAILs to Kerbenets and many > others. My goal for the original email is just visibility for the need to > develop software or improve the older fingerprinting software that way it can > fingerprint and detect container signatures. The concrete example of Kali's > bleeding edge docker container is shown for more understanding on the > movement of this sector. Unfortunately noone else is willing to invest the time to rebuild the entire system (in an incredibly careful way) for you.
Re: csh(1), ksh(1), time(1): print durations with millisecond precision
Todd C. Miller wrote: > Other implementations of "time -p" (both builtin and standalone) > only display two digits after the radix point. I'm a little concerned > about breaking scripts that consume out the output of "time -p". I share that concern.
Re: profclock, gmonclock: new callbacks for profil(2)/GPROF statclock() code
Claudio Jeker wrote: > On Mon, Jun 19, 2023 at 06:41:14PM -0500, Scott Cheloha wrote: > > > On Jun 19, 2023, at 18:07, Theo de Raadt wrote: > > > > > > Make sure to STOP all kernel profiling before attempting to > > >suspend or hibernate your machine. Otherwise I expect it > > >will hang. > > > > > > It is completely acceptable if it produces wrong results, but it must > > > not hang the system. > > > > The hang is present in -current, with or > > without this patch. > > > > I am working to figure it out. > > I don't think the suspend or hibernate code has any code to disable > kernel profiling. This is bad since these code paths are extremly > sensitive and try not to do side-effects. > > So in the suspend/hibernate code path we should disable profiling early > on. It makes no sense to try to run gprof collection in those code paths. Yes, that's right. It will be somewhere in kern/subr_suspend.c Be careful that the "stop profiling" and "restart profiling" are at the correct places. The sleep_state() function has a bunch of unrolling goto's which are not 100% reflexive, so be careful.
Re: profclock, gmonclock: new callbacks for profil(2)/GPROF statclock() code
Make sure to STOP all kernel profiling before attempting to suspend or hibernate your machine. Otherwise I expect it will hang. It is completely acceptable if it produces wrong results, but it must not hang the system.
Re: profclock, gmonclock: new callbacks for profil(2)/GPROF statclock() code
Make sure to STOP all kernel profiling before attempting to suspend or hibernate your machine. Otherwise I expect it will hang. That is not acceptable. People suspend and hibernate machines without being aware of what applications are doing. GPROF is a kernel compile-time option. If you don't enable it, you have nothing to worry about. Well that's a great hidden reason why noone would ever turn on this subsystem -- so why is it getting done on it??
Re: reorder libssl and libtls at boot?
Relinking's goal is to reduce gadget discovery.
There are two reasons we do this:
- The existance of many small stub functions that might be reached with
the wrong parameters to act upon data structures incorrectly
- polymorphic gadget availability on variable-sized instruction architectures
Random relinking makes the offsets to those things less discoverable.
But these two librares you propose are than the others we relink, and
have far fewer gadgets to begin with.
I don't see a positive value:cost tradeoff here, where cost is "time during
boot, and potential for fragility in case of relink failure".
So these libraries are simply not in the same scope as libcrypto, libc,
sshd, and the kernel. For those pieces, the boot-time investment is
worth it. I only really consider one more program worth investing in
relinking: httpd, but haven't done so yet.
Job Snijders wrote:
> Hi all,
>
> Would it be worth it to reorder libssl & libtls at boot?
>
> Kind regards,
>
> Job
>
> Index: etc/rc
> ===
> RCS file: /cvs/src/etc/rc,v
> retrieving revision 1.571
> diff -u -p -r1.571 rc
> --- etc/rc26 Apr 2023 14:28:09 - 1.571
> +++ etc/rc17 Jun 2023 05:18:46 -
> @@ -199,7 +199,7 @@ reorder_libs() {
> done
>
> # Only choose the latest version of the libraries.
> - for _liba in $_relink/usr/lib/lib{c,crypto}; do
> + for _liba in $_relink/usr/lib/lib{c,crypto,ssl,tls}; do
> _libas="$_libas $(ls $_liba.so.+([0-9.]).a | sort -rV | head
> -1)"
> done
>
> Index: lib/libssl/Makefile
> ===
> RCS file: /cvs/src/lib/libssl/Makefile,v
> retrieving revision 1.79
> diff -u -p -r1.79 Makefile
> --- lib/libssl/Makefile 5 May 2023 21:23:02 - 1.79
> +++ lib/libssl/Makefile 17 Jun 2023 05:18:46 -
> @@ -10,6 +10,7 @@ PC_FILES=openssl.pc libssl.pc
> CLEANFILES=${PC_FILES} ${VERSION_SCRIPT}
>
> LIB= ssl
> +LIBREBUILD=y
>
> CFLAGS+= -Wall -Wundef
> .if ${COMPILER_VERSION:L} == "clang"
> Index: lib/libtls/Makefile
> ===
> RCS file: /cvs/src/lib/libtls/Makefile,v
> retrieving revision 1.38
> diff -u -p -r1.38 Makefile
> --- lib/libtls/Makefile 5 May 2023 21:23:02 - 1.38
> +++ lib/libtls/Makefile 17 Jun 2023 05:18:46 -
> @@ -16,6 +16,7 @@ CLEANFILES= ${VERSION_SCRIPT}
> WARNINGS= Yes
>
> LIB= tls
> +LIBREBUILD=y
>
> DPADD= ${LIBCRYPTO} ${LIBSSL}
>
> Index: distrib/sets/lists/base/mi
> ===
> RCS file: /cvs/src/distrib/sets/lists/base/mi,v
> retrieving revision 1.1099
> diff -u -p -r1.1099 mi
> --- distrib/sets/lists/base/mi10 Jun 2023 15:16:43 - 1.1099
> +++ distrib/sets/lists/base/mi17 Jun 2023 05:18:50 -
> @@ -3009,6 +3009,8 @@
> ./usr/share/relink/usr/lib
> ./usr/share/relink/usr/lib/libc.so.97.0.a
> ./usr/share/relink/usr/lib/libcrypto.so.51.0.a
> +./usr/share/relink/usr/lib/libssl.so.54.0
> +./usr/share/relink/usr/lib/libtls.so.27.0
> ./usr/share/relink/usr/libexec
> ./usr/share/relink/usr/libexec/ld.so.a
> ./usr/share/relink/usr/sbin
>
Re: First step towards improved unlocking in the VFS layer.
Thordur I. Bjornsson wrote: > On Mon, Jun 12, 2023 at 9:15 PM Bob Beck wrote: > > > > On Mon, Jun 12, 2023 at 11:01:18AM -0600, Theo de Raadt wrote: > > > + KASSERTMSG(1, "Ich Habe eine Rotweinflarsche in meinem Arsche"); > > > That part of the diff is not OK. If everyone did this, we would have a > > > mess on our hands. > > (or I could simply deleted it) > +1 > > Is it on purpose that this is completely silent ? > Perhaps the warning in ffs_vfsops should go "WARNING: soft updates are > now ignored" and the option dropped from GENERIC ? The purpose of Bob's diff is to silently (quietly) simply disable softdep mount requests. They will be downgraded to regular mounts. But then we want to make sure that the back-end code isn't accidentaly called, because of some bug, that's where his whiny assert comes into play. The reason to disable softdep, is that softdep has crazy callback schemes and context issues that are making it hard to reason about vfs locking. If we disable softdep, we may be able to unlock nami / vfs / etc better. When / if such locking/scheduling changes are finished, softdep can be repaired.
Re: First step towards improved unlocking in the VFS layer.
+ KASSERTMSG(1, "Ich Habe eine Rotweinflarsche in meinem Arsche"); That part of the diff is not OK. If everyone did this, we would have a mess on our hands.
Re: acme-client(8): preliminary support for HiCA
Todd C. Miller wrote: > On Fri, 09 Jun 2023 07:25:04 +0200, Florian Obser wrote: > > > OK? > > > > p.s. I'm currently busy writing an ISC licensed bash in rust to safely > > support HiCA. So this might take a while... > > Have you considered implementing wordexp(3) to allow command > substitution? It may be necessary to add inline support for IFS > to fully support your use case. > > Also, for full compatibility I think it would be better to choose > a User-Agent similar to: > > Mozilla/5.0 (OpenBSD 7.3; acme-client; x64) AppleWebKit/537.36 (KHTML, > like Gecko) Chrome/104.0.5112.34 Safari/537.36 > > obviously you need to substitute in the OpenBSD version and architecture. With other change being proposed, it should even be possible and avantageous for HiCA to use SFTP inside their embedded curl sub-operation, including control access using keys in the user's .ssh directory.
Re: autopledge
g...@oat.com wrote: > Theo de Raadt wrote: > After pledge, 80% of the base programs were converted to pledge-assisted > priv-drop, because it was really obvious that "initialization code" > could > and should be moved earlier in the program, so that pledge (or multiple > pledge calls dropping permissions further) could be added to the > program. > > Inside the group, we called this moving of initialization code to > earlier "hoisting". > > Hoisting and cleanup can have very large benefits independent of implementing > pledge > or other security features. > I have seen programs shrink by almost 90% and gain functionality as a result. > > In one case it -was- a program which ran with privileges equivalent to root. > As a byproduct of the cleanup we were later able to assure ourselves that the > result > needed no more changes to be as secure as we could make it. It is a big mental shift. If you don't attempt & perform the hoisting action once, yourself to a real pre-existing program, then you will never understand pledge and have mysterious beliefs about how it is 1-line secret sauce that makes programs safer. On the contrary, it is 90% guidance and 10% enforcement.
Re: autopledge
William Ahern wrote: > Rather, the point of pledge and unveil is to make that > deliberate refactoring as pleasant and minimal as is practicable. Indeed, after the first 10 programs were converted to use pledge, it became very obvious what would happen next: "priv-drop everything" The first priv-drop program in OpenBSD was ping, that was well before pledge. After pledge, 80% of the base programs were converted to pledge-assisted priv-drop, because it was really obvious that "initialization code" could and should be moved earlier in the program, so that pledge (or multiple pledge calls dropping permissions further) could be added to the program. Inside the group, we called this moving of initialization code to earlier "hoisting". Moving the initialization code upwards is the hard part. There is no need to decide on what the pledge should be, because the movement goal is decided by the semantic seperation of the pledge promises features. We didn't add "minimal pledge" to each program. We aren't stupid like that. Instead, pledge was a tool to refactor all the programs and separate them into initialization + main loop. Go look at nc/netcat.c. Automated software could conceivably create that outcome?? That's laughable. The proposal is blind to how pledge gets used.
Re: autopledge
We will wait for the demo.
Leah Rowe wrote:
> Hi Theo,
>
> On Fri, 02 Jun 2023 11:03:40 -0600
> "Theo de Raadt" wrote:
>
> > Additionally the two outcomes of this will be:
> >
> > 1. Don't call pledge in the program.
> >
> > 2. Use pledge("audio bpf chown cpath disklabel dns dpath drm error
> > exec fattr flock getpw id inet mcast pf proc prot_exec ps recvfd
> > route rpath sendfd settime stdio tape tmppath tty unix unveil video
> > vminfo vmm wpath wroute", NULL);
>
> Yeah I was kinda thinking, just have it be a tool to *assist* but not
> to automatically pledge the program itself. It wouldn't replace
> human-performed auditing or analysis.
>
> You'd run it just to get a basic gist of where you're going, for
> different code paths (which are affected by how you use the program).
>
> If you can trace from specific points in code it's more useful. So
> you'd run different tests depending on the program. It wouldn't
> substitute simply reading and understanding (possibly re-writing) parts
> of the code in a program, to pledge it.
>
> For really huge codebases it might be useful. For smaller code it
> wouldn't be as useful (can more easily just read all of the code).
>
> > We should write a program that looks at all conflict and finds a
> > simple solution for world peace.
>
> The point is well taken :)
>
> --
> Leah Rowe,
> Company Director,
> Minifree Ltd
>
> Registered in England, registration No. 9361826
> VAT Registration No. GB202190462
> Minifree Ltd, 19 Hilton Road, Canvey Island
> Essex SS8 9QA, United Kingdom
> United Kingdom
Re: autopledge
Leah Rowe wrote:
> Hi everyone,
>
> I had an interesting idea for OpenBSD. Haven't tried it yet. I'm
> wondering what other people think of it? The idea is, thus:
>
> 1) Do execution tracing and just run a program. Do everything possible
> in it to the fullest extent feasible and get an entire log of the
> trace. OpenBSD can do tracing:
>
> https://man.openbsd.org/dt
>
> https://man.openbsd.org/btrace
>
> https://blog.lambda.cx/posts/openbsd-dynamic-tracing/
>
> 2) Write a program that scans for all system calls in the trace,
> suggesting what pledge promises to use. See:
>
> https://man.openbsd.org/pledge.2
>
> I call this idea "autopledge".
Additionally the two outcomes of this will be:
1. Don't call pledge in the program.
2. Use pledge("audio bpf chown cpath disklabel dns dpath drm error exec fattr
flock getpw id inet mcast pf proc prot_exec ps recvfd route rpath sendfd
settime stdio tape tmppath tty unix unveil video vminfo vmm wpath wroute",
NULL);
And 1 and 2 are different, subtle but important.
We should write a program that looks at all conflict and finds a simple
solution for world peace.
Re: autopledge
How do you ensure you have coverage of all the operational choices the program makes? How about we what you propose and remove all the bugs and then we don't need pledge? Anyone who has done a 3nd year computer science course knows why this does not work. Leah Rowe wrote: > > > Hi everyone, > > I had an interesting idea for OpenBSD. Haven't tried it yet. I'm > wondering what other people think of it? The idea is, thus: > > 1) Do execution tracing and just run a program. Do everything possible > in it to the fullest extent feasible and get an entire log of the > trace. OpenBSD can do tracing: > > https://man.openbsd.org/dt > > https://man.openbsd.org/btrace > > https://blog.lambda.cx/posts/openbsd-dynamic-tracing/ > > 2) Write a program that scans for all system calls in the trace, > suggesting what pledge promises to use. See: > > https://man.openbsd.org/pledge.2 > > I call this idea "autopledge". > > PS: > > I initially proposed this on IRC, but I was told that the IRC channel > is mostly for user support, so I thought it best to discuss here. > > -- > Leah Rowe
Re: install.sub: two little ergonomic tweaks
Alexander Klimov wrote: > Wait a second! > (Yes, one of my biggest talents is to "oversee elephants"TM, but not this > time.) > E.g. I use the (C)ustom layout which *clearly indicates* its one-(C)har > shortcut. > Other prompts, like my diff(1)ed one, *do not*. > > Change my mind. > > Now, given that I've not overseen yet another elephant right now, > if I as a user see some "(C)ustom layout" stuff on the one hand > and some "yes"/"autoconf" stuff on the other hand, > how should I even think of the latter being shortcuttable? > apt(8) and IIRC yum(8) at least write e.g. "[Y/n]" > indicating something (the default in this case) by an upper-case. > > Just let me know in case you change your mind. > I.e. if you'd accept a s/yes/(y)es/ etc. patch from me. No.
Re: install.sub: two little ergonomic tweaks
Alexander Klimov wrote:
> First of all, my compliment.
> The installer is already quite ergonomic (for a CLI ;) ).
> But there are the following two little diff(1)s standing
> between it and its perfection IMAO.
>
>
> --- distrib/miniroot/install.sub.orig Thu May 18 12:37:52 2023
> +++ distrib/miniroot/install.subThu May 18 12:44:49 2023
> @@ -1220,3 +1220,3 @@
> ask_until "IPv6 address for $_if? (or 'autoconf' or 'none')" \
> - "${_addr:-none}"
> + "${_addr:-autoconf}"
> case $resp in
>
> I personally enable IPv6 everywhere,
> even if I have only link-local addresses.
> If I got SLAAC, nice for my OpenBSD clients
> and the clients of my OpenBSD servers.
> Win-win. If not, I haven't lost anything.
> In the worst case I have to do specific config,
> but then the default doesn't matter anyway.
>
> The only reason against this could be a permit-default pf.conf.
> But such shouldn't be done and this is the installer after all.
> One writes pf.conf after the installer or can -in extreme case-
> still type "none" here (which is shorter to type).
> I know that you folks like not to surprise users.
> But IMAO default-enabling IPv6 *on new installs* isn't a surprise
> (in 2023 when IIRC some US gov orgs already sell their whole IPv4s).
I disagree. I believe network participation should be opt-in. Minimal
network configuration is encouraged, but the minimum is v4, not v6.
v6 is still not natural. It is a surprise. I think it will be too easy
for users to turn it on in the wrong places, and since OpenBSD
automatically learns DNS configuration from such v6 network configuration
edges, this is a hazard. Configurating v6 must be intentional.
> In case you don't agree with me:
> What about a shortcut "a" (= autoconf)
> for IPv[46] address (like below)?
That would make sense. It would also need to be done for v4. Diff would
be something like this, not tested, there might be other pieces missing to
do it perfectly.
Index: install.sub
===
RCS file: /cvs/src/distrib/miniroot/install.sub,v
retrieving revision 1.1244
diff -u -p -u -r1.1244 install.sub
--- install.sub 2 May 2023 15:55:58 - 1.1244
+++ install.sub 18 May 2023 14:01:01 -
@@ -1105,7 +1105,7 @@ v4_config() {
case $resp in
none) return
;;
- autoconf|dhcp)
+ a|autoconf|dhcp)
dhcp_request $_if
echo "inet autoconf" >>$_hn
return
@@ -1222,7 +1222,7 @@ v6_config() {
case $resp in
none) return
;;
- autoconf)
+ a|autoconf)
ifconfig $_if inet6 autoconf up
echo "inet6 autoconf" >>$_hn
return
Re: install.sub: two little ergonomic tweaks
Alexander Klimov wrote: > --- distrib/miniroot/install.sub.orig Thu May 18 12:37:52 2023 > +++ distrib/miniroot/install.subThu May 18 12:44:49 2023 > @@ -2306,15 +2306,15 @@ > [[ $START_SSHD == y ]] || return > > if [[ -z $ADMIN ]]; then > echo "Since no user was setup, root logins via sshd(8) might > be useful." > fi > echo "WARNING: root is targeted by password guessing attacks, pubkeys > are safer." > while :; do > - ask "Allow root ssh login? (yes, no, prohibit-password)" no > + ask "Allow root ssh login? (yes, no, (p)rohibit-password)" no > _resp=$resp > case $_resp in > y|yes) SSHD_ENABLEROOT=yes > ;; > n|no) SSHD_ENABLEROOT=no > ;; > w|p|without-password|prohibit-password) > > Originally I wanted to do the same thing as above here. > I.e. to change the default no -> prohibit-password > which isn't less secure IMAO until you explicitly set auth. keys. > But then I've discovered the "p" shortcut (I'm showing you via diff(1) -U7). > IMAO showing it as I patched wouldn't harm anyone. This should not be neccessary. Almost all the prompts in the installer allow shortcut answers, as long as it is unambigious. Users can assume this, and learn it quickly. Therefore we do NOT chang all the "yes" to "(y)es". I'd be shocked if you haven't discovered this on your own. What would be interesting, is to hear of a bug where the case statements are not handling a short form. You'll see in the code above that we match "y|yes", we do not match "ye". That is also intentional. But if someone forgets to add a "n", that would be bad, and we would want to fix it.
Re: Small change in sysupgrade for custom release and test
No.
First of all, because there is no justification.
Secondly, because it is not documented.
But thirdly, because we keep shit simple so that people don't build
their own stuff on top of our infrastructure, so that if we feel the
need to change/break our own infrastructure we don't need to give a shit
about anyone doing weird stuff on their own.
So the short answer would be no, because what you propose does not
serve the greater userbase in any way. It only serves you, apparently.
Sven F. wrote:
> Bienvenue,
>
> --- /usr/sbin/sysupgrade.oldTue May 16 18:53:13 2023
> +++ /usr/sbin/sysupgradeTue May 16 19:04:46 2023
> @@ -143,6 +143,7 @@
> case ${_LINE} in
> *\ ${_KEY})SIGNIFY_KEY=/etc/signify/${_KEY} ;;
> *\ ${_NEXTKEY})SIGNIFY_KEY=/etc/signify/${_NEXTKEY} ;;
> +*\ *.pub) SIGNIFY_KEY=/etc/signify/${_LINE##* } && echo Using custom
> key $SIGNIFY_KEY ;;
> *) err "invalid signing key" ;;
> esac
>
> Read the signing key in the file so you can use a custom key when
> testing release,
>
> Have a good one.
>
Re: Status of Virtual Function driver for Intel 82599 series port?
Yuichiro NAITO wrote: > 2. MTU 9000 is required for 10Gbps performance. > > The default MTU size 1500 is too small for 10Gbps link for now. It is dangerous to give this suggestion without caveats. MTU over 1500 does not work on even small parts of the internet, and many people will have a difficult time diagnosing the failure modes. Sadly, it can only be used on point to point configurations.
Re: smtpd: nits to reduce the diff with -portable
Omar Polo wrote: > On 2023/05/10 09:30:12 +0200, Theo Buehler wrote: > > > I forgot to include one off_t cast since it was in a different > > > directory and -even if off topic because it's not in portable- instead > > > of "const"-ify only tz why don't mark as const also the two arrays day > > > and month? > > > > ok. > > > > The previous diff used (long long int) and this one now uses (long long). > > Would be nice to be consistent. > > Yes, indeed. smtpd uses `long long int', while for mail.local doesn't > have any. I'll go with `long long int' for consistency, typed `long > long' out of muscular memory. I think it is wrong for smtpd to use "long long int". It is pointless silliness, and there is more value in being idiomatically identical with the greater body of code.
Re: smtpd: nits to reduce the diff with -portable
> + (long long int)tv.tv_sec, tv.tv_usec, Please do not use that form. (long long) is enough.
Re: [patch] Avoid change of permissions in /etc/resolv.conf
I am still not seeing any reason to change how OpenBSD works, since noone else has ever been affected by this behaviour. Juan Picca wrote: > Hi Stuart > > > I'd suggest targetting the umask setting, either by giving all users > > class 'staff' or adding a new one which inherits from default. > > Thanks for your explanations. > > > This is a sensitive file. Keep a root shell open when modifying and > > don't close it until tested, there are various ways to break the format. > > Yes, I will try to remember it when I play again to be a sysadmin ;) > > > > Do you accept patches to avoid the interpretation of the last \ > > > (backslash) as a line continuation in a comment? > > > > I would object to such a diff. If somebody has written a file like that > > on purpose, that will break their machine on upgrade. > > I agree with you. > > Regards, > JMPC >
Re: [patch] Avoid change of permissions in /etc/resolv.conf
Well, now you get to own the consequences of your change, which is wrong. You pointed a gun at your own foot. Have you noticed that noone else has holes in their feet? Juan Picca wrote: > > I'm saying you will find this "problem" in 100 places, because the real > > problem is your own change. > > Yes, you are right. > The change that gives the error correctly infered by you and Stuart: > > --- /etc/login.conf.orig > +++ /etc/login.conf > @@ -40,7 +40,7 @@ > # > default:\ > :path=/usr/bin /bin /usr/sbin /sbin /usr/X11R6/bin /usr/local/bin > /usr/local/sbin:\ > - :umask=022:\ > + :umask=027:\ > :datasize-max=1024M:\ > :datasize-cur=1024M:\ > :maxproc-max=256:\ > > > Currently I'm using: > > --- /etc/login.conf.orig > +++ /etc/login.conf > @@ -70,6 +70,7 @@ > # Staff have fewer restrictions and can login even when nologins are set. > # > staff:\ > + :umask=027:\ > :datasize-cur=1536M:\ > :datasize-max=infinity:\ > :maxproc-max=512:\ > > > But maybe a less surprise config for /etc/login.conf can be: > > --- /etc/login.conf.orig > +++ /etc/login.conf > @@ -58,6 +58,7 @@ > # Be sure to reset these values to system defaults in the default class! > # > daemon:\ > + :umask=022:\ > :ignorenologin:\ > :datasize=4096M:\ > :maxproc=infinity:\ > > > With this umask from the default class can change without affecting the > daemon class. > Do the usage of openfiles-max currently follows the same idea? > > > Funny fact: by mistake I do > > > --- /etc/login.conf.orig > +++ /etc/login.conf > @@ -57,6 +57,7 @@ > # This must be set properly for daemons started as root by inetd as well. > # Be sure to reset these values to system defaults in the default class! > # > +#:umask=022:\ > daemon:\ > :ignorenologin:\ > :datasize=4096M:\ > > > And after that I couldn't use doas anymore to correct the file > > $ doas -s > doas: failed to set user context for target > > > Do you accept patches to avoid the interpretation of the last \ > (backslash) as a line continuation in a comment? > > Regards, > JMPC
Re: plt section in kernel due to endbr64
Christian Weisgerber wrote: > Alexander Bluhm: > > > After enabling -fcf-protection=branch for the kernel, we have a new > > .plt section in the kernel. It was not there before. > > Same issue in userland: At least /usr/lib/crt0.o and /usr/lib/crtbegin.o > have grown .plt and .note.gnu.property sections and some tools > (ld.bfd?) don't like it, as shown by corresponding ports build > failures. I think it is more important to teach ld.bfd that it should accept the new sections, than removing them. Obviously including them is the going to be the new way or we'll always be fighting the issue every clang update.
Re: plt section in kernel due to endbr64
It may still be better to add it to match the style. On i386, also. It is quite surprising compiler behaviour to create a PLT for such .rodata.. Alexander Bluhm wrote: > On Thu, Apr 20, 2023 at 05:21:37PM -0600, Theo de Raadt wrote: > > I wonder if the same happens on arm64. > > On amd64 with the strange behavior linking gapdummy.o to gap.o adds > a .plt. > > root@ot32:.../obj# objdump -s gapdummy.o | grep 'Contents of section' > Contents of section .note.gnu.property: > root@ot32:.../obj# objdump -s gap.o | grep 'Contents of section' > Contents of section .text: > Contents of section .rodata: > Contents of section .data: > Contents of section .plt: > Contents of section .note.gnu.property: > > On arm64 we get the .note.gnu.property, but no .plt. No .plt in > bsd. > > root@ot11:.../obj# objdump -s gapdummy.o | grep 'Contents of section' > Contents of section .note.gnu.property: > Contents of section .comment: > root@ot11:.../obj# objdump -s gap.o | grep 'Contents of section' > Contents of section .text: > Contents of section .rodata: > Contents of section .data: > Contents of section .comment: > Contents of section .note.gnu.property: > > With my fix (compiling -fcf-protection=none gapdummy.c) on amd64 > neither .note.gnu.property nor .plt is added. > > bluhm@t430s:.../obj$ objdump -s gapdummy.o | grep 'Contents of section' > bluhm@t430s:.../obj$ objdump -s gap.o | grep 'Contents of section' > Contents of section .text: > Contents of section .rodata: > Contents of section .data: > > Don't know if we want to change anything on arm64. > > bluhm
Re: [patch] Avoid change of permissions in /etc/resolv.conf
I'm saying you will find this "problem" in 100 places, because the real problem is your own change. Juan Picca wrote: > On Thu, Apr 20, 2023 at 11:33:30PM -0600, Theo de Raadt wrote: > > But this situation does not arise, not in this program, and not in 20 other > > daemons. > > > > You changed something to cause this problem. > > Yes. > > I found a similar case in > https://cvsweb.openbsd.org/src/usr.sbin/pkg_add/OpenBSD/AddDelete.pm?rev=1.97=text/x-cvsweb-markup > but from your response maybe it's not the same. > > Sorry for the noise! > Regards, > JMPC
Re: [patch] Avoid change of permissions in /etc/resolv.conf
But this situation does not arise, not in this program, and not in 20 other daemons. You changed something to cause this problem. Juan Picca wrote: > Force a standard umask in /sbin/resolvd/resolvd.c. > If not done and the default mask is a restrictive one, /etc/resolv.conf > ends up not readable. > > Regards, > JMPC
Re: plt section in kernel due to endbr64
I wonder if the same happens on arm64. Someone might want to try to do endbr32 on i386. It lacks a solid tail-CFI (only stack-protector on some functions), mostly because retguard isn't possible on the limited registers. So i386 would benefit from having a head CFI.
Re: plt section in kernel due to endbr64
Thank you. That is correct.
Alexander Bluhm wrote:
> Hi,
>
> After enabling -fcf-protection=branch for the kernel, we have a new
> .plt section in the kernel. It was not there before.
>
> $ objdump -s .../snapshots/amd64/bsd
> ...
> 82048540 c7c13140 0682c9e9 c43646ff ..1@.6F.
> Contents of section .plt:
> 82048550
> Contents of section .rodata:
> 82049000
>
> This is caused by compiling gapdummy.c with -fcf-protection=branch.
> Then gapdummy.o gets a new .note.gnu.property section.
>
> $ objdump -s arch/amd64/compile/GENERIC.MP/obj/gapdummy.o
>
> arch/amd64/compile/GENERIC.MP/obj/gapdummy.o: file format elf64-x86-64
>
> Contents of section .note.gnu.property:
> 0400 1000 0500 474e5500 GNU.
> 0010 02c0 0400 0100
>
> When creating gap.o from that, the linker adds a .plt section which
> finally shows up in the kernel.
>
> Diff below restores old behavior. I stumbled over this when linking
> kernel with sorted and aligned objects for my performance tests.
> There it resulted in unexpexted gaps within the kernel image.
>
> I am not sure if we want to fix this. If not, I can probly add
> another workaround for my use case. But this new .plt does not
> make much sense.
>
> ok?
>
> bluhm
>
> Index: sys/arch/amd64/conf/Makefile.amd64
> ===
> RCS file: /mount/openbsd/cvs/src/sys/arch/amd64/conf/Makefile.amd64,v
> retrieving revision 1.131
> diff -u -p -r1.131 Makefile.amd64
> --- sys/arch/amd64/conf/Makefile.amd6417 Apr 2023 01:14:24 -
> 1.131
> +++ sys/arch/amd64/conf/Makefile.amd6420 Apr 2023 21:25:05 -
> @@ -177,7 +177,7 @@ ld.script: ${_machdir}/conf/ld.script
>
> gapdummy.o:
> echo '__asm(".section .rodata,\"a\"");' > gapdummy.c
> - ${CC} -c ${CFLAGS} ${CPPFLAGS} gapdummy.c -o $@
> + ${CC} -c ${CFLAGS} ${CPPFLAGS} -fcf-protection=none gapdummy.c -o $@
>
> makegap.sh:
> cp $S/conf/makegap.sh $@
>
Re: llvm15: Make -mbranch-protection=bti the default
Christian Weisgerber wrote:
> Mark Kettenis:
>
> > CVSROOT:/cvs
> > Module name:src
> > Changes by: kette...@cvs.openbsd.org2023/04/17 12:10:26
> >
> > Modified files:
> > gnu/llvm/clang/lib/Driver/ToolChains: Clang.cpp
> >
> > Log message:
> > Make -mbranch-protection=bti the default on OpenBSD.
>
> LLVM 15 reshuffled some code, so the change above no longer applies
> there. Here's my attempt to add it:
I suspect there will be a different arm64 variation of that diff coming
which tries to change the default at a lower level.
Here is the corresponding amd64 diff (an earlier version had a bug) for
in-tree clang. It changes it so BTI is enabled by default, and then adds
missing code to allow it to be turned off.
Index: gnu/llvm/clang/include/clang/Basic/CodeGenOptions.def
===
RCS file: /cvs/src/gnu/llvm/clang/include/clang/Basic/CodeGenOptions.def,v
retrieving revision 1.4
diff -u -p -u -r1.4 CodeGenOptions.def
--- gnu/llvm/clang/include/clang/Basic/CodeGenOptions.def 17 Dec 2021
14:46:43 - 1.4
+++ gnu/llvm/clang/include/clang/Basic/CodeGenOptions.def 17 Apr 2023
18:36:15 -
@@ -102,7 +102,7 @@ CODEGENOPT(InstrumentFunctionEntryBare ,
///< -finstrument-function-entry-bare is
enabled.
CODEGENOPT(CFProtectionReturn , 1, 0) ///< if -fcf-protection is
///< set to full or return.
-CODEGENOPT(CFProtectionBranch , 1, 0) ///< if -fcf-protection is
+CODEGENOPT(CFProtectionBranch , 1, 1) ///< if -fcf-protection is
///< set to full or branch.
CODEGENOPT(XRayInstrumentFunctions , 1, 0) ///< Set when -fxray-instrument is
///< enabled.
Index: gnu/llvm/clang/lib/Frontend/CompilerInvocation.cpp
===
RCS file: /cvs/src/gnu/llvm/clang/lib/Frontend/CompilerInvocation.cpp,v
retrieving revision 1.5
diff -u -p -u -r1.5 CompilerInvocation.cpp
--- gnu/llvm/clang/lib/Frontend/CompilerInvocation.cpp 17 Dec 2021 14:46:44
- 1.5
+++ gnu/llvm/clang/lib/Frontend/CompilerInvocation.cpp 18 Apr 2023 10:48:28
-
@@ -1792,7 +1792,10 @@ bool CompilerInvocation::ParseCodeGenArg
Opts.CFProtectionReturn = 1;
else if (Name == "branch")
Opts.CFProtectionBranch = 1;
-else if (Name != "none")
+else if (Name == "none") {
+ Opts.CFProtectionBranch = 0;
+ Opts.CFProtectionReturn = 0;
+} else
Diags.Report(diag::err_drv_invalid_value) << A->getAsString(Args) <<
Name;
}
@@ -3653,7 +3656,8 @@ bool CompilerInvocation::ParseLangArgs(L
StringRef Name = A->getValue();
if (Name == "full" || Name == "branch") {
Opts.CFProtectionBranch = 1;
-}
+} else if (Name == "none")
+ Opts.CFProtectionBranch = 1;
}
if ((Args.hasArg(OPT_fsycl_is_device) || Args.hasArg(OPT_fsycl_is_host)) &&
Re: installer: indent interface and disk listings
Yes, that is nice
Klemens Nanni wrote:
> '?' output could better distuingish from questions and other lines,
> like sets selection does with four leading spaces.
>
> ---
> Terminal type? [vt220]
> Available disks are: sd0.
> Which disk is the root disk? ('?' for details) [sd0] ?
> -sd0: VirtIO, Block Device (5.0G)
> -sd1: VirtIO, Block Device (5.0G)
> +sd0: VirtIO, Block Device (5.0G)
> +sd1: VirtIO, Block Device (5.0G)
> Available disks are: sd0.
> Which disk is the root disk? ('?' for details) [sd0]
> ---
> Network interface to configure? (name, lladdr, '?', or 'done') [vio0] ?
> Available network interfaces are: vio0 vio1 vlan0.
> - vio0: lladdr fe:e1:bb:d1:e4:c3
> - vio1: lladdr fe:e1:bb:d2:63:dd
> +vio0: lladdr fe:e1:bb:d1:e4:c3
> +vio1: lladdr fe:e1:bb:d2:63:dd
> Network interface to configure? (name, lladdr, '?', or 'done') [vio0]
> ---
>
> Feedback? OK?
>
> Index: install.sub
> ===
> RCS file: /cvs/src/distrib/miniroot/install.sub,v
> retrieving revision 1.1241
> diff -u -p -r1.1241 install.sub
> --- install.sub 7 Apr 2023 13:48:42 - 1.1241
> +++ install.sub 16 Apr 2023 20:02:13 -
> @@ -281,7 +281,7 @@ diskinfo() {
> _s=$(disklabel -dpg $_d 2>/dev/null | sed -n '/.*# total bytes:
> \(.*\)/{s//(\1)/p;}')
> rm -f /dev/{r,}$_d?
>
> - echo "$_d: $_i $_n $_s"
> + echo "$_d: $_i $_n $_s"
> done
> }
>
> @@ -1354,7 +1354,7 @@ configure_ifs() {
> if [[ $resp == '?' ]]; then
> for _if in "${_ifs[@]}"; do
> _lladdr=$(if_name_to_lladdr $_if)
> - [[ -n $_lladdr ]] && echo " $_if: lladdr
> $_lladdr"
> + [[ -n $_lladdr ]] && echo "$_if: lladdr
> $_lladdr"
> done
> fi
>
>
Re: patch: profiling using utrace(2) (compatible with pledge and unveil)
Sebastien Marie wrote: > For post-execution gmon.out extraction, an helper might be needed: ktrace.out > could contain multiple gmon.out files. kdump can be taught to generate the right file.
Re: OF_getpropstr()
Mark Kettenis wrote:
> > +{
> > + int len;
> > +
> > + len = OF_getprop(handle, prop, buf, buflen);
> > + if (buflen > 0)
> > + buf[min(len, buflen - 1)] = '\0';
> > +
> > + return (len);
>
I've mailed dlg seperately, but will raise it here also.
If buflen is 0, then why call OF_getprop at all? I doubt this situation
occurs, but you want to protect against it, ok
Maybe in the end if looks like this:
int len = 0;
if (buflen > 0) {
len = OF_getprop(handle, prop, buf, buflen - 1);
buf[min(len, buflen - 1)] = '\0';
}
return (len);
OF_getprop() is now being called with buflen -1, which can avoid one
extra character of processing effort for a long input string.
Re: cleanup vmm_start_vm, simplifying fd cleanup
Florian Obser wrote: > On 2023-04-07 10:51 -04, Dave Voutila wrote: > > In vmd, the vmm process forks to create the resulting vm process. After > > this fork, the vmm parent process closes all the file descriptors > > pointing to the vm's devices (cdrom, kernel, disks, nics, etc.). > > > > The logic was a bit funky, so this change relies on the fact we can > > attempt the close(2) call and use its success/failure to determine if we > > have an fd to mark -1 in the vm structure. (I guess we could just > > blindly set them to -1 post-close, but this feels more sensical to me.) > > > > this will create some noise in ktrace every time you pass -1 to close(2) > you'll see > > CALL close(-1) > RET close -1 errno 9 Bad file descriptor > > Not a vmd user and I don't plan to hack on it any time soon, so > *shrug*. And also, have you considered EINTR? I prefer if you keep state in the application.
Re: use labels in the device tree to init interface descriptions
Claudio Jeker wrote: > On Fri, Apr 07, 2023 at 04:53:52PM +1000, David Gwynne wrote: > > ethernet interfaces in device trees can have a "label" property which > > is generally used (when it is used) to identify which connector it is on > > the case or something like that. eg, eth2 in the turris omnia device > > tree has 'label = "wan"' on the mvneta interface. > > > > ive been using labels in the dts recently to help me figure out what > > goes where, so this has been useful to me. if/when we support switches > > (eg mvsw), their ports are often labelled so i'd like to apply this idea > > to them too. > > > > ok? > > I think doing this is OK but I'm not sure if the usage of OF_getprop() > is correct. OF_getprop() uses 'memcpy(buf, data, min(len, buflen));' > to fill the if_description buffer. So if the provided label is larger than > sizeof(ifp->if_description) the description string is no longer NUL > terminated. I don't have a problem with the pre-population of the string either. It's informational either way. But claudio is right, OF_getprop() is not like strlcpy, it behaves like strncpy() so if you want to use it you need to copy n-1 bytes, and manuall set the last byte to 0. Or make a new function which is more suitable.
