Re: CVS commit: src/sbin/cgdconfig

[Robert Elz]

Please test it.  In HEAD today, and last week, and for probably
a long time back into the past, /sbin/cgdconfig has threads, and
/rescue/cgdconfig does not.

I don"t know when argon2 support was added, or how to use it,
but if you do, it should be simple to create an cgd in vnd using
one, and then attempt to access it using the other.

Let us know the results.

kre

Re: CVS commit: src/sbin/cgdconfig

[nia]

On Mon, May 16, 2022 at 09:10:40AM +, Taylor R Campbell wrote:
> Surely `disabling threads' just means cgdconfig can't take advantage
> of parallelism to compute the same function in less time, not that
> cgdconfig computes a different function or fails to compute the same
> function, no?
> 

My understanding is that argon2 gives different results for
different values of P:

$ echo test | argon2 testtest -i -p 18
Hash:   07d31bd489c4264bde42d32a2cb1cd6020964d9c5789ae96025c0111478e07b
$ echo test | argon2 testtest -i -p 19
Hash:   b02710381cfc4c943ce4bafc5ac28684a4878dedd01c5e25617e9424c87619a2

If the differences between P are preserved when compiled without
pthreads, then please ignore my comment :/

Re: CVS commit: src/sbin/cgdconfig

[Robert Elz]

Date:Mon, 16 May 2022 09:10:40 +
From:Taylor R Campbell 
Message-ID:  <20220516090946.a3c4660...@jupiter.mumble.net>

  | > Please re-enable threads. They influence the output hash
  | > so by disabling threads you stop people from being able
  | > to decrypt their disks.
  |
  | Surely `disabling threads' just means cgdconfig can't take advantage
  | of parallelism to compute the same function in less time, not that
  | cgdconfig computes a different function or fails to compute the same
  | function, no?

I agree, the issue, whatever it was that nia saw, is far more
likely caused by the namespace changes influencing just what
functions are getting called, in an unintended way, than by
anything related to threading.

Can we have threads back the way they were last week?  That
is not race around adding -lpthread to every static link
that exists, most likely breaking some size limits along the
way.

Then, once things build again, if there is a problem, we can
debug it, rather than just guessing.

kre

Re: CVS commit: src/sbin/cgdconfig

[Christos Zoulas]

> On May 16, 2022, at 5:10 AM, Taylor R Campbell  wrote:
> 
>> Date: Mon, 16 May 2022 04:49:22 +
>> From: nia 
>> 
>> On Sun, May 15, 2022 at 03:53:27PM -0400, Christos Zoulas wrote:
>>> Log Message:
>>> Build argon2 inline so that crunched programs work. I also disabled threads
>>> for now; we can put them back if needed.
>> 
>> Please re-enable threads. They influence the output hash
>> so by disabling threads you stop people from being able
>> to decrypt their disks.
> 
> Surely `disabling threads' just means cgdconfig can't take advantage
> of parallelism to compute the same function in less time, not that
> cgdconfig computes a different function or fails to compute the same
> function, no?
> 
> I agree threads should be re-enabled, but maybe it would be reasonable
> to find a way to conditionalize this on crunchgen/rescue/whatever if
> that gets in the way.
> 
> Christos, can you write down the problems that led to making this
> commit?  The commit message doesn't explain any of what went wrong so
> I don't even know what to look for when putting threads back.

It is simple. You just take out the cpp define to disable and add thread.c to 
the
list of files to be built and -lpthread. The reason I changed the build in 
cgdconfig
from reaching out to libargon2 and using the pre-built library to explicitly 
building
the necessary objects locally was that I did not want to teach rescue about 
libargon2.
I just disabled threads in the process because it was the same way done in 
libcrypt.
I didn't expect that this would produce different results. Rescue was not 
threaded before
but now I had to add -lpthread for it to link. The whole thing is very weird. 
It all started
with me protecting all the extra symbols that libargon2 exposed to libcrypt. 
This in
turn made cgdconfig not link in rescue because it was missing argon2_hash which
before it was resolving from libcrypt. Which means that the cgdconfig in rescue 
was
built without threads before... What a mess.

christos



signature.asc
Description: Message signed with OpenPGP

Re: CVS commit: src/sbin/cgdconfig

[Taylor R Campbell]

> Date: Mon, 16 May 2022 04:49:22 +
> From: nia 
> 
> On Sun, May 15, 2022 at 03:53:27PM -0400, Christos Zoulas wrote:
> > Log Message:
> > Build argon2 inline so that crunched programs work. I also disabled threads
> > for now; we can put them back if needed.
> 
> Please re-enable threads. They influence the output hash
> so by disabling threads you stop people from being able
> to decrypt their disks.

Surely `disabling threads' just means cgdconfig can't take advantage
of parallelism to compute the same function in less time, not that
cgdconfig computes a different function or fails to compute the same
function, no?

I agree threads should be re-enabled, but maybe it would be reasonable
to find a way to conditionalize this on crunchgen/rescue/whatever if
that gets in the way.

Christos, can you write down the problems that led to making this
commit?  The commit message doesn't explain any of what went wrong so
I don't even know what to look for when putting threads back.

Re: CVS commit: src/sbin/cgdconfig

[nia]

On Sun, May 15, 2022 at 03:53:27PM -0400, Christos Zoulas wrote:
> Log Message:
> Build argon2 inline so that crunched programs work. I also disabled threads
> for now; we can put them back if needed.

Please re-enable threads. They influence the output hash
so by disabling threads you stop people from being able
to decrypt their disks.

Re: CVS commit: src/sbin/cgdconfig

[Joerg Sonnenberger]

On Sun, Nov 28, 2021 at 07:42:55AM -0800, Jason Thorpe wrote:
> 
> 
> > On Nov 27, 2021, at 6:01 PM, Christos Zoulas  wrote:
> > 
> > Module Name:src
> > Committed By:   christos
> > Date:   Sun Nov 28 02:01:30 UTC 2021
> > 
> > Modified Files:
> > src/sbin/cgdconfig: Makefile
> > 
> > Log Message:
> > -lpthread to LDADD (fixes lint build)
> 
> This change is wrong.  The -pthread option to the compiler does more than 
> just add -lpthread to the link phase.

Yeah, but the other changes are pretty much useless.

Joerg

Re: CVS commit: src/sbin/cgdconfig

[Christos Zoulas]

> On Nov 28, 2021, at 11:57 AM, Roland Illig  wrote:
>
> Am 28.11.2021 um 17:37 schrieb Jason Thorpe:
>>> On Nov 28, 2021, at 8:05 AM, Christos Zoulas 
>>> wrote:
>>>
>>> 1. which compilation flag should we add -pthread to? CFLAGS or
>>> COPTS? What about c++?
>>
>> GCC defines some preprocessor macros in response to -pthread, so …
>> CPPFLAGS?  Perhaps a better choice is to have a USE_PTHREADS that
>> individual program / library Makefiles can set to YES to cause the
>> right magic to happen in bsd.sys.mk?
>
> I like the idea of USE_PTHREADS.
>
> The option -pthread is not specified by POSIX and the GCC manual doesn't
> define which exact macros -pthread defines. Sure, Clang is compatible
> with GCC, but PCC doesn't need to. I don't want to add support for 3
> different compilers to lint. Having all the magic hidden behind a simple
> flag sounds easiest to me.
>

I agree!

christos



signature.asc
Description: Message signed with OpenPGP

Re: CVS commit: src/sbin/cgdconfig

[Roland Illig]

Am 28.11.2021 um 17:37 schrieb Jason Thorpe:

On Nov 28, 2021, at 8:05 AM, Christos Zoulas 
wrote:

1. which compilation flag should we add -pthread to? CFLAGS or
COPTS? What about c++?


GCC defines some preprocessor macros in response to -pthread, so …
CPPFLAGS?  Perhaps a better choice is to have a USE_PTHREADS that
individual program / library Makefiles can set to YES to cause the
right magic to happen in bsd.sys.mk?


I like the idea of USE_PTHREADS.

The option -pthread is not specified by POSIX and the GCC manual doesn't
define which exact macros -pthread defines. Sure, Clang is compatible
with GCC, but PCC doesn't need to. I don't want to add support for 3
different compilers to lint. Having all the magic hidden behind a simple
flag sounds easiest to me.

Roland

Re: CVS commit: src/sbin/cgdconfig

[Jason Thorpe]

> On Nov 28, 2021, at 8:05 AM, Christos Zoulas  wrote:
> 
> The change is correct; this is how it is done everywhere else in the tree. 
> You are right about -pthread doing more than adding -lpthread, but
> in that case, the -pthread should be added to CFLAGS/COPTS etc, 
> not LDADD so that it is effective during the compilation phase too, 
> not just the link phase. When I made the change, I considered going
> through the tree and adding -pthread to the CFLAGS/COPTS in the
> Makefiles where -pthread is in LDADD, but I did not want to do a
> half-assed job without thinking about it more:
> 
> 1. which compilation flag should we add -pthread to? CFLAGS or 
>   COPTS? What about c++?

GCC defines some preprocessor macros in response to -pthread, so … CPPFLAGS?  
Perhaps a better choice is to have a USE_PTHREADS that individual program / 
library Makefiles can set to YES to cause the right magic to happen in 
bsd.sys.mk?

> 2. do we remove the LDADD/DPADD pthread settings? I am thinking
>perhaps not, it does  not hurt, plus the DPADD will cause a rebuild 
>when libpthread changes.

That could be hidden away by the above suggestion.

-- thorpej

Re: CVS commit: src/sbin/cgdconfig

[Christos Zoulas]

The change is correct; this is how it is done everywhere else in the tree. 
You are right about -pthread doing more than adding -lpthread, but
in that case, the -pthread should be added to CFLAGS/COPTS etc, 
not LDADD so that it is effective during the compilation phase too, 
not just the link phase. When I made the change, I considered going
through the tree and adding -pthread to the CFLAGS/COPTS in the
Makefiles where -pthread is in LDADD, but I did not want to do a
half-assed job without thinking about it more:

1. which compilation flag should we add -pthread to? CFLAGS or 
   COPTS? What about c++?
2. do we remove the LDADD/DPADD pthread settings? I am thinking
perhaps not, it does  not hurt, plus the DPADD will cause a rebuild 
when libpthread changes.

The libargon addition to cgdconfig broke lint building because lint h
as not been taught about -pthread yet, and fixing it the way I fixed it, 
makes the lint  build work again and is consistent with the rest of the tree.

Best,

christos

> On Nov 28, 2021, at 10:42 AM, Jason Thorpe  wrote:
> 
> 
> 
>> On Nov 27, 2021, at 6:01 PM, Christos Zoulas  wrote:
>> 
>> Module Name: src
>> Committed By:christos
>> Date:Sun Nov 28 02:01:30 UTC 2021
>> 
>> Modified Files:
>>  src/sbin/cgdconfig: Makefile
>> 
>> Log Message:
>> -lpthread to LDADD (fixes lint build)
> 
> This change is wrong.  The -pthread option to the compiler does more than 
> just add -lpthread to the link phase.
> 
> -- thorpej

Re: CVS commit: src/sbin/cgdconfig

[Jason Thorpe]

> On Nov 27, 2021, at 6:01 PM, Christos Zoulas  wrote:
> 
> Module Name:  src
> Committed By: christos
> Date: Sun Nov 28 02:01:30 UTC 2021
> 
> Modified Files:
>   src/sbin/cgdconfig: Makefile
> 
> Log Message:
> -lpthread to LDADD (fixes lint build)

This change is wrong.  The -pthread option to the compiler does more than just 
add -lpthread to the link phase.

-- thorpej

Re: CVS commit: src/sbin/cgdconfig

[Christoph Badura]

On Sat, Dec 29, 2018 at 01:33:23PM +, Alexander Nasonov wrote:
> Christoph Badura wrote:
> > On Thu, Dec 27, 2018 at 10:41:55PM +, Alexander Nasonov wrote:
> > > Perhaps the simplest change would be to pass an unresolved (original)
> > > name when composing a paramsfile. E.g.
> > > 
> > > /etc/cgd/NAME=mylabel
> > > /etc/cgd/ROOT.e
> > 
> > Alas, this will break existing installations that e.g. use /etc/cgd/dkNN 
> > when
> > using NAME=label in fstab.
> 
> You can't use the same dkNN in fstab and in cgd.conf because mount will
> refuse to mount an encrypted partition.

Hmm. Right.

> I think it will only break setups that use NAME=label in _cgd.conf_ and
> don't specify a paramsfile. These setups are rare because NAME=label
> syntax was documented only a couple of days ago ;-) Though, some people
> may have figured it out before me.

I think you are right.  If it requires an explicit configuration change in
cgd.conf we're good.

--chris

Re: CVS commit: src/sbin/cgdconfig

[Christoph Badura]

On Thu, Dec 27, 2018 at 10:41:55PM +, Alexander Nasonov wrote:
> Perhaps the simplest change would be to pass an unresolved (original)
> name when composing a paramsfile. E.g.
> 
> /etc/cgd/NAME=mylabel
> /etc/cgd/ROOT.e

Alas, this will break existing installations that e.g. use /etc/cgd/dkNN when
using NAME=label in fstab.
For compatibility it may be necessary to try the resolved named when the
unresolved form does not exist.

I would prefer /etc/cgd/mylabel, btw.

--chris

Re: CVS commit: src/sbin/cgdconfig

[Alexander Nasonov]

Christoph Badura wrote:
> Using /etc/cgd/ROOT. has the advantage that the cgd will configure
> if the root device changes name, thus upholding POLA.
> 
> E.g. moving disks from a controller that attaches sd(4)s to one that
> attaches ld(4)s.  I believe you can see that when dd'ing an image from
> SDcard to MMC on Pinebook.
> 
> It seems to me that similar behaviour for NAME=label would be more useful
> too. dk(4) attachments move around in practice.

Yeah, I discovered it the hard way ;-)

Perhaps the simplest change would be to pass an unresolved (original)
name when composing a paramsfile. E.g.

/etc/cgd/NAME=mylabel
/etc/cgd/ROOT.e

-- 
Alex

Re: CVS commit: src/sbin/cgdconfig

[Christoph Badura]

On Thu, Dec 27, 2018 at 09:53:44PM +, Alexander Nasonov wrote:
> Alexander Nasonov wrote:
> > XXX Default paramsfile for NAME=label is /etc/cgd/dkNN (resolved wedge
> > partition) and /etc/cgd/ROOT. for ROOT.. This isn't yet
> > documented. IMO, it should be the other way around: /etc/cgd/label
> > for the former and /et/cgd/[root-device] for the latter.
> 
> This is true for NetBSD-8 which doesn't support ROOT. prefix.
> Both prefixes are resolved to real device names before composing
> a default paramsfile in NetBSD-current.

Using /etc/cgd/ROOT. has the advantage that the cgd will configure
if the root device changes name, thus upholding POLA.

E.g. moving disks from a controller that attaches sd(4)s to one that
attaches ld(4)s.  I believe you can see that when dd'ing an image from
SDcard to MMC on Pinebook.

It seems to me that similar behaviour for NAME=label would be more useful
too. dk(4) attachments move around in practice.

--chris

Re: CVS commit: src/sbin/cgdconfig

[Alexander Nasonov]

Robert Elz wrote:
> Module Name:  src
> Committed By: kre
> Date: Sat May  5 11:28:44 UTC 2018
> 
> Modified Files:
>   src/sbin/cgdconfig: cgdconfig.c
> 
> Log Message:
> Check whether the cgd device selected is available to be
> configured,that is, not already in use, before requesting
> passwords from the user (or elsewhere).

Is now a good time to request pullup-8 for this change (with
a follow-up fix) and a couple of other small changes?

-- 
Alex

Re: CVS commit: src/sbin/cgdconfig

[Alexander Nasonov]

matthew green wrote:
> "Alexander Nasonov" writes:
> > XXX Using memset for wiping isn't a good idea because memset is likely
> > optimised away by gcc. This should be revisited.
> 
> use explicit_memset(3)?

Yes, we should change memsets of sensitive buffers to explicit_memset
but we also should inspect code for any missing memsets.

-- 
Alex

re: CVS commit: src/sbin/cgdconfig

[matthew green]

"Alexander Nasonov" writes:
> Module Name:  src
> Committed By: alnsn
> Date: Wed May  9 18:11:56 UTC 2018
> 
> Modified Files:
>   src/sbin/cgdconfig: cgdconfig.8 cgdconfig.c
> 
> Log Message:
> Add '-e' option (echo the passphrase) and wipe the passphrase after use.
> 

> XXX Using memset for wiping isn't a good idea because memset is likely
> optimised away by gcc. This should be revisited.

use explicit_memset(3)?


.mrg.

Re: CVS commit: src/sbin/cgdconfig

[Robert Elz]

Date:Wed, 9 May 2018 08:59:55 +0100
From:Alexander Nasonov 
Message-ID:  <20180509075955.GA7743@neva>

  | Adding (argc > 0) check before calling opendisk1 fixes the crash.

Thanks - and I see what is wrong now, but (for whatever reason) that did
not fail for me, I guess Xen DomU allows *0 to work (though it is strange that
it would allow the opendisk(() to succeed.

Never mind, that is clearly broken, thanks.   I will fix it, but not quite 
that way I think.

kre

Re: CVS commit: src/sbin/cgdconfig

[Alexander Nasonov]

Alexander Nasonov wrote:
> (gdb) b opendisk1
> (gdb) run -p
> Starting program:
> /home/alnsn/netbsd-current/clean/src/sbin/cgdconfig/obj/cgdconfig -p
> 
> Breakpoint 1, 0x7f7ff78111f6 in opendisk1 () from /lib/libutil.so.7
> (gdb) x/s $rdi
> 0x0: # path=NULL

Adding (argc > 0) check before calling opendisk1 fixes the crash.

-- 
Alex

Re: CVS commit: src/sbin/cgdconfig

[Alexander Nasonov]

Robert Elz wrote:
> Date:Tue, 8 May 2018 19:15:28 +0100
> From:Alexander Nasonov 
> Message-ID:  <20180508180815.GA5990@neva>
> 
>   | I think it broke the tool. If you run
>   |
>   | cgdconfig -p
>   |
>   | it will crash.
> 
> Sorry, I cannot reproduce this, it looks to work OK to me.
> 
> Can you tell me exactly what command you gave and what
> "it will crash" means (core dump? other failure? ??)

(gdb) b opendisk1
(gdb) run -p
Starting program:
/home/alnsn/netbsd-current/clean/src/sbin/cgdconfig/obj/cgdconfig -p

Breakpoint 1, 0x7f7ff78111f6 in opendisk1 () from /lib/libutil.so.7
(gdb) x/s $rdi
0x0: # path=NULL

(gdb) c
Program received signal SIGSEGV, Segmentation fault.
0x7f7ff7116880 in strchr () from /lib/libc.so.12
(gdb) bt
#0  0x7f7ff7116880 in strchr () from /lib/libc.so.12
#1  0x7f7ff78110a8 in ?? () from /lib/libutil.so.7
#2  0x00202bc3 in configure ()
#3  0x002074d8 in main ()

(gdb) disassemble
Dump of assembler code for function strchr:
   0x7f7ff7116860 <+0>: movabs $0x101010101010101,%r8
   0x7f7ff711686a <+10>:movzbq %sil,%rdx
   0x7f7ff711686e <+14>:imul   $0x80,%r8,%r9
   0x7f7ff7116875 <+21>:imul   %r8,%rdx
   0x7f7ff7116879 <+25>:test   $0x7,%dil
   0x7f7ff711687d <+29>:jne0x7f7ff71168d5 
   0x7f7ff711687f <+31>:nop
=> 0x7f7ff7116880 <+32>:mov(%rdi),%rax
 
(gdb) x $rdi
0x0:Cannot access memory at address 0x0 # presumably the path argument

If I comment out the if block with opendisk1 inside:

(gdb) run -p
Starting program: 
/home/alnsn/netbsd-current/clean/src/sbin/cgdconfig/obj/cgdconfig -p
cgdconfig: wrong number of args
usage: cgdconfig [-nv] [-V vmeth] cgd dev [paramsfile]
   cgdconfig -C [-nv] [-f configfile]
   cgdconfig -G [-nv] [-i ivmeth] [-k kgmeth] [-o outfile] paramsfile
   cgdconfig -g [-nv] [-i ivmeth] [-k kgmeth] [-o outfile] alg [keylen]
   cgdconfig -l
   cgdconfig -s [-nv] [-i ivmeth] cgd dev alg [keylen]
   cgdconfig -U [-nv] [-f configfile]
   cgdconfig -u [-nv] cgd
[Inferior 1 (process 26827) exited with code 01]

-- 
Alex

Re: CVS commit: src/sbin/cgdconfig

[Robert Elz]

Date:Tue, 8 May 2018 19:15:28 +0100
From:Alexander Nasonov 
Message-ID:  <20180508180815.GA5990@neva>

  | I think it broke the tool. If you run
  |
  | cgdconfig -p
  |
  | it will crash.

Sorry, I cannot reproduce this, it looks to work OK to me.

Can you tell me exactly what command you gave and what
"it will crash" means (core dump? other failure? ??)

kre

ps: I also cannot see any way that the (really quote tiny, if you
ignore the rump related botch I made) change could have almost
any effect at all - it just adds a quick test that the cgd is not already
in use before it begins (attempting to) configure.

Re: CVS commit: src/sbin/cgdconfig

[Robert Elz]

Date:Tue, 8 May 2018 19:15:28 +0100
From:Alexander Nasonov 
Message-ID:  <20180508180815.GA5990@neva>

  | I think it broke the tool. If you run
  | cgdconfig -p
  | it will crash.

I shall take a look.

kre

Re: CVS commit: src/sbin/cgdconfig

[Alexander Nasonov]

Robert Elz wrote:
> Module Name:  src
> Committed By: kre
> Date: Sat May  5 11:28:44 UTC 2018
> 
> Modified Files:
>   src/sbin/cgdconfig: cgdconfig.c
> 
> Log Message:
> Check whether the cgd device selected is available to be
> configured,that is, not already in use, before requesting
> passwords from the user (or elsewhere).

I think it broke the tool. If you run

cgdconfig -p

it will crash.

Alex