Re: uvm_pagelookup(): moar sanity checks

2023-08-11 Thread Mark Kettenis
> Date: Fri, 11 Aug 2023 21:34:45 +0200
> From: Martin Pieuchot 
> 
> On 11/08/23(Fri) 20:41, Mark Kettenis wrote:
> > > Date: Fri, 11 Aug 2023 20:12:19 +0200
> > > From: Martin Pieuchot 
> > > 
> > > Here's a simple diff to add some more sanity checks in uvm_pagelookup().
> > > 
> > > Nothing fancy, it helps documenting the flags and reduce the difference
> > > with NetBSD.  This is part of my on-going work on UVM.
> > > 
> > > ok?
> > 
> > NetBSD really has that extra blank line after the return?
> 
> No.  It's my mistake.

ok kettenis@

> > > Index: uvm/uvm_page.c
> > > ===
> > > RCS file: /cvs/src/sys/uvm/uvm_page.c,v
> > > retrieving revision 1.172
> > > diff -u -p -r1.172 uvm_page.c
> > > --- uvm/uvm_page.c13 May 2023 09:24:59 -  1.172
> > > +++ uvm/uvm_page.c11 Aug 2023 17:55:43 -
> > > @@ -1219,10 +1219,16 @@ struct vm_page *
> > >  uvm_pagelookup(struct uvm_object *obj, voff_t off)
> > >  {
> > >   /* XXX if stack is too much, handroll */
> > > - struct vm_page pg;
> > > + struct vm_page p, *pg;
> > > +
> > > + p.offset = off;
> > > + pg = RBT_FIND(uvm_objtree, &obj->memt, &p);
> > > +
> > > + KASSERT(pg == NULL || obj->uo_npages != 0);
> > > + KASSERT(pg == NULL || (pg->pg_flags & PG_RELEASED) == 0 ||
> > > + (pg->pg_flags & PG_BUSY) != 0);
> > > + return (pg);
> > >  
> > > - pg.offset = off;
> > > - return RBT_FIND(uvm_objtree, &obj->memt, &pg);
> > >  }
> > >  
> > >  /*
> > > 
> > > 
> 



smr_grace_wait(): Skip halted CPUs

2023-08-11 Thread Martin Pieuchot
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.

ok?

Index: kern/kern_smr.c
===
RCS file: /cvs/src/sys/kern/kern_smr.c,v
retrieving revision 1.16
diff -u -p -r1.16 kern_smr.c
--- kern/kern_smr.c 14 Aug 2022 01:58:27 -  1.16
+++ kern/kern_smr.c 11 Aug 2023 19:43:54 -
@@ -158,6 +158,8 @@ smr_grace_wait(void)
CPU_INFO_FOREACH(cii, ci) {
if (!CPU_IS_RUNNING(ci))
continue;
+   if (ci->ci_schedstate.spc_schedflags & SPCF_HALTED)
+   continue;
if (READ_ONCE(ci->ci_schedstate.spc_smrgp) == smrgp)
continue;
sched_peg_curproc(ci);



Re: uvm_pagelookup(): moar sanity checks

2023-08-11 Thread Martin Pieuchot
On 11/08/23(Fri) 20:41, Mark Kettenis wrote:
> > Date: Fri, 11 Aug 2023 20:12:19 +0200
> > From: Martin Pieuchot 
> > 
> > Here's a simple diff to add some more sanity checks in uvm_pagelookup().
> > 
> > Nothing fancy, it helps documenting the flags and reduce the difference
> > with NetBSD.  This is part of my on-going work on UVM.
> > 
> > ok?
> 
> NetBSD really has that extra blank line after the return?

No.  It's my mistake.
 
> > Index: uvm/uvm_page.c
> > ===
> > RCS file: /cvs/src/sys/uvm/uvm_page.c,v
> > retrieving revision 1.172
> > diff -u -p -r1.172 uvm_page.c
> > --- uvm/uvm_page.c  13 May 2023 09:24:59 -  1.172
> > +++ uvm/uvm_page.c  11 Aug 2023 17:55:43 -
> > @@ -1219,10 +1219,16 @@ struct vm_page *
> >  uvm_pagelookup(struct uvm_object *obj, voff_t off)
> >  {
> > /* XXX if stack is too much, handroll */
> > -   struct vm_page pg;
> > +   struct vm_page p, *pg;
> > +
> > +   p.offset = off;
> > +   pg = RBT_FIND(uvm_objtree, &obj->memt, &p);
> > +
> > +   KASSERT(pg == NULL || obj->uo_npages != 0);
> > +   KASSERT(pg == NULL || (pg->pg_flags & PG_RELEASED) == 0 ||
> > +   (pg->pg_flags & PG_BUSY) != 0);
> > +   return (pg);
> >  
> > -   pg.offset = off;
> > -   return RBT_FIND(uvm_objtree, &obj->memt, &pg);
> >  }
> >  
> >  /*
> > 
> > 



Re: uvm_pagelookup(): moar sanity checks

2023-08-11 Thread Mark Kettenis
> Date: Fri, 11 Aug 2023 20:12:19 +0200
> From: Martin Pieuchot 
> 
> Here's a simple diff to add some more sanity checks in uvm_pagelookup().
> 
> Nothing fancy, it helps documenting the flags and reduce the difference
> with NetBSD.  This is part of my on-going work on UVM.
> 
> ok?

NetBSD really has that extra blank line after the return?

> Index: uvm/uvm_page.c
> ===
> RCS file: /cvs/src/sys/uvm/uvm_page.c,v
> retrieving revision 1.172
> diff -u -p -r1.172 uvm_page.c
> --- uvm/uvm_page.c13 May 2023 09:24:59 -  1.172
> +++ uvm/uvm_page.c11 Aug 2023 17:55:43 -
> @@ -1219,10 +1219,16 @@ struct vm_page *
>  uvm_pagelookup(struct uvm_object *obj, voff_t off)
>  {
>   /* XXX if stack is too much, handroll */
> - struct vm_page pg;
> + struct vm_page p, *pg;
> +
> + p.offset = off;
> + pg = RBT_FIND(uvm_objtree, &obj->memt, &p);
> +
> + KASSERT(pg == NULL || obj->uo_npages != 0);
> + KASSERT(pg == NULL || (pg->pg_flags & PG_RELEASED) == 0 ||
> + (pg->pg_flags & PG_BUSY) != 0);
> + return (pg);
>  
> - pg.offset = off;
> - return RBT_FIND(uvm_objtree, &obj->memt, &pg);
>  }
>  
>  /*
> 
> 



uvm_pagelookup(): moar sanity checks

2023-08-11 Thread Martin Pieuchot
Here's a simple diff to add some more sanity checks in uvm_pagelookup().

Nothing fancy, it helps documenting the flags and reduce the difference
with NetBSD.  This is part of my on-going work on UVM.

ok?

Index: uvm/uvm_page.c
===
RCS file: /cvs/src/sys/uvm/uvm_page.c,v
retrieving revision 1.172
diff -u -p -r1.172 uvm_page.c
--- uvm/uvm_page.c  13 May 2023 09:24:59 -  1.172
+++ uvm/uvm_page.c  11 Aug 2023 17:55:43 -
@@ -1219,10 +1219,16 @@ struct vm_page *
 uvm_pagelookup(struct uvm_object *obj, voff_t off)
 {
/* XXX if stack is too much, handroll */
-   struct vm_page pg;
+   struct vm_page p, *pg;
+
+   p.offset = off;
+   pg = RBT_FIND(uvm_objtree, &obj->memt, &p);
+
+   KASSERT(pg == NULL || obj->uo_npages != 0);
+   KASSERT(pg == NULL || (pg->pg_flags & PG_RELEASED) == 0 ||
+   (pg->pg_flags & PG_BUSY) != 0);
+   return (pg);
 
-   pg.offset = off;
-   return RBT_FIND(uvm_objtree, &obj->memt, &pg);
 }
 
 /*



Re: bioctl: do not confirm new passphrases on stdin

2023-08-11 Thread Klemens Nanni
On Wed, Aug 02, 2023 at 10:37:36AM +, Klemens Nanni wrote:
> Creating new volumes prompts
>   Passphrase:
>   Re-type passphrase:
> which is sane for interative usage, but -s (which omits prompts) to read
> from stdin also prompts twice.
> 
> I think that's neither intuitive nor ergonomical and as intended for
> non-interactive scripts, -s should take a new passphase just once.
> 
> Until a month ago, the manual errorneously said -s could not be used during
> initial creation anyway, so I worry little about existing scripts like
> 
>   printf '%s\n%s\n' "$pw" "$pw" | bioctl -s -cC -lsd0a softraid0
> 
> Diff below makes -s create new volumes with a single passphase:
> 
>   # echo secret |   bioctl -s -Cforce -cC -lvnd0a softraid0
>   bioctl: Passphrases did not match
>   # echo secret | ./obj/bioctl -s -Cforce -cC -lvnd0a softraid0
>   softraid0: CRYPTO volume attached as sd3
> 
> Feedback? Objection? OK?

Ping.

Index: bioctl.c
===
RCS file: /cvs/src/sbin/bioctl/bioctl.c,v
retrieving revision 1.151
diff -u -p -r1.151 bioctl.c
--- bioctl.c18 Oct 2022 07:04:20 -  1.151
+++ bioctl.c11 Aug 2023 15:42:41 -
@@ -989,7 +989,7 @@ bio_kdf_generate(struct sr_crypto_kdfinf
derive_key(kdfinfo->pbkdf.generic.type, kdfinfo->pbkdf.rounds,
kdfinfo->maskkey, sizeof(kdfinfo->maskkey),
kdfinfo->pbkdf.salt, sizeof(kdfinfo->pbkdf.salt),
-   "New passphrase: ", 1);
+   "New passphrase: ", rpp_flag == RPP_REQUIRE_TTY ? 1 : 0);
 }
 
 int



Re: installer: disk crypto: crank KDF rounds to hardware based default

2023-08-11 Thread Mark Kettenis
> From: "Theo de Raadt" 
> Date: Fri, 11 Aug 2023 08:50:32 -0600
> 
> 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.

So the man page just lies?



Re: installer: disk crypto: crank KDF rounds to hardware based default

2023-08-11 Thread Klemens Nanni
On Fri, Aug 11, 2023 at 03:51:38PM +0100, Stuart Henderson wrote:
> Agreed. (Re bcrypt, I usually completely ignore auto rounds, I had just
> forgotten to set that up on the machine where I noticed the problem..)
> 
> Also, am I right in thinking that this only affects the time when
> entering the passphrase when mounting or creating the device, i.e. once
> per boot?

Yes, correct.

> 
> If so, there's nowhere near as much a downside to that being slow
> as there is for user login. (anyone actually wanting to crack these
> passphrases would be doing it on a fast system rather than whatever
> the device is normally used with, so there are valid reasons for
> picking something that might be a bit slow if it doesn't cause too
> much system impact).

The minimum of 16 can be cranked independently whilst still defaulting
to a hardware based default.



Re: installer: disk crypto: crank KDF rounds to hardware based default

2023-08-11 Thread Stuart Henderson
On 2023/08/11 16:43, 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.

Agreed. (Re bcrypt, I usually completely ignore auto rounds, I had just
forgotten to set that up on the machine where I noticed the problem..)

Also, am I right in thinking that this only affects the time when
entering the passphrase when mounting or creating the device, i.e. once
per boot?

If so, there's nowhere near as much a downside to that being slow
as there is for user login. (anyone actually wanting to crack these
passphrases would be doing it on a fast system rather than whatever
the device is normally used with, so there are valid reasons for
picking something that might be a bit slow if it doesn't cause too
much system impact).


> > >From bioctl(8):
> >  -r rounds
> >  The number of iterations for the KDF algorithm to use when
> >  converting a passphrase into a key, in order to create a new
> >  encrypted volume or change the passphrase of an existing
> >  encrypted volume.  A larger number of iterations takes more 
> > time,
> >  but offers increased resistance against passphrase guessing
> >  attacks.  If rounds is specified as auto, the number of rounds
> >  will be automatically determined based on system performance.
> >  Otherwise the minimum is 4 rounds and the default is 16.
> > 
> > Rebased diff.
> > 
> > Index: install.sub
> > ===
> > RCS file: /cvs/src/distrib/miniroot/install.sub,v
> > retrieving revision 1.1253
> > diff -u -p -r1.1253 install.sub
> > --- install.sub 10 Aug 2023 17:09:34 -  1.1253
> > +++ install.sub 11 Aug 2023 11:02:19 -
> > @@ -3097,7 +3097,7 @@ encrypt_root() {
> > md_prep_fdisk $_chunk
> > echo 'RAID *' | disklabel -w -A -T- $_chunk
> >  
> > -   until bioctl -Cforce -cC -l${_chunk}a softraid0 >/dev/null; do
> > +   until bioctl -Cforce -cC -rauto -l${_chunk}a softraid0 >/dev/null; do
> > # Most likely botched passphrases, silently retry twice.
> > ((++_tries < 3)) || exit
> > done
> > 
> > 
> 



Re: installer: disk crypto: crank KDF rounds to hardware based default

2023-08-11 Thread Theo de Raadt
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: installer: disk crypto: crank KDF rounds to hardware based default

2023-08-11 Thread Mark Kettenis
> 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.

> >From bioctl(8):
>  -r rounds
>  The number of iterations for the KDF algorithm to use when
>  converting a passphrase into a key, in order to create a new
>  encrypted volume or change the passphrase of an existing
>  encrypted volume.  A larger number of iterations takes more time,
>  but offers increased resistance against passphrase guessing
>  attacks.  If rounds is specified as auto, the number of rounds
>  will be automatically determined based on system performance.
>  Otherwise the minimum is 4 rounds and the default is 16.
> 
> Rebased diff.
> 
> Index: install.sub
> ===
> RCS file: /cvs/src/distrib/miniroot/install.sub,v
> retrieving revision 1.1253
> diff -u -p -r1.1253 install.sub
> --- install.sub   10 Aug 2023 17:09:34 -  1.1253
> +++ install.sub   11 Aug 2023 11:02:19 -
> @@ -3097,7 +3097,7 @@ encrypt_root() {
>   md_prep_fdisk $_chunk
>   echo 'RAID *' | disklabel -w -A -T- $_chunk
>  
> - until bioctl -Cforce -cC -l${_chunk}a softraid0 >/dev/null; do
> + until bioctl -Cforce -cC -rauto -l${_chunk}a softraid0 >/dev/null; do
>   # Most likely botched passphrases, silently retry twice.
>   ((++_tries < 3)) || exit
>   done
> 
> 



bioctl: default KDF rounds to hardware based value

2023-08-11 Thread Klemens Nanni
Joel encouraged me to switch to '-r auto' by default sooner than later.
The alternative installer diff on tech@ would thus be obsolete/a NOOP.

If you do encrypted disk installs on one machine, but use them on another,
or you want a specific number of rounds, just use '-r N' during creation
or passphrase updates to change existing volumes (might be recommended to
increase rounds on older volumes?).

Diff below only flips the default so '-r' value behaviour does not change.
Do we want a warning on '-r auto' usage to deprecate explicitly specifying
the new default?

Feedback? Objection? OK?

Index: bioctl.8
===
RCS file: /cvs/src/sbin/bioctl/bioctl.8,v
retrieving revision 1.111
diff -u -p -r1.111 bioctl.8
--- bioctl.86 Jul 2023 21:08:50 -   1.111
+++ bioctl.811 Aug 2023 13:49:09 -
@@ -282,11 +282,13 @@ passphrase into a key, in order to creat
 passphrase of an existing encrypted volume.
 A larger number of iterations takes more time, but offers increased resistance
 against passphrase guessing attacks.
-If
+By default, or if
 .Ar rounds
-is specified as "auto", the number of rounds will be automatically determined
+is specified as
+.Cm auto ,
+the number of rounds will be automatically determined
 based on system performance.
-Otherwise the minimum is 4 rounds and the default is 16.
+The minimum is 4 rounds.
 .It Fl s
 Read the passphrase for the selected crypto volume from
 .Pa /dev/stdin
Index: bioctl.c
===
RCS file: /cvs/src/sbin/bioctl/bioctl.c,v
retrieving revision 1.151
diff -u -p -r1.151 bioctl.c
--- bioctl.c18 Oct 2022 07:04:20 -  1.151
+++ bioctl.c11 Aug 2023 13:36:41 -
@@ -89,7 +89,7 @@ int   devh = -1;
 inthuman;
 intverbose;
 u_int32_t  cflags = 0;
-intrflag = 0;
+intrflag = -1; /* auto */
 char   *password;
 
 void   *bio_cookie;



Re: ldd: check read return value to avoid unitialized struct fields

2023-08-11 Thread Lucas
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.

Like this? The int -> size_t promotion can be a different commit or left
out.

---
commit 7663fd702838ae390515cb9326c2706a57a2983b (ldd-read-rv)
from: Lucas 
date: Fri Aug 11 11:43:32 2023 UTC
 
 Check for a full read. Don't use warn when errno might be unmodified.
 Promote size from int to size_t.
 
 M  libexec/ld.so/ldd/ldd.c  |  5+  4-

1 file changed, 5 insertions(+), 4 deletions(-)

diff 7d242c13afd19e56cc21befac2ce5cdc1ac4992b 
7663fd702838ae390515cb9326c2706a57a2983b
commit - 7d242c13afd19e56cc21befac2ce5cdc1ac4992b
commit + 7663fd702838ae390515cb9326c2706a57a2983b
blob - 9e8c5065cd843ff36d91efcb868b94ffd4c98365
blob + 16b59a75e63bfe894922c4195aa11cd5e75802a1
--- libexec/ld.so/ldd/ldd.c
+++ libexec/ld.so/ldd/ldd.c
@@ -96,7 +96,8 @@ doit(char *name)
 {
Elf_Ehdr ehdr;
Elf_Phdr *phdr;
-   int fd, i, size, status, interp=0;
+   size_t size;
+   int fd, i, status, interp=0;
char buf[PATH_MAX];
struct stat st;
void * dlhandle;
@@ -118,8 +119,8 @@ doit(char *name)
return 1;
}
 
-   if (read(fd, &ehdr, sizeof(ehdr)) < 0) {
-   warn("read(%s)", name);
+   if (read(fd, &ehdr, sizeof(ehdr)) != sizeof(ehdr)) {
+   warnx("%s: incomplete ELF header", name);
close(fd);
return 1;
}
@@ -141,7 +142,7 @@ doit(char *name)
size = ehdr.e_phnum * sizeof(Elf_Phdr);
 
if (pread(fd, phdr, size, ehdr.e_phoff) != size) {
-   warn("read(%s)", name);
+   warnx("%s: incomplete program header", name);
close(fd);
free(phdr);
return 1;



Re: installer: disk crypto: crank KDF rounds to hardware based default

2023-08-11 Thread 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.

>From bioctl(8):
 -r rounds
 The number of iterations for the KDF algorithm to use when
 converting a passphrase into a key, in order to create a new
 encrypted volume or change the passphrase of an existing
 encrypted volume.  A larger number of iterations takes more time,
 but offers increased resistance against passphrase guessing
 attacks.  If rounds is specified as auto, the number of rounds
 will be automatically determined based on system performance.
 Otherwise the minimum is 4 rounds and the default is 16.

Rebased diff.

Index: install.sub
===
RCS file: /cvs/src/distrib/miniroot/install.sub,v
retrieving revision 1.1253
diff -u -p -r1.1253 install.sub
--- install.sub 10 Aug 2023 17:09:34 -  1.1253
+++ install.sub 11 Aug 2023 11:02:19 -
@@ -3097,7 +3097,7 @@ encrypt_root() {
md_prep_fdisk $_chunk
echo 'RAID *' | disklabel -w -A -T- $_chunk
 
-   until bioctl -Cforce -cC -l${_chunk}a softraid0 >/dev/null; do
+   until bioctl -Cforce -cC -rauto -l${_chunk}a softraid0 >/dev/null; do
# Most likely botched passphrases, silently retry twice.
((++_tries < 3)) || exit
done