* Re: [PATCH v7 2/3] pid: Introduce pidfd_getfd syscall
From: kbuild test robot @ 2019-12-26 22:20 UTC (permalink / raw)
To: Sargun Dhillon
Cc: kbuild-all, linux-kernel, containers, linux-api, linux-fsdevel,
tycho, jannh, cyphar, christian.brauner, oleg, luto, viro,
gpascutto, ealvarez, fweimer, jld, arnd
In-Reply-To: <20191226180334.GA29409@ircssh-2.c.rugged-nimbus-611.internal>
[-- Attachment #1: Type: text/plain, Size: 1383 bytes --]
Hi Sargun,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on kselftest/next]
[also build test ERROR on linus/master v5.5-rc3]
[cannot apply to tip/x86/asm next-20191220]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Sargun-Dhillon/Add-pidfd_getfd-syscall/20191227-025151
base: https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git next
config: alpha-defconfig (attached as .config)
compiler: alpha-linux-gcc (GCC) 7.5.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.5.0 make.cross ARCH=alpha
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
arch/alpha/kernel/systbls.o: In function `sys_call_table':
>> (.data+0x1120): undefined reference to `sys_pidfd'
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 13327 bytes --]
^ permalink raw reply
* Re: [PATCH v3 0/8] Rework random blocking
From: Andy Lutomirski @ 2019-12-26 23:29 UTC (permalink / raw)
To: Theodore Y. Ts'o
Cc: Stephan Mueller, Andy Lutomirski, LKML, Linux API, Kees Cook,
Jason A. Donenfeld, Ahmed S. Darwish, Lennart Poettering,
Eric W. Biederman, Alexander E. Patrakov, Michael Kerrisk,
Willy Tarreau, Matthew Garrett, Ext4 Developers List, linux-man
In-Reply-To: <20191226140423.GB3158@mit.edu>
>> On Dec 26, 2019, at 10:04 PM, Theodore Y. Ts'o <tytso@mit.edu> wrote:
>>
>> On Thu, Dec 26, 2019 at 01:03:34PM +0100, Stephan Mueller wrote:
>> Agreed. I was just trying to outline that the removal of the blocking_pool is
>> a good thing. Even when we decide that random.c should receive a TRNG, we do
>> not need to re-add a blocking pool, but can easily use the existing ChaCha20
>> DRNG (most likely with its own instance).
>
> Well, it depends on what you mean by "TRNG" --- the ChaCha20 DRNG only
> has a state of 256 bits. So if you want to only depend on "true
> entropy" you can't extract more than 256 bits without violating that
> assumption, at least if you're using a very strict definition of TRNG.
>
> By getting rid of the blocking pool, and making /dev/random work like
> getrandom with flags set to 0, we're effectively abandoning any kind
> of assertion that /dev/random is some kind of TRNG. This is not
> insane; this is what the *BSD's have always done.
>
> But once we do this, and /dev/random takes on the semantics of "block
> until the CRNG has been initialized, and then it won't block after
> that", if we change it so that it now has some different semantics,
> such as "one you extract a 256-bit key, the read from /dev/random will
> block until we can refill it, which might take seconds, minutes or
> hours", will be considered a regression, and we can't do that.
I don’t think Stephan was proposing that. He was proposing a way to implement a new interface that blocks.
>
> Of course, we can hope that people will be using getrandom() and there
> will be very few new users of the /dev/random pathname. But nothing
> is ever guaranteed..
>
> - Ted
^ permalink raw reply
* Re: [PATCH v7 2/3] pid: Introduce pidfd_getfd syscall
From: Sargun Dhillon @ 2019-12-27 1:35 UTC (permalink / raw)
To: kbuild test robot
Cc: kbuild-all, LKML, Linux Containers, Linux API,
Linux FS-devel Mailing List, Tycho Andersen, Jann Horn,
Aleksa Sarai, Christian Brauner, Oleg Nesterov, Andy Lutomirski,
Al Viro, Gian-Carlo Pascutto, Emilio Cobos Álvarez,
Florian Weimer, Jed Davis, Arnd Bergmann
In-Reply-To: <201912270545.TQnRs9kG%lkp@intel.com>
On Thu, Dec 26, 2019 at 5:20 PM kbuild test robot <lkp@intel.com> wrote:
>
> Hi Sargun,
>
> Thank you for the patch! Yet something to improve:
>
> All errors (new ones prefixed by >>):
>
> arch/alpha/kernel/systbls.o: In function `sys_call_table':
> >> (.data+0x1120): undefined reference to `sys_pidfd'
This is a small typo. I'll fix this in the next respin.
>
> ---
> 0-DAY kernel test infrastructure Open Source Technology Center
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation
^ permalink raw reply
* Re: [PATCH] seccomp: Check flags on seccomp_notif is unset
From: Aleksa Sarai @ 2019-12-27 2:24 UTC (permalink / raw)
To: Christian Brauner
Cc: Sargun Dhillon, linux-kernel, linux-api, tycho, jannh, keescook
In-Reply-To: <57C06925-0CC6-4251-AD57-8FF1BC28F049@ubuntu.com>
[-- Attachment #1: Type: text/plain, Size: 3564 bytes --]
On 2019-12-26, Christian Brauner <christian.brauner@ubuntu.com> wrote:
> On December 26, 2019 3:32:29 PM GMT+01:00, Aleksa Sarai <cyphar@cyphar.com> wrote:
> >On 2019-12-26, Christian Brauner <christian.brauner@ubuntu.com> wrote:
> >> On Wed, Dec 25, 2019 at 09:45:33PM +0000, Sargun Dhillon wrote:
> >> > This patch is a small change in enforcement of the uapi for
> >> > SECCOMP_IOCTL_NOTIF_RECV ioctl. Specificaly, the datastructure
> >which is
> >> > passed (seccomp_notif), has a flags member. Previously that could
> >be
> >> > set to a nonsense value, and we would ignore it. This ensures that
> >> > no flags are set.
> >> >
> >> > Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> >> > Cc: Kees Cook <keescook@chromium.org>
> >>
> >> I'm fine with this since we soon want to make use of the flag
> >argument
> >> when we add a flag to get a pidfd from the seccomp notifier on
> >receive.
> >> The major users I could identify already pass in seccomp_notif with
> >all
> >> fields set to 0. If we really break users we can always revert; this
> >> seems very unlikely to me though.
> >>
> >> One more question below, otherwise:
> >>
> >> Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com>
> >>
> >> > ---
> >> > kernel/seccomp.c | 7 +++++++
> >> > 1 file changed, 7 insertions(+)
> >> >
> >> > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> >> > index 12d2227e5786..455925557490 100644
> >> > --- a/kernel/seccomp.c
> >> > +++ b/kernel/seccomp.c
> >> > @@ -1026,6 +1026,13 @@ static long seccomp_notify_recv(struct
> >seccomp_filter *filter,
> >> > struct seccomp_notif unotif;
> >> > ssize_t ret;
> >> >
> >> > + if (copy_from_user(&unotif, buf, sizeof(unotif)))
> >> > + return -EFAULT;
> >> > +
> >> > + /* flags is reserved right now, make sure it's unset */
> >> > + if (unotif.flags)
> >> > + return -EINVAL;
> >> > +
> >>
> >> Might it make sense to use
> >>
> >> err = copy_struct_from_user(&unotif, sizeof(unotif), buf,
> >sizeof(unotif));
> >> if (err)
> >> return err;
> >>
> >> This way we check that the whole struct is 0 and report an error as
> >soon
> >> as one of the members is non-zero. That's more drastic but it'd
> >ensure
> >> that other fields can be used in the future for whatever purposes.
> >> It would also let us get rid of the memset() below.
> >
> >Given that this isn't an extensible struct, it would be simpler to just
> >do
> >check_zeroed_user() -- copy_struct_from_user() is overkill. That would
> >also remove the need for any copy_from_user()s and the memset can be
> >dropped by just doing
> >
> > struct seccomp_notif unotif = {};
> >
> >> > memset(&unotif, 0, sizeof(unotif));
> >> >
> >> > ret = down_interruptible(&filter->notif->request);
> >> > --
> >> > 2.20.1
> >> >
>
> It is an extensible struct. That's why we have notifier size checking built in.
Ah right, NOTIF_GET_SIZES. I reckon check_zeroed_user() is still a bit
simpler since none of the fields are used right now (and really, this
patch should be checking all of them, not just ->flags, if we want to
use any of them in the future).
But sure, copy_struct_from_user() also makes sense since it is
extensible (though I personally do find the whole NOTIF_GET_SIZES thing
a bit scary -- but that's water under the bridge at this point, and as
long as userspace is clever enough it shouldn't be a problem).
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH] seccomp: Check flags on seccomp_notif is unset
From: Aleksa Sarai @ 2019-12-27 2:28 UTC (permalink / raw)
To: Tycho Andersen
Cc: Christian Brauner, Sargun Dhillon, linux-kernel, linux-api, jannh,
keescook
In-Reply-To: <20191226153753.GA15663@cisco>
[-- Attachment #1: Type: text/plain, Size: 3248 bytes --]
On 2019-12-26, Tycho Andersen <tycho@tycho.ws> wrote:
> On Fri, Dec 27, 2019 at 01:32:29AM +1100, Aleksa Sarai wrote:
> > On 2019-12-26, Christian Brauner <christian.brauner@ubuntu.com> wrote:
> > > On Wed, Dec 25, 2019 at 09:45:33PM +0000, Sargun Dhillon wrote:
> > > > This patch is a small change in enforcement of the uapi for
> > > > SECCOMP_IOCTL_NOTIF_RECV ioctl. Specificaly, the datastructure which is
> > > > passed (seccomp_notif), has a flags member. Previously that could be
> > > > set to a nonsense value, and we would ignore it. This ensures that
> > > > no flags are set.
> > > >
> > > > Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> > > > Cc: Kees Cook <keescook@chromium.org>
> > >
> > > I'm fine with this since we soon want to make use of the flag argument
> > > when we add a flag to get a pidfd from the seccomp notifier on receive.
> > > The major users I could identify already pass in seccomp_notif with all
> > > fields set to 0. If we really break users we can always revert; this
> > > seems very unlikely to me though.
> > >
> > > One more question below, otherwise:
> > >
> > > Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com>
> > >
> > > > ---
> > > > kernel/seccomp.c | 7 +++++++
> > > > 1 file changed, 7 insertions(+)
> > > >
> > > > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> > > > index 12d2227e5786..455925557490 100644
> > > > --- a/kernel/seccomp.c
> > > > +++ b/kernel/seccomp.c
> > > > @@ -1026,6 +1026,13 @@ static long seccomp_notify_recv(struct seccomp_filter *filter,
> > > > struct seccomp_notif unotif;
> > > > ssize_t ret;
> > > >
> > > > + if (copy_from_user(&unotif, buf, sizeof(unotif)))
> > > > + return -EFAULT;
> > > > +
> > > > + /* flags is reserved right now, make sure it's unset */
> > > > + if (unotif.flags)
> > > > + return -EINVAL;
> > > > +
> > >
> > > Might it make sense to use
> > >
> > > err = copy_struct_from_user(&unotif, sizeof(unotif), buf, sizeof(unotif));
> > > if (err)
> > > return err;
> > >
> > > This way we check that the whole struct is 0 and report an error as soon
> > > as one of the members is non-zero. That's more drastic but it'd ensure
> > > that other fields can be used in the future for whatever purposes.
> > > It would also let us get rid of the memset() below.
> >
> > Given that this isn't an extensible struct, it would be simpler to just do
> > check_zeroed_user() -- copy_struct_from_user() is overkill. That would
> > also remove the need for any copy_from_user()s and the memset can be
> > dropped by just doing
> >
> > struct seccomp_notif unotif = {};
>
> This doesn't zero the padding according to the C standard, so no, you
> can't drop the memset, or you may leak kernel stack bits.
Ah right, I didn't double-check if there was any un-named. IMHO, It's a
bit odd to have un-named padding in a struct intended for extensions
(specifically to avoid these problems -- because it means userspace will
pass garbage by accident and there's nothing we can do about it). But
it's a bit late to worry about that now. :P
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH] seccomp: Check flags on seccomp_notif is unset
From: Aleksa Sarai @ 2019-12-27 2:31 UTC (permalink / raw)
To: Christian Brauner
Cc: Sargun Dhillon, linux-kernel, linux-api, tycho, jannh, keescook
In-Reply-To: <20191227022446.37e64ag4uaqms2w4@yavin.dot.cyphar.com>
[-- Attachment #1: Type: text/plain, Size: 4071 bytes --]
On 2019-12-27, Aleksa Sarai <cyphar@cyphar.com> wrote:
> On 2019-12-26, Christian Brauner <christian.brauner@ubuntu.com> wrote:
> > On December 26, 2019 3:32:29 PM GMT+01:00, Aleksa Sarai <cyphar@cyphar.com> wrote:
> > >On 2019-12-26, Christian Brauner <christian.brauner@ubuntu.com> wrote:
> > >> On Wed, Dec 25, 2019 at 09:45:33PM +0000, Sargun Dhillon wrote:
> > >> > This patch is a small change in enforcement of the uapi for
> > >> > SECCOMP_IOCTL_NOTIF_RECV ioctl. Specificaly, the datastructure
> > >which is
> > >> > passed (seccomp_notif), has a flags member. Previously that could
> > >be
> > >> > set to a nonsense value, and we would ignore it. This ensures that
> > >> > no flags are set.
> > >> >
> > >> > Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> > >> > Cc: Kees Cook <keescook@chromium.org>
> > >>
> > >> I'm fine with this since we soon want to make use of the flag
> > >argument
> > >> when we add a flag to get a pidfd from the seccomp notifier on
> > >receive.
> > >> The major users I could identify already pass in seccomp_notif with
> > >all
> > >> fields set to 0. If we really break users we can always revert; this
> > >> seems very unlikely to me though.
> > >>
> > >> One more question below, otherwise:
> > >>
> > >> Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com>
> > >>
> > >> > ---
> > >> > kernel/seccomp.c | 7 +++++++
> > >> > 1 file changed, 7 insertions(+)
> > >> >
> > >> > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> > >> > index 12d2227e5786..455925557490 100644
> > >> > --- a/kernel/seccomp.c
> > >> > +++ b/kernel/seccomp.c
> > >> > @@ -1026,6 +1026,13 @@ static long seccomp_notify_recv(struct
> > >seccomp_filter *filter,
> > >> > struct seccomp_notif unotif;
> > >> > ssize_t ret;
> > >> >
> > >> > + if (copy_from_user(&unotif, buf, sizeof(unotif)))
> > >> > + return -EFAULT;
> > >> > +
> > >> > + /* flags is reserved right now, make sure it's unset */
> > >> > + if (unotif.flags)
> > >> > + return -EINVAL;
> > >> > +
> > >>
> > >> Might it make sense to use
> > >>
> > >> err = copy_struct_from_user(&unotif, sizeof(unotif), buf,
> > >sizeof(unotif));
> > >> if (err)
> > >> return err;
> > >>
> > >> This way we check that the whole struct is 0 and report an error as
> > >soon
> > >> as one of the members is non-zero. That's more drastic but it'd
> > >ensure
> > >> that other fields can be used in the future for whatever purposes.
> > >> It would also let us get rid of the memset() below.
> > >
> > >Given that this isn't an extensible struct, it would be simpler to just
> > >do
> > >check_zeroed_user() -- copy_struct_from_user() is overkill. That would
> > >also remove the need for any copy_from_user()s and the memset can be
> > >dropped by just doing
> > >
> > > struct seccomp_notif unotif = {};
> > >
> > >> > memset(&unotif, 0, sizeof(unotif));
> > >> >
> > >> > ret = down_interruptible(&filter->notif->request);
> > >> > --
> > >> > 2.20.1
> > >> >
> >
> > It is an extensible struct. That's why we have notifier size checking built in.
>
> Ah right, NOTIF_GET_SIZES. I reckon check_zeroed_user() is still a bit
> simpler since none of the fields are used right now (and really, this
> patch should be checking all of them, not just ->flags, if we want to
> use any of them in the future).
Scratch that -- as Tycho just mentioned, there is un-named padding in
the struct so check_zeroed_user() is the wrong thing to do. But this
also will make extensions harder to deal with because (presumably) they
will also have un-named padding, making copy_struct_from_user() the
wrong thing to do as well.
So while there's not much to be done to fix the current struct layout, I
humbly suggest that any future struct extensions should not have any
un-named padding (so that at the very least you could use
copy_struct_from_user() in some form).
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v3 0/8] Rework random blocking
From: Stephan Mueller @ 2019-12-27 9:55 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Andy Lutomirski, Ted Ts'o, LKML, Linux API, Kees Cook,
Jason A. Donenfeld, Ahmed S. Darwish, Lennart Poettering,
Eric W. Biederman, Alexander E. Patrakov, Michael Kerrisk,
Willy Tarreau, Matthew Garrett, Ext4 Developers List, linux-man
In-Reply-To: <E464D551-1631-4EA8-ADFB-852E16D29CA9@amacapital.net>
Am Donnerstag, 26. Dezember 2019, 13:46:52 CET schrieb Andy Lutomirski:
Hi Andy,
> > On Dec 26, 2019, at 8:04 PM, Stephan Mueller <smueller@chronox.de> wrote:
> >
> > Am Donnerstag, 26. Dezember 2019, 12:12:29 CET schrieb Andy Lutomirski:
> >
> > Hi Andy,
> >
> >>>> On Dec 26, 2019, at 5:29 PM, Stephan Müller <smueller@chronox.de>
wrote:
> >>> Am Montag, 23. Dezember 2019, 09:20:43 CET schrieb Andy Lutomirski:
> >>>
> >>> Hi Andy,
> >>>
> >>>> There are some open questions and future work here:
> >>>>
> >>>> Should the kernel provide an interface to get software-generated
> >>>> "true random" numbers? I can think of only one legitimate reason to
> >>>> use such an interface: compliance with government standards. If the
> >>>> kernel provides such an interface going forward, I think it should
> >>>> be a brand new character device, and it should have a default mode
> >>>> 0440 or similar. Software-generated "true random numbers" are a
> >>>> very limited resource, and resource exhaustion is a big deal. Ask
> >>>> anyone who has twiddled their thumbs while waiting for gnupg to
> >>>> generate a key. If we think the kernel might do such a thing, then
> >>>> patches 5-8 could be tabled for now.
> >>>
> >>> What about offering a compile-time option to enable or disable such
> >>> code?
> >>> Note, with the existing random.c code base, there is no need to have a
> >>> separate blocking_pool. The ChaCha20 DRNG could be used for that very
> >>> same
> >>> purpose, provided that in case these true random numbers are generated
> >>> when
> >>> the Chacha20 DRNG received an equal amount of "unused" entropy.
> >>
> >> This scares me. The DRNG should be simple and easy to understand. If
> >> we’re
> >> tapping extra numbers in some weird way, then I would be more comfortable
> >> with some clear assurance that this doesn’t break the security. If we’re
> >> tapping numbers in the same way as normal urandom, then I don’t really
> >> see
> >> the point.
> >
> > Agreed. I was just trying to outline that the removal of the blocking_pool
> > is a good thing. Even when we decide that random.c should receive a TRNG,
> > we do not need to re-add a blocking pool, but can easily use the existing
> > ChaCha20 DRNG (most likely with its own instance).
>
> Fair enough.
>
> >>>> Alternatively, perhaps the kernel should instead provide a
> >>>> privileged interface to read out raw samples from the various
> >>>> entropy sources, and users who care could have a user daemon that
> >>>> does something intelligent with them. This would push the mess of
> >>>> trying to comply with whatever standards are involved to userspace.
> >>>> Userspace could then export "true randomness" via CUSE if it is so
> >>>> inclined, or could have a socket with a well-known name, or whatever
> >>>> else seems appropriate.
> >>>
> >>> With the patch set v26 of my LRNG I offer another possible alternative
> >>> avoiding any additional character device file and preventing the
> >>> starvation of legitimate use cases: the LRNG has an entropy pool that
> >>> leaves different levels of entropy in the pool depending on the use
> >>> cases
> >>> of this data.
> >>>
> >>> If an unprivileged caller requests true random data, at least 1024 bits
> >>> of
> >>> entropy is left in the pool. I.e. all entropy above that point is
> >>> available
> >>> for this request type. Note, even namespaces fall into this category
> >>> considering that unprivileged users can create a user name space in
> >>> which
> >>> they can become root.
> >>
> >> This doesn’t solve the problem. If two different users run stupid
> >> programs
> >> like gnupg, they will starve each other.
> >
> > But such scenario will always occur, will it not? If there are two callers
> > for a limited resource, they will content if one "over-uses" the
> > resource. My idea was to provide an interface where its use does not
> > starve other more relevant use cases (e.g. seeding of the DRNGs). I.e. a
> > user of a TRNG has the right to be DoSed - that is the price to pay when
> > using this concept.
>
> Maybe I’m just cynical, but I expect that, if the feature is available to
> everyone, then lots of user programmers will use it even though they don’t
> need to. If, on the other hand, there is a barrier to entry, then people
> will be more likely to stop and think.
I would tend to agree with you. But if the man page provides an appropriate
warning that such DoS is the price to pay, wouldn't you say it is sufficient?
I guess you will say no :-)
Thus, if you can convince Greg to allow us creating another device node, I am
definitely not in the way of creating and using it. All my suggestions simply
try to bridge the gap between Greg's rather reluctant agreement and our needs.
The good thing is IMHO that we are only talking about the actual kernel/user
interface. The plumbing behind it will be identical - at least in my LRNG
implementation, I can use the very same handler function for accessing the
TRNG with either the device file or the getrandom syscall. I guess the same
would be applicable to any possible random.c TRNG implementation.
>
> Even gnupg could have been more clever — when generating a 4096-bit RSA key,
> there is no actual need for 4096 bits of entropy, however entropy is
> defined. 256 bits would have been more than adequate.
I am in violent agreement.
>
> (FWIW, my personal view is that 512 bits, in the sense of “the distribution
> being sampled produces no output with probability greater than about
> 2^-512”, is a good upper limit for even the most paranoid. This is because
> it’s reasonable to assume that an attacker can’t do more than 2^128
> operations. As djb has noted, multi-target attacks mean that you can
> amplify success probability in some cases by a factor that won’t exceed
> 2^128. Some day, quantum computers might square-root everything, giving
> 512 bits. Actually, quantum computers won’t square root everything, but
> much more complicated analysis is needed to get a believable bound.)
>
> —Andy
Ciao
Stephan
^ permalink raw reply
* Re: [PATCH v3 0/8] Rework random blocking
From: Stephan Mueller @ 2019-12-27 10:29 UTC (permalink / raw)
To: Andy Lutomirski, Theodore Y. Ts'o
Cc: Andy Lutomirski, LKML, Linux API, Kees Cook, Jason A. Donenfeld,
Ahmed S. Darwish, Lennart Poettering, Eric W. Biederman,
Alexander E. Patrakov, Michael Kerrisk, Willy Tarreau,
Matthew Garrett, Ext4 Developers List, linux-man
In-Reply-To: <26B7EEAE-1166-4B45-9534-E00C5B2767C1@amacapital.net>
Am Freitag, 27. Dezember 2019, 00:29:20 CET schrieb Andy Lutomirski:
Hi Ted, Andy,
> >> On Dec 26, 2019, at 10:04 PM, Theodore Y. Ts'o <tytso@mit.edu> wrote:
> >>
> >> On Thu, Dec 26, 2019 at 01:03:34PM +0100, Stephan Mueller wrote:
> >> Agreed. I was just trying to outline that the removal of the
> >> blocking_pool is a good thing. Even when we decide that random.c should
> >> receive a TRNG, we do not need to re-add a blocking pool, but can easily
> >> use the existing ChaCha20 DRNG (most likely with its own instance).
> >
> > Well, it depends on what you mean by "TRNG" --- the ChaCha20 DRNG only
> > has a state of 256 bits. So if you want to only depend on "true
> > entropy" you can't extract more than 256 bits without violating that
> > assumption, at least if you're using a very strict definition of TRNG.
My definition of TRNG is identical to the German AIS 31 and I guess identical
to your definition of a TRNG.
A TRNG will produce an amount of random data that is equal to the amount of
"fresh" entropy that was provided by the noise source. I.e. it should be
identical to the blocking_pool behavior.
This definition is slightly stricter than the SP800-90A definition of "a DRBG
with prediction resistance" which requires a reseed with entropy equal to the
security strength of the DRBG, but allows one generate operation which at most
generates 2^19 random bits.
Such TRNG has two components
1. the noise source / the entropy pool
2. the random number generator
All I try to say is that the random number generator does not need to be a
special implementation of, say, a blocking_pool, but it can be any type of
DRNG (ChaCha20, SP800-90A DRBG, ...).
To manage that DRNG, the logic needs to ensure that the maximum entropy
content assumed to be present in the DRNG is min(entropy_from_noise_source,
security_strength_DRNG). For the case of the blocking_pool, the security
strength is 1024 bits which means that at most the blocking_pool can hold up
to 1024 bits. With a ChaCha20 DRNG, the security strength is 256 bits.
SP800-90A defines the security strengths of the DRBGs.
That said, for a TRNG, the DRNG part must be seeded with the amount of entropy
equaling the requested numbers of random bits, but at most with entropy
equaling the security strength of the DRNG. If the caller wants more random
data, the request must be chunked to ensure that the DRNG is always reseeded
before satisfying the chunk of the request.
> >
> > By getting rid of the blocking pool, and making /dev/random work like
> > getrandom with flags set to 0, we're effectively abandoning any kind
> > of assertion that /dev/random is some kind of TRNG. This is not
> > insane; this is what the *BSD's have always done.
Correct, and I am not disputing it. And I think that making Linux to behave
like the BSD's and guaranteeing that the DRNG is fully seeded based on Andy's
patch set is a good thing.
All I try to say is that there are use cases where a TRNG with the initially
defined operation is required. This most prominent use case is the German AIS
31 and the (re)seeding requirements of deterministic RNGs.
> >
> > But once we do this, and /dev/random takes on the semantics of "block
> > until the CRNG has been initialized, and then it won't block after
> > that", if we change it so that it now has some different semantics,
> > such as "one you extract a 256-bit key, the read from /dev/random will
> > block until we can refill it, which might take seconds, minutes or
> > hours", will be considered a regression, and we can't do that.
>
> I don’t think Stephan was proposing that. He was proposing a way to
> implement a new interface that blocks.
Thank you, Andy. Yes. I am trying to propose a separate interface.
Our discussion currently produced the following suggestions:
- add a new GRND_TRUERANDOM flag to getrandom(2) which allows access to the
TRNG. Andy did not like it because he mentioned that it may be misused since
the syscall is unprivileged. I had some suggestions to overcome this problem,
but not all of Andy's considerations could be addressed with this suggestion.
As an idea, my current LRNG system call implementation looks like:
SYSCALL_DEFINE3(getrandom, char __user *, buf, size_t, count,
unsigned int, flags)
{
if (flags & ~(GRND_NONBLOCK|GRND_RANDOM|GRND_INSECURE|
GRND_TRUERANDOM))
return -EINVAL;
/*
* Requesting insecure and blocking randomness at the same time makes
* no sense.
*/
if ((flags &
(GRND_INSECURE|GRND_RANDOM)) == (GRND_INSECURE|GRND_RANDOM))
return -EINVAL;
/* Only allow GRND_TRUERANDOM by itself or with NONBLOCK */
if ((flags & GRND_TRUERANDOM) &&
((flags &~ GRND_TRUERANDOM) != 0) &&
((flags &~ (GRND_TRUERANDOM | GRND_NONBLOCK)) != 0))
return -EINVAL;
if (count > INT_MAX)
count = INT_MAX;
if (flags & GRND_TRUERANDOM)
return lrng_read_common_block(flags & GRND_NONBLOCK, buf,
count, lrng_trng_get);
if (flags & GRND_INSECURE)
return lrng_sdrng_read(NULL, buf, count, NULL);
return lrng_read_common_block(flags & GRND_NONBLOCK, buf, count,
lrng_sdrng_get_sleep);
}
- Andy mentioned that he likes the approach with having another new char
device with permissions 440 to provide an interface to the TRNG as more
appropriate. However, Greg was reluctant to add a new device file.
I personally am indifferent. All I am suggesting is to have a TRNG offered to
user space.
> > Of course, we can hope that people will be using getrandom() and there
> > will be very few new users of the /dev/random pathname. But nothing
> > is ever guaranteed..
> >
> > - Ted
Ciao
Stephan
^ permalink raw reply
* Re: [PATCH] seccomp: Check flags on seccomp_notif is unset
From: Christian Brauner @ 2019-12-27 11:47 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Sargun Dhillon, linux-kernel, linux-api, tycho, jannh, keescook
In-Reply-To: <20191227023131.klnobtlfgeqcmvbb@yavin.dot.cyphar.com>
On Fri, Dec 27, 2019 at 01:31:31PM +1100, Aleksa Sarai wrote:
> On 2019-12-27, Aleksa Sarai <cyphar@cyphar.com> wrote:
> > On 2019-12-26, Christian Brauner <christian.brauner@ubuntu.com> wrote:
> > > On December 26, 2019 3:32:29 PM GMT+01:00, Aleksa Sarai <cyphar@cyphar.com> wrote:
> > > >On 2019-12-26, Christian Brauner <christian.brauner@ubuntu.com> wrote:
> > > >> On Wed, Dec 25, 2019 at 09:45:33PM +0000, Sargun Dhillon wrote:
> > > >> > This patch is a small change in enforcement of the uapi for
> > > >> > SECCOMP_IOCTL_NOTIF_RECV ioctl. Specificaly, the datastructure
> > > >which is
> > > >> > passed (seccomp_notif), has a flags member. Previously that could
> > > >be
> > > >> > set to a nonsense value, and we would ignore it. This ensures that
> > > >> > no flags are set.
> > > >> >
> > > >> > Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> > > >> > Cc: Kees Cook <keescook@chromium.org>
> > > >>
> > > >> I'm fine with this since we soon want to make use of the flag
> > > >argument
> > > >> when we add a flag to get a pidfd from the seccomp notifier on
> > > >receive.
> > > >> The major users I could identify already pass in seccomp_notif with
> > > >all
> > > >> fields set to 0. If we really break users we can always revert; this
> > > >> seems very unlikely to me though.
> > > >>
> > > >> One more question below, otherwise:
> > > >>
> > > >> Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com>
> > > >>
> > > >> > ---
> > > >> > kernel/seccomp.c | 7 +++++++
> > > >> > 1 file changed, 7 insertions(+)
> > > >> >
> > > >> > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> > > >> > index 12d2227e5786..455925557490 100644
> > > >> > --- a/kernel/seccomp.c
> > > >> > +++ b/kernel/seccomp.c
> > > >> > @@ -1026,6 +1026,13 @@ static long seccomp_notify_recv(struct
> > > >seccomp_filter *filter,
> > > >> > struct seccomp_notif unotif;
> > > >> > ssize_t ret;
> > > >> >
> > > >> > + if (copy_from_user(&unotif, buf, sizeof(unotif)))
> > > >> > + return -EFAULT;
> > > >> > +
> > > >> > + /* flags is reserved right now, make sure it's unset */
> > > >> > + if (unotif.flags)
> > > >> > + return -EINVAL;
> > > >> > +
> > > >>
> > > >> Might it make sense to use
> > > >>
> > > >> err = copy_struct_from_user(&unotif, sizeof(unotif), buf,
> > > >sizeof(unotif));
> > > >> if (err)
> > > >> return err;
> > > >>
> > > >> This way we check that the whole struct is 0 and report an error as
> > > >soon
> > > >> as one of the members is non-zero. That's more drastic but it'd
> > > >ensure
> > > >> that other fields can be used in the future for whatever purposes.
> > > >> It would also let us get rid of the memset() below.
> > > >
> > > >Given that this isn't an extensible struct, it would be simpler to just
> > > >do
> > > >check_zeroed_user() -- copy_struct_from_user() is overkill. That would
> > > >also remove the need for any copy_from_user()s and the memset can be
> > > >dropped by just doing
> > > >
> > > > struct seccomp_notif unotif = {};
> > > >
> > > >> > memset(&unotif, 0, sizeof(unotif));
> > > >> >
> > > >> > ret = down_interruptible(&filter->notif->request);
> > > >> > --
> > > >> > 2.20.1
> > > >> >
> > >
> > > It is an extensible struct. That's why we have notifier size checking built in.
> >
> > Ah right, NOTIF_GET_SIZES. I reckon check_zeroed_user() is still a bit
> > simpler since none of the fields are used right now (and really, this
> > patch should be checking all of them, not just ->flags, if we want to
> > use any of them in the future).
>
> Scratch that -- as Tycho just mentioned, there is un-named padding in
> the struct so check_zeroed_user() is the wrong thing to do. But this
Hm, I don't think so.
I understood Tycho's point as _if_ there ever is padding then this would
not be zeroed.
Right now, there is no padding since the struct is correctly padded:
struct seccomp_data {
int nr;
__u32 arch;
__u64 instruction_pointer;
__u64 args[6];
};
struct seccomp_notif {
__u64 id;
__u32 pid;
__u32 flags;
struct seccomp_data data;
};
which would be - using pahole:
struct seccomp_data {
int nr; /* 0 4 */
__u32 arch; /* 4 4 */
__u64 instruction_pointer; /* 8 8 */
__u64 args[6]; /* 16 48 */
/* size: 64, cachelines: 1, members: 4 */
};
struct seccomp_notif {
__u64 id; /* 0 8 */
__u32 pid; /* 8 4 */
__u32 flags; /* 12 4 */
struct seccomp_data data; /* 16 64 */
/* size: 80, cachelines: 2, members: 4 */
/* last cacheline: 16 bytes */
};
The only worry would be a 2byte int type but there's no architecture
we support which does this right now afaict.
> also will make extensions harder to deal with because (presumably) they
> will also have un-named padding, making copy_struct_from_user() the
This all will be a non-issue if we just use __u64 for extensions.
My point about using copy_struct_from_user() was that we should verify
that _all_ fields are uninitialized and not just the flags argument
since we might introduce a flags argument that requires another already
existing member in seccomp_notif to be set to a value. We should do this
change now so we don't have to risk breaking someone in the future.
I'm trying to get at least Mozilla/Firefox off of their crazy
SECCOMP_RET_TRAP way of implementing their broker onto the user notifier
and they will likely need some extensions. That includes the pidfd stuff
for seccomp that Sargun will likely be doing and the new pidfd_getfd()
syscall. So it's not unlikely that we might need other already existing
fields in that struct to be set to some value.
I don't particulary care how we do it:
- We can do a simple copy_from_user() and check each field individually.
- Use copy_struct_from_user().
That is safe to do right now since there is no padding afaict and
it'll automatically verify new fields as well.
If I understand the worry correctly then the argument against
copy_struct_from_user() here is that there might be padding introduced
and userspace will not do an explicit memset() but rather rely on an
empty inializer {} and will _accidently_ pass down a struct which has
__all fields cleared__ but __uninitialized padding__ and we tell them
EINVAL? That can only happen if we introduce padding in the struct
which I'd argue we just don't do. That'll be in line with what we
require from our ABIs already anyway.
Christian
^ permalink raw reply
* Re: [PATCH v3 0/8] Rework random blocking
From: Theodore Y. Ts'o @ 2019-12-27 13:04 UTC (permalink / raw)
To: Stephan Mueller
Cc: Andy Lutomirski, Andy Lutomirski, LKML, Linux API, Kees Cook,
Jason A. Donenfeld, Ahmed S. Darwish, Lennart Poettering,
Eric W. Biederman, Alexander E. Patrakov, Michael Kerrisk,
Willy Tarreau, Matthew Garrett, Ext4 Developers List, linux-man
In-Reply-To: <4048434.Q8HajmOrkZ@tauon.chronox.de>
On Fri, Dec 27, 2019 at 11:29:22AM +0100, Stephan Mueller wrote:
>
> My definition of TRNG is identical to the German AIS 31 and I guess identical
> to your definition of a TRNG.
>
> A TRNG will produce an amount of random data that is equal to the amount of
> "fresh" entropy that was provided by the noise source. I.e. it should be
> identical to the blocking_pool behavior.
This begs the question of determining: (a) how much "fresh entropy"
you can actually get from a noise source, (b) at what rate the "fresh
entropy" is arriving, and (c) what assurance(s) you have that the
noise source is actually working correctly.
You can't make those assurances from software alone; it needs to be an
aspect of holistic design of the hardware's design; the supply chain,
and the software. So if we are going to claime that we have something
like GRND_TRUERANDOM or /dev/trandom, or whatever, it needs to work on
IOT devices running ARM, RISC-V, MIPS, PowerPC, x86. Some of these
architectures have no instruction reordering and are stupid simple;
some of these hardware platforms may have no high-resolution clock or
cryptographic instructions.
In addition, if you use a hardware device which is USB attached, how
does the kernel know that it really is the device that you think it
is? The only way you know that a ChaosKey is a ChaosKey is by its USB
vendor and product id --- which can be easily forged by an attacker,
either in the supply chain or delivery path, or who walks up to the
laptop, yanks out the ChaosKey and replaces it with a "PutinKey" or a
"NSAKey".
So creating somethinig which shows up as "true random number
generator" as a generic Linux concept seems to me to be fraught
endeavor, and I'm not at all convince people need it.
> - add a new GRND_TRUERANDOM flag to getrandom(2) which allows access to the
> TRNG. Andy did not like it because he mentioned that it may be misused since
> the syscall is unprivileged.
Even if we could solve the "how the hell can the kernel guarantee that
the noise source is legitimate" problem in a general way that works
across all of the architectures, we still have the problem that
everyone thinks they need "the good stuff".
Suppose the system call was privileged and "true randomness" could
only be accessed as root. What would happen? Application programmers
would give instructions requiring that their application be installed
as root to be more secure, "because that way you can get access the
_really_ good random numbers".
So let's take a step back and ask the question: "Exactly what _value_
do you want to provide by creating some kind of true random
interface?" What does this enable? What applications does this
really help?
As I thought while watching the latest Star Wars movie: Why? Why?
Whywhywhy?
- Ted
^ permalink raw reply
* Re: [PATCH] seccomp: Check flags on seccomp_notif is unset
From: Sargun Dhillon @ 2019-12-27 14:22 UTC (permalink / raw)
To: Christian Brauner
Cc: Aleksa Sarai, LKML, Linux API, Tycho Andersen, Jann Horn,
Kees Cook
In-Reply-To: <20191227114725.xsacnaoaaxdv6yg3@wittgenstein>
On Fri, Dec 27, 2019 at 6:47 AM Christian Brauner
<christian.brauner@ubuntu.com> wrote:
>
> On Fri, Dec 27, 2019 at 01:31:31PM +1100, Aleksa Sarai wrote:
> > On 2019-12-27, Aleksa Sarai <cyphar@cyphar.com> wrote:
> >
> > Scratch that -- as Tycho just mentioned, there is un-named padding in
> > the struct so check_zeroed_user() is the wrong thing to do. But this
>
> Hm, I don't think so.
> I understood Tycho's point as _if_ there ever is padding then this would
> not be zeroed.
> Right now, there is no padding since the struct is correctly padded:
>
> struct seccomp_data {
> int nr;
> __u32 arch;
> __u64 instruction_pointer;
> __u64 args[6];
> };
>
> struct seccomp_notif {
> __u64 id;
> __u32 pid;
> __u32 flags;
> struct seccomp_data data;
> };
>
> which would be - using pahole:
>
> struct seccomp_data {
> int nr; /* 0 4 */
> __u32 arch; /* 4 4 */
> __u64 instruction_pointer; /* 8 8 */
> __u64 args[6]; /* 16 48 */
>
> /* size: 64, cachelines: 1, members: 4 */
> };
> struct seccomp_notif {
> __u64 id; /* 0 8 */
> __u32 pid; /* 8 4 */
> __u32 flags; /* 12 4 */
> struct seccomp_data data; /* 16 64 */
>
> /* size: 80, cachelines: 2, members: 4 */
> /* last cacheline: 16 bytes */
> };
>
> The only worry would be a 2byte int type but there's no architecture
> we support which does this right now afaict.
>
> > also will make extensions harder to deal with because (presumably) they
> > will also have un-named padding, making copy_struct_from_user() the
>
> This all will be a non-issue if we just use __u64 for extensions.
>
> My point about using copy_struct_from_user() was that we should verify
> that _all_ fields are uninitialized and not just the flags argument
> since we might introduce a flags argument that requires another already
> existing member in seccomp_notif to be set to a value. We should do this
> change now so we don't have to risk breaking someone in the future.
>
> I'm trying to get at least Mozilla/Firefox off of their crazy
> SECCOMP_RET_TRAP way of implementing their broker onto the user notifier
> and they will likely need some extensions. That includes the pidfd stuff
> for seccomp that Sargun will likely be doing and the new pidfd_getfd()
> syscall. So it's not unlikely that we might need other already existing
> fields in that struct to be set to some value.
>
> I don't particulary care how we do it:
> - We can do a simple copy_from_user() and check each field individually.
Just doing a simple copy_from_user, and for now, calling memchr_inv
on the whole thing. We can drop the memset, and just leave a note to
indicate that if unpadded fields are introduced in the future, this structure
must be manually zeroed out. Although, this might be laying a trap for
ourselves.
This leaves us in a good position for introducing a flag field in the future.
All we have to do is change the memchr_inv from checking on an
entire struct basis to checking on a per-field basis.
> - Use copy_struct_from_user().
> That is safe to do right now since there is no padding afaict and
> it'll automatically verify new fields as well.
> If I understand the worry correctly then the argument against
> copy_struct_from_user() here is that there might be padding introduced
> and userspace will not do an explicit memset() but rather rely on an
> empty inializer {} and will _accidently_ pass down a struct which has
> __all fields cleared__ but __uninitialized padding__ and we tell them
> EINVAL? That can only happen if we introduce padding in the struct
> which I'd argue we just don't do. That'll be in line with what we
> require from our ABIs already anyway.
>
> Christian
^ permalink raw reply
* Re: [PATCH] seccomp: Check flags on seccomp_notif is unset
From: Tycho Andersen @ 2019-12-27 14:38 UTC (permalink / raw)
To: Sargun Dhillon
Cc: Christian Brauner, Aleksa Sarai, LKML, Linux API, Jann Horn,
Kees Cook
In-Reply-To: <CAMp4zn8iMsRvDoDtrotfnEm2_UUULH9VRiR6q9u8CS4qham2Eg@mail.gmail.com>
On Fri, Dec 27, 2019 at 09:22:20AM -0500, Sargun Dhillon wrote:
> Just doing a simple copy_from_user, and for now, calling memchr_inv
> on the whole thing. We can drop the memset, and just leave a note to
> indicate that if unpadded fields are introduced in the future, this structure
> must be manually zeroed out. Although, this might be laying a trap for
> ourselves.
Yes, please keep the memset().
Tycho
^ permalink raw reply
* Re: [PATCH] seccomp: Check flags on seccomp_notif is unset
From: Aleksa Sarai @ 2019-12-27 15:15 UTC (permalink / raw)
To: Sargun Dhillon
Cc: Christian Brauner, LKML, Linux API, Tycho Andersen, Jann Horn,
Kees Cook
In-Reply-To: <CAMp4zn8iMsRvDoDtrotfnEm2_UUULH9VRiR6q9u8CS4qham2Eg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 4713 bytes --]
On 2019-12-27, Sargun Dhillon <sargun@sargun.me> wrote:
> On Fri, Dec 27, 2019 at 6:47 AM Christian Brauner
> <christian.brauner@ubuntu.com> wrote:
> >
> > On Fri, Dec 27, 2019 at 01:31:31PM +1100, Aleksa Sarai wrote:
> > > On 2019-12-27, Aleksa Sarai <cyphar@cyphar.com> wrote:
> > >
> > > Scratch that -- as Tycho just mentioned, there is un-named padding in
> > > the struct so check_zeroed_user() is the wrong thing to do. But this
> >
> > Hm, I don't think so.
> > I understood Tycho's point as _if_ there ever is padding then this would
> > not be zeroed.
> > Right now, there is no padding since the struct is correctly padded:
> >
> > struct seccomp_data {
> > int nr;
> > __u32 arch;
> > __u64 instruction_pointer;
> > __u64 args[6];
> > };
> >
> > struct seccomp_notif {
> > __u64 id;
> > __u32 pid;
> > __u32 flags;
> > struct seccomp_data data;
> > };
> >
> > which would be - using pahole:
> >
> > struct seccomp_data {
> > int nr; /* 0 4 */
> > __u32 arch; /* 4 4 */
> > __u64 instruction_pointer; /* 8 8 */
> > __u64 args[6]; /* 16 48 */
> >
> > /* size: 64, cachelines: 1, members: 4 */
> > };
> > struct seccomp_notif {
> > __u64 id; /* 0 8 */
> > __u32 pid; /* 8 4 */
> > __u32 flags; /* 12 4 */
> > struct seccomp_data data; /* 16 64 */
> >
> > /* size: 80, cachelines: 2, members: 4 */
> > /* last cacheline: 16 bytes */
> > };
> >
> > The only worry would be a 2byte int type but there's no architecture
> > we support which does this right now afaict.
> >
> > > also will make extensions harder to deal with because (presumably) they
> > > will also have un-named padding, making copy_struct_from_user() the
> >
> > This all will be a non-issue if we just use __u64 for extensions.
> >
> > My point about using copy_struct_from_user() was that we should verify
> > that _all_ fields are uninitialized and not just the flags argument
> > since we might introduce a flags argument that requires another already
> > existing member in seccomp_notif to be set to a value. We should do this
> > change now so we don't have to risk breaking someone in the future.
> >
> > I'm trying to get at least Mozilla/Firefox off of their crazy
> > SECCOMP_RET_TRAP way of implementing their broker onto the user notifier
> > and they will likely need some extensions. That includes the pidfd stuff
> > for seccomp that Sargun will likely be doing and the new pidfd_getfd()
> > syscall. So it's not unlikely that we might need other already existing
> > fields in that struct to be set to some value.
> >
> > I don't particulary care how we do it:
> > - We can do a simple copy_from_user() and check each field individually.
>
> Just doing a simple copy_from_user, and for now, calling memchr_inv
> on the whole thing. We can drop the memset, and just leave a note to
> indicate that if unpadded fields are introduced in the future, this structure
> must be manually zeroed out. Although, this might be laying a trap for
> ourselves.
>
> This leaves us in a good position for introducing a flag field in the future.
> All we have to do is change the memchr_inv from checking on an
> entire struct basis to checking on a per-field basis.
There is no need to do memchr_inv() on copy_from_user() to check for
zero-ness. That's the entire point of check_zeroed_user() -- to not need
to do it that way.
> > - Use copy_struct_from_user().
> > That is safe to do right now since there is no padding afaict and
> > it'll automatically verify new fields as well.
> > If I understand the worry correctly then the argument against
> > copy_struct_from_user() here is that there might be padding introduced
> > and userspace will not do an explicit memset() but rather rely on an
> > empty inializer {} and will _accidently_ pass down a struct which has
> > __all fields cleared__ but __uninitialized padding__ and we tell them
> > EINVAL? That can only happen if we introduce padding in the struct
> > which I'd argue we just don't do. That'll be in line with what we
> > require from our ABIs already anyway.
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH] seccomp: Check flags on seccomp_notif is unset
From: Christian Brauner @ 2019-12-27 15:32 UTC (permalink / raw)
To: Aleksa Sarai, Sargun Dhillon
Cc: LKML, Linux API, Tycho Andersen, Jann Horn, Kees Cook
In-Reply-To: <20191227151501.osy2m6o6p6odzk74@yavin.dot.cyphar.com>
On December 27, 2019 4:15:01 PM GMT+01:00, Aleksa Sarai <cyphar@cyphar.com> wrote:
>On 2019-12-27, Sargun Dhillon <sargun@sargun.me> wrote:
>> On Fri, Dec 27, 2019 at 6:47 AM Christian Brauner
>> <christian.brauner@ubuntu.com> wrote:
>> >
>> > On Fri, Dec 27, 2019 at 01:31:31PM +1100, Aleksa Sarai wrote:
>> > > On 2019-12-27, Aleksa Sarai <cyphar@cyphar.com> wrote:
>> > >
>> > > Scratch that -- as Tycho just mentioned, there is un-named
>padding in
>> > > the struct so check_zeroed_user() is the wrong thing to do. But
>this
>> >
>> > Hm, I don't think so.
>> > I understood Tycho's point as _if_ there ever is padding then this
>would
>> > not be zeroed.
>> > Right now, there is no padding since the struct is correctly
>padded:
>> >
>> > struct seccomp_data {
>> > int nr;
>> > __u32 arch;
>> > __u64 instruction_pointer;
>> > __u64 args[6];
>> > };
>> >
>> > struct seccomp_notif {
>> > __u64 id;
>> > __u32 pid;
>> > __u32 flags;
>> > struct seccomp_data data;
>> > };
>> >
>> > which would be - using pahole:
>> >
>> > struct seccomp_data {
>> > int nr; /* 0
> 4 */
>> > __u32 arch; /* 4
> 4 */
>> > __u64 instruction_pointer; /* 8
> 8 */
>> > __u64 args[6]; /* 16
> 48 */
>> >
>> > /* size: 64, cachelines: 1, members: 4 */
>> > };
>> > struct seccomp_notif {
>> > __u64 id; /* 0
> 8 */
>> > __u32 pid; /* 8
> 4 */
>> > __u32 flags; /* 12
> 4 */
>> > struct seccomp_data data; /* 16
> 64 */
>> >
>> > /* size: 80, cachelines: 2, members: 4 */
>> > /* last cacheline: 16 bytes */
>> > };
>> >
>> > The only worry would be a 2byte int type but there's no
>architecture
>> > we support which does this right now afaict.
>> >
>> > > also will make extensions harder to deal with because
>(presumably) they
>> > > will also have un-named padding, making copy_struct_from_user()
>the
>> >
>> > This all will be a non-issue if we just use __u64 for extensions.
>> >
>> > My point about using copy_struct_from_user() was that we should
>verify
>> > that _all_ fields are uninitialized and not just the flags argument
>> > since we might introduce a flags argument that requires another
>already
>> > existing member in seccomp_notif to be set to a value. We should do
>this
>> > change now so we don't have to risk breaking someone in the future.
>> >
>> > I'm trying to get at least Mozilla/Firefox off of their crazy
>> > SECCOMP_RET_TRAP way of implementing their broker onto the user
>notifier
>> > and they will likely need some extensions. That includes the pidfd
>stuff
>> > for seccomp that Sargun will likely be doing and the new
>pidfd_getfd()
>> > syscall. So it's not unlikely that we might need other already
>existing
>> > fields in that struct to be set to some value.
>> >
>> > I don't particulary care how we do it:
>> > - We can do a simple copy_from_user() and check each field
>individually.
>>
>> Just doing a simple copy_from_user, and for now, calling memchr_inv
>> on the whole thing. We can drop the memset, and just leave a note to
>> indicate that if unpadded fields are introduced in the future, this
>structure
>> must be manually zeroed out. Although, this might be laying a trap
>for
>> ourselves.
>>
>> This leaves us in a good position for introducing a flag field in the
>future.
>> All we have to do is change the memchr_inv from checking on an
>> entire struct basis to checking on a per-field basis.
>
>There is no need to do memchr_inv() on copy_from_user() to check for
>zero-ness. That's the entire point of check_zeroed_user() -- to not
>need
>to do it that way.
Right, we added that too a while ago.
Let's use it.
Christian
^ permalink raw reply
* Re: [PATCH v3 0/8] Rework random blocking
From: Stephan Mueller @ 2019-12-27 21:22 UTC (permalink / raw)
To: Theodore Y. Ts'o
Cc: Andy Lutomirski, Andy Lutomirski, LKML, Linux API, Kees Cook,
Jason A. Donenfeld, Ahmed S. Darwish, Lennart Poettering,
Eric W. Biederman, Alexander E. Patrakov, Michael Kerrisk,
Willy Tarreau, Matthew Garrett, Ext4 Developers List, linux-man
In-Reply-To: <20191227130436.GC70060@mit.edu>
Am Freitag, 27. Dezember 2019, 14:04:36 CET schrieb Theodore Y. Ts'o:
Hi Theodore,
> On Fri, Dec 27, 2019 at 11:29:22AM +0100, Stephan Mueller wrote:
> > My definition of TRNG is identical to the German AIS 31 and I guess
> > identical to your definition of a TRNG.
> >
> > A TRNG will produce an amount of random data that is equal to the amount
> > of
> > "fresh" entropy that was provided by the noise source. I.e. it should be
> > identical to the blocking_pool behavior.
>
> This begs the question of determining: (a) how much "fresh entropy"
> you can actually get from a noise source, (b) at what rate the "fresh
> entropy" is arriving, and (c) what assurance(s) you have that the
> noise source is actually working correctly.
>
> You can't make those assurances from software alone; it needs to be an
> aspect of holistic design of the hardware's design; the supply chain,
> and the software. So if we are going to claime that we have something
> like GRND_TRUERANDOM or /dev/trandom, or whatever, it needs to work on
> IOT devices running ARM, RISC-V, MIPS, PowerPC, x86. Some of these
> architectures have no instruction reordering and are stupid simple;
> some of these hardware platforms may have no high-resolution clock or
> cryptographic instructions.
>
> In addition, if you use a hardware device which is USB attached, how
> does the kernel know that it really is the device that you think it
> is? The only way you know that a ChaosKey is a ChaosKey is by its USB
> vendor and product id --- which can be easily forged by an attacker,
> either in the supply chain or delivery path, or who walks up to the
> laptop, yanks out the ChaosKey and replaces it with a "PutinKey" or a
> "NSAKey".
>
> So creating somethinig which shows up as "true random number
> generator" as a generic Linux concept seems to me to be fraught
> endeavor, and I'm not at all convince people need it.
I am unsure but it sounds like you are refuting your blocking_pool
implementation. Nothing more and nothing less than the blocking_pool, just
with a more modern and further analyzed DRNG is what was referenced as a TRNG.
Or maybe the terminology of TRNG (i.e. "true") is offending. I have no concern
to have it replaced with some other terminology. Yet, I was just taking one
well-defined term.
Yet, I fully agree that a noise source always must be vetted. This is what I
tried with random.c in [1], specifically section 6.1 for x86 systems.
For my LRNG, I tried that in [2] section 3.2 compliant to SP800-90B. In order
to provide a means to everybody to perform such entropy analysis, the entire
tool set required for it is provided:
- with CONFIG_LRNG_TESTING providing an interface to the raw unconditioned
noise
- with [3] providing a tool set to gather all data needed for an SP800-90B
compliant quantitative analysis
Unfortunately due to license restrictions, I cannot make the same tool set
available used for the quantiative study provided with [1] section 6.1.
Finally, to support the conclusions drawn from a noise source analysis, the
health tests provided with LRNG compliant to SP800-90B are available with
CONFIG_LRNG_HEALTH_TESTS. These tests help in identifying weak or broken noise
sources.
It is fully clear that such studies of non-physical noise sources do not have
a stochastical model which implies that we cannot make global statements. That
is the limitation when using such noise sources. Though, the implementation
should have sufficient "leeway" (i.e. underestmation) when crediting entropy
to some events.
>
> > - add a new GRND_TRUERANDOM flag to getrandom(2) which allows access to
> > the
> > TRNG. Andy did not like it because he mentioned that it may be misused
> > since the syscall is unprivileged.
>
> Even if we could solve the "how the hell can the kernel guarantee that
> the noise source is legitimate" problem in a general way that works
> across all of the architectures, we still have the problem that
> everyone thinks they need "the good stuff".
>
> Suppose the system call was privileged and "true randomness" could
> only be accessed as root. What would happen? Application programmers
> would give instructions requiring that their application be installed
> as root to be more secure, "because that way you can get access the
> _really_ good random numbers".
That is why I think that it is no bug when this interface can DoS other users
wanting to access the very same resource. This is the price to pay for getting
access to this type of data.
>
> So let's take a step back and ask the question: "Exactly what _value_
> do you want to provide by creating some kind of true random
> interface?" What does this enable? What applications does this
> really help?
There are simply cryptographers who have use cases for such random numbers.
The core use case is to seed other DRNGs and avoiding the chaining of free-
running DRNGs.
This is a common approach that you can see in action with the RDSEED
instruction, for example.
>
> As I thought while watching the latest Star Wars movie: Why? Why?
> Whywhywhy?
>
> - Ted
[1] https://www.bsi.bund.de/SharedDocs/Downloads/EN/BSI/Publications/Studies/
LinuxRNG/LinuxRNG_EN.pdf?__blob=publicationFile&v=11
[2] https://chronox.de/lrng/doc/lrng.pdf
[3] https://chronox.de/lrng/lrng-tests-20191123.tar.xz
Ciao
Stephan
^ permalink raw reply
* Re: [PATCH v3 0/8] Rework random blocking
From: Theodore Y. Ts'o @ 2019-12-27 22:08 UTC (permalink / raw)
To: Stephan Mueller
Cc: Andy Lutomirski, Andy Lutomirski, LKML, Linux API, Kees Cook,
Jason A. Donenfeld, Ahmed S. Darwish, Lennart Poettering,
Eric W. Biederman, Alexander E. Patrakov, Michael Kerrisk,
Willy Tarreau, Matthew Garrett, Ext4 Developers List, linux-man
In-Reply-To: <15817620.rmTN4T87Wr@tauon.chronox.de>
On Fri, Dec 27, 2019 at 10:22:23PM +0100, Stephan Mueller wrote:
>
> I am unsure but it sounds like you are refuting your blocking_pool
> implementation. Nothing more and nothing less than the blocking_pool, just
> with a more modern and further analyzed DRNG is what was referenced as a TRNG.
Yes, and that's why I am planning on taking Andy's patches to drop the
blocking pool. Trying to make the claim that you can read one byte
from /dev/random if and only if one byte of entropy has flowed into
it.... is a mug's game, for the reasons I gave above.
> Or maybe the terminology of TRNG (i.e. "true") is offending. I have no concern
> to have it replaced with some other terminology. Yet, I was just taking one
> well-defined term.
But my point is that it *isn't* a well defined term, precisely because
it's completely unclear what application programmer can expect when
they try to use some hypothetical GRANDOM_TRUERANDOM flag. What does
that *mean*? The kernel can't offer up any guarantees about whether
or not the noise source has been appropriately characterized. All
say, a GPG or OpenSSL developer can do is get the vague sense that
TRUERANDOM is "better" and of course, they want the best security, so
of *course* they are going to try to use it. At which point it will
block, and when some other clever user (maybe a distro release
engineer) puts it into an init script, then systems will stop working
and users will complain to Linus.
And then we'll have companies like Intel claiming that RDSEED has been
very carefully characterized --- by them --- and we should *obviously*
trust it, and wire up RDSEED so that TRUERANDOM will have a near
infinite supply of really good entropy. And they might even be
correct. But this way lies a huge mess which is fundamentally social,
not technical.
The claim we can make for getrandom(2) is that we do the best job that
we can, and we feed in as many sources as possible and hope that at
least one or more sources is not known to the attacker. One of the
sources could very well be AES(NSA_KEY, SEQ++). But that still will
protect us from the Chinese and Russian crypto teams. And we can hope
that the NSA doesn't have access to the inter-packet arrival times on
the local area network, or the radio strength as recorded from the
WiFi radio, etc. etc. But note that we didn't make any claims of how
many bits of entropy that we have; it helps that we are implicitly
making a claim that we trust the crypto algorithms.
> > So let's take a step back and ask the question: "Exactly what _value_
> > do you want to provide by creating some kind of true random
> > interface?" What does this enable? What applications does this
> > really help?
>
> There are simply cryptographers who have use cases for such random numbers.
> The core use case is to seed other DRNGs and avoiding the chaining of free-
> running DRNGs.
For this very specialized use case, what I think the kernel should
provide is maximal transparency; that is, given the DRBG direct access
to the TPM's random number generator, or direct access to the
ChaosKey, and the userspace DRBG should be able to get a list of the
various hardware RNG's, and select one, with the characterization
being done userspace, not in the kernel.
The kernel shouldn't be mixing various noise sources together, and it
certainly shouldn't be trying to claim that it knows how many bits of
entropy that it gets when is trying to play some jitter entropy game
on a stupid-simple CPU architecture for IOT/Embedded user cases where
everything is synchronized off of a single master oscillator, and
there is no CPU instruction reordering or register renaming, etc.,
etc.
You can talk about providing tools that try to make these estimations
--- but these sorts of things would have to be done on each user's
hardware, and for most distro users, it's just not practical.
So if it's just for cryptographers, then let it all be done in
userspace, and let's not make it easy for GPG, OpenSSL, etc., to all
say, "We want TrueRandom(tm); we won't settle for less". We can talk
about how do we provide the interfaces so that those cryptographers
can get the information they need so they can get access to the raw
noise sources, separated out and named, and with possibly some way
that the noise source can authenticate itself to the Cryptographer's
userspace library/application.
But all of this should probably not be in drivers/char/random.c, and
we probably need to figure out a better kernel to userspace interface
than what we have with /dev/hwrng.
- Ted
^ permalink raw reply
* [PATCH v2 1/2] samples, selftests/seccomp: Zero out seccomp_notif
From: Sargun Dhillon @ 2019-12-28 1:48 UTC (permalink / raw)
To: linux-kernel, linux-api; +Cc: tycho, jannh, christian.brauner, keescook, cyphar
The seccomp_notif structure should be zeroed out prior to calling the
SECCOMP_IOCTL_NOTIF_RECV ioctl. Previously, the kernel did not check
whether these structures were zeroed out or not, so these worked.
Signed-off-by: Sargun Dhillon <sargun@sargun.me>
Cc: Kees Cook <keescook@chromium.org>
---
samples/seccomp/user-trap.c | 2 +-
tools/testing/selftests/seccomp/seccomp_bpf.c | 2 ++
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/samples/seccomp/user-trap.c b/samples/seccomp/user-trap.c
index 6d0125ca8af7..0ca8fb37cd79 100644
--- a/samples/seccomp/user-trap.c
+++ b/samples/seccomp/user-trap.c
@@ -298,7 +298,6 @@ int main(void)
req = malloc(sizes.seccomp_notif);
if (!req)
goto out_close;
- memset(req, 0, sizeof(*req));
resp = malloc(sizes.seccomp_notif_resp);
if (!resp)
@@ -306,6 +305,7 @@ int main(void)
memset(resp, 0, sizeof(*resp));
while (1) {
+ memset(req, 0, sizes.seccomp_notif);
if (ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, req)) {
perror("ioctl recv");
goto out_resp;
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index 6944b898bb53..f53f14971bff 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -3278,6 +3278,7 @@ TEST(user_notification_signal)
close(sk_pair[1]);
+ memset(&req, 0, sizeof(req));
EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0);
EXPECT_EQ(kill(pid, SIGUSR1), 0);
@@ -3296,6 +3297,7 @@ TEST(user_notification_signal)
EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp), -1);
EXPECT_EQ(errno, ENOENT);
+ memset(&req, 0, sizeof(req));
EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0);
resp.id = req.id;
--
2.20.1
^ permalink raw reply related
* [PATCH v2 2/2] seccomp: Check that seccomp_notif is zeroed out by the user
From: Sargun Dhillon @ 2019-12-28 1:48 UTC (permalink / raw)
To: linux-kernel, linux-api; +Cc: tycho, jannh, christian.brauner, keescook, cyphar
This patch is a small change in enforcement of the uapi for
SECCOMP_IOCTL_NOTIF_RECV ioctl. Specifically, the datastructure which
is passed (seccomp_notif) must be zeroed out. Previously any of its
members could be set to nonsense values, and we would ignore it.
This ensures all fields are set to their zero value.
This relies on the seccomp_notif datastructure to not have
any unnamed padding, as it is valid to initialize the datastructure
as:
struct seccomp_notif notif = {};
This only initializes named members to their 0-value [1].
[1]: https://lore.kernel.org/lkml/20191227023131.klnobtlfgeqcmvbb@yavin.dot.cyphar.com/
Signed-off-by: Sargun Dhillon <sargun@sargun.me>
Cc: Kees Cook <keescook@chromium.org>
---
kernel/seccomp.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 12d2227e5786..4fd73cbdd01e 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -1026,6 +1026,12 @@ static long seccomp_notify_recv(struct seccomp_filter *filter,
struct seccomp_notif unotif;
ssize_t ret;
+ ret = check_zeroed_user(buf, sizeof(unotif));
+ if (ret < 0)
+ return ret;
+ if (!ret)
+ return -EINVAL;
+
memset(&unotif, 0, sizeof(unotif));
ret = down_interruptible(&filter->notif->request);
--
2.20.1
^ permalink raw reply related
* Re: [PATCH v2 2/2] seccomp: Check that seccomp_notif is zeroed out by the user
From: Aleksa Sarai @ 2019-12-28 2:06 UTC (permalink / raw)
To: Sargun Dhillon
Cc: linux-kernel, linux-api, tycho, jannh, christian.brauner,
keescook
In-Reply-To: <20191228014849.GA31783@ircssh-2.c.rugged-nimbus-611.internal>
[-- Attachment #1: Type: text/plain, Size: 1631 bytes --]
On 2019-12-28, Sargun Dhillon <sargun@sargun.me> wrote:
> This patch is a small change in enforcement of the uapi for
> SECCOMP_IOCTL_NOTIF_RECV ioctl. Specifically, the datastructure which
> is passed (seccomp_notif) must be zeroed out. Previously any of its
> members could be set to nonsense values, and we would ignore it.
>
> This ensures all fields are set to their zero value.
>
> This relies on the seccomp_notif datastructure to not have
> any unnamed padding, as it is valid to initialize the datastructure
> as:
>
> struct seccomp_notif notif = {};
>
> This only initializes named members to their 0-value [1].
>
> [1]: https://lore.kernel.org/lkml/20191227023131.klnobtlfgeqcmvbb@yavin.dot.cyphar.com/
>
> Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> Cc: Kees Cook <keescook@chromium.org>
Looks good.
Reviewed-by: Aleksa Sarai <cyphar@cyphar.com>
> ---
> kernel/seccomp.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index 12d2227e5786..4fd73cbdd01e 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -1026,6 +1026,12 @@ static long seccomp_notify_recv(struct seccomp_filter *filter,
> struct seccomp_notif unotif;
> ssize_t ret;
>
> + ret = check_zeroed_user(buf, sizeof(unotif));
> + if (ret < 0)
> + return ret;
> + if (!ret)
> + return -EINVAL;
> +
> memset(&unotif, 0, sizeof(unotif));
>
> ret = down_interruptible(&filter->notif->request);
> --
> 2.20.1
>
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v3 0/8] Rework random blocking
From: Andy Lutomirski @ 2019-12-28 2:06 UTC (permalink / raw)
To: Theodore Y. Ts'o
Cc: Stephan Mueller, Andy Lutomirski, LKML, Linux API, Kees Cook,
Jason A. Donenfeld, Ahmed S. Darwish, Lennart Poettering,
Eric W. Biederman, Alexander E. Patrakov, Michael Kerrisk,
Willy Tarreau, Matthew Garrett, Ext4 Developers List, linux-man
In-Reply-To: <20191227220857.GD70060@mit.edu>
On Fri, Dec 27, 2019 at 2:09 PM Theodore Y. Ts'o <tytso@mit.edu> wrote:
> So if it's just for cryptographers, then let it all be done in
> userspace, and let's not make it easy for GPG, OpenSSL, etc., to all
> say, "We want TrueRandom(tm); we won't settle for less". We can talk
> about how do we provide the interfaces so that those cryptographers
> can get the information they need so they can get access to the raw
> noise sources, separated out and named, and with possibly some way
> that the noise source can authenticate itself to the Cryptographer's
> userspace library/application.
>
> But all of this should probably not be in drivers/char/random.c, and
> we probably need to figure out a better kernel to userspace interface
> than what we have with /dev/hwrng.
I'm thinking of having a real class device and chardev for each hwrng
device. Authentication is entirely in userspace: whatever user code
is involved can look at the sysfs hierarchy and decide to what extent
it trusts a given source. This could be done based on bus topology or
based on anything else.
The kernel could also separately expose various noise sources, and the
user code can do whatever it wants with them. But these should be
explicitly unconditioned, un-entropy-extracted sources -- user code
can run its favorite algorithm to extract something it believes to be
useful. The only conceptually tricky bit is keeping user code like
this from interfering with the in-kernel RNG.
--Andy
^ permalink raw reply
* Re: [PATCH v2 2/2] seccomp: Check that seccomp_notif is zeroed out by the user
From: Tycho Andersen @ 2019-12-28 3:49 UTC (permalink / raw)
To: Sargun Dhillon
Cc: linux-kernel, linux-api, jannh, christian.brauner, keescook,
cyphar
In-Reply-To: <20191228014849.GA31783@ircssh-2.c.rugged-nimbus-611.internal>
On Sat, Dec 28, 2019 at 01:48:51AM +0000, Sargun Dhillon wrote:
> This patch is a small change in enforcement of the uapi for
> SECCOMP_IOCTL_NOTIF_RECV ioctl. Specifically, the datastructure which
> is passed (seccomp_notif) must be zeroed out. Previously any of its
> members could be set to nonsense values, and we would ignore it.
>
> This ensures all fields are set to their zero value.
>
> This relies on the seccomp_notif datastructure to not have
> any unnamed padding, as it is valid to initialize the datastructure
> as:
>
> struct seccomp_notif notif = {};
>
> This only initializes named members to their 0-value [1].
>
> [1]: https://lore.kernel.org/lkml/20191227023131.klnobtlfgeqcmvbb@yavin.dot.cyphar.com/
>
> Signed-off-by: Sargun Dhillon <sargun@sargun.me>
Acked-by: Tycho Andersen <tycho@tycho.ws>
^ permalink raw reply
* Re: [PATCH v3 0/8] Rework random blocking
From: Willy Tarreau @ 2019-12-28 7:01 UTC (permalink / raw)
To: Theodore Y. Ts'o
Cc: Stephan Mueller, Andy Lutomirski, Andy Lutomirski, LKML,
Linux API, Kees Cook, Jason A. Donenfeld, Ahmed S. Darwish,
Lennart Poettering, Eric W. Biederman, Alexander E. Patrakov,
Michael Kerrisk, Matthew Garrett, Ext4 Developers List, linux-man
In-Reply-To: <20191227220857.GD70060@mit.edu>
On Fri, Dec 27, 2019 at 05:08:57PM -0500, Theodore Y. Ts'o wrote:
> > Or maybe the terminology of TRNG (i.e. "true") is offending. I have no concern
> > to have it replaced with some other terminology. Yet, I was just taking one
> > well-defined term.
>
> But my point is that it *isn't* a well defined term, precisely because
> it's completely unclear what application programmer can expect when
> they try to use some hypothetical GRANDOM_TRUERANDOM flag. What does
> that *mean*?
I've also seen this term used and abused too many times and this bothers
me because the expectations around it are the cause of the current
situation.
Randomness doesn't exist by itself. It's what characterizes the
unpredictable nature of something. I.e. our inability to model it and
guess what will happen based on what we know. 200 years ago we'd have
considered the weather as a true random source. Now we have super
computers making this moot. In the current state of art we consider
that cesium decay or tunnel noise are unpredictable and excellent
random sources, until one day we figure that magnetic fields,
temperature or gamma rays strongly affect them.
So in practice we should only talk about the complexity of the model we
rely on. The more complex it is (i.e. the most independent variables it
relies on), the less predictable it is and the more random it is. Jitter
entropy and RAM contents are good examples of this: they may be highly
unpredictable on some platforms and almost constant on others. And for
sure, software cannot fix this, it can at best make the output *look*
like it's unpredictable. Once someone can model all variables of the
environment this is not true random anymore.
That's why the best we can do is to combine as many sources as possible
hoping that nobody can model enough of them, and produce an output which
never ever reveals these sources' internal states. *This* is what software
can and must do. And once the initial entropy is hidden enough and there
is enough of it, there's no reason for it to ever get depleted if these
initial bits cannot be guessed nor brute-forced.
And quite frankly I'd rather just speak about the diversity of sources
than "true" randomness. Just asking a user to press 10 random keys and
to enter a random word for some operations can break many assumptions
an attacker could have about the environment, by just adding one extra,
less controllable, source.
Just my two cents,
Willy
^ permalink raw reply
* Re: [PATCH v2 2/2] seccomp: Check that seccomp_notif is zeroed out by the user
From: Christian Brauner @ 2019-12-28 7:09 UTC (permalink / raw)
To: Sargun Dhillon; +Cc: linux-kernel, linux-api, tycho, jannh, keescook, cyphar
In-Reply-To: <20191228014849.GA31783@ircssh-2.c.rugged-nimbus-611.internal>
On Sat, Dec 28, 2019 at 01:48:51AM +0000, Sargun Dhillon wrote:
> This patch is a small change in enforcement of the uapi for
> SECCOMP_IOCTL_NOTIF_RECV ioctl. Specifically, the datastructure which
> is passed (seccomp_notif) must be zeroed out. Previously any of its
> members could be set to nonsense values, and we would ignore it.
>
> This ensures all fields are set to their zero value.
The upper part is correct and useful.
>
> This relies on the seccomp_notif datastructure to not have
> any unnamed padding, as it is valid to initialize the datastructure
> as:
>
> struct seccomp_notif notif = {};
The interesting part here is accidently leaking kernel addresses to
userspace. For this to be an issue we'd need to do
struct seccomp_notif unotif = {};
copy_to_user(<user-buffer>, &unotif, sizeof(unotif))
_and_ seccomp_notif would need to contain unintentional padding. Even if
the latter were true we still use memset() anwyay and will likely never
remove it. So the code here sure doesn't rely or depends on correct
padding at all.
>
> This only initializes named members to their 0-value [1].
>
> [1]: https://lore.kernel.org/lkml/20191227023131.klnobtlfgeqcmvbb@yavin.dot.cyphar.com/
That link isn't useful and also incorrectly claims that there is
non-intentional padding in the struct which there isn't.
Just drop that whole paragraph. The expectation is that all of our ABIs
are correctly padded anyway and this really just confuses more than it
helps.
Please resend, otherwise:
Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com>
But see a small comment below.
>
> Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> Cc: Kees Cook <keescook@chromium.org>
> ---
> kernel/seccomp.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index 12d2227e5786..4fd73cbdd01e 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -1026,6 +1026,12 @@ static long seccomp_notify_recv(struct seccomp_filter *filter,
> struct seccomp_notif unotif;
> ssize_t ret;
>
> + ret = check_zeroed_user(buf, sizeof(unotif));
It wouldn't hurt to place a small comment here so the reader can easily
spot we've ensured that this struct can be extended. But up to you...
/* Verify that we're not given garbage to keep struct extensible. */
> + if (ret < 0)
> + return ret;
> + if (!ret)
> + return -EINVAL;
> +
> memset(&unotif, 0, sizeof(unotif));
>
> ret = down_interruptible(&filter->notif->request);
> --
> 2.20.1
>
^ permalink raw reply
* Re: [PATCH v2 1/2] samples, selftests/seccomp: Zero out seccomp_notif
From: Christian Brauner @ 2019-12-28 9:25 UTC (permalink / raw)
To: Sargun Dhillon; +Cc: linux-kernel, linux-api, tycho, jannh, keescook, cyphar
In-Reply-To: <20191228014837.GA31774@ircssh-2.c.rugged-nimbus-611.internal>
On Sat, Dec 28, 2019 at 01:48:39AM +0000, Sargun Dhillon wrote:
> The seccomp_notif structure should be zeroed out prior to calling the
> SECCOMP_IOCTL_NOTIF_RECV ioctl. Previously, the kernel did not check
> whether these structures were zeroed out or not, so these worked.
>
> Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> Cc: Kees Cook <keescook@chromium.org>
Can you please also add a test, that verifies that we catch garbage
values, please?
^ permalink raw reply
* Re: [PATCH v7 2/3] pid: Introduce pidfd_getfd syscall
From: Christian Brauner @ 2019-12-28 10:11 UTC (permalink / raw)
To: Sargun Dhillon
Cc: linux-kernel, containers, linux-api, linux-fsdevel, tycho, jannh,
cyphar, oleg, luto, viro, gpascutto, ealvarez, fweimer, jld, arnd
In-Reply-To: <20191226180334.GA29409@ircssh-2.c.rugged-nimbus-611.internal>
On Thu, Dec 26, 2019 at 06:03:36PM +0000, Sargun Dhillon wrote:
> This syscall allows for the retrieval of file descriptors from other
> processes, based on their pidfd. This is possible using ptrace, and
> injection of parasitic code to inject code which leverages SCM_RIGHTS
> to move file descriptors between a tracee and a tracer. Unfortunately,
> ptrace comes with a high cost of requiring the process to be stopped,
> and breaks debuggers. This does not require stopping the process under
> manipulation.
>
> One reason to use this is to allow sandboxers to take actions on file
> descriptors on the behalf of another process. For example, this can be
> combined with seccomp-bpf's user notification to do on-demand fd
> extraction and take privileged actions. One such privileged action
> is binding a socket to a privileged port.
>
> This also adds the syscall to all architectures at the same time.
>
> /* prototype */
> /* flags is currently reserved and should be set to 0 */
> int sys_pidfd_getfd(int pidfd, int fd, unsigned int flags);
>
> /* testing */
> Ran self-test suite on x86_64
Fyi, I'm likely going to rewrite/add parts of/to this once I apply.
A few comments below.
> diff --git a/kernel/pid.c b/kernel/pid.c
> index 2278e249141d..4a551f947869 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -578,3 +578,106 @@ void __init pid_idr_init(void)
> init_pid_ns.pid_cachep = KMEM_CACHE(pid,
> SLAB_HWCACHE_ALIGN | SLAB_PANIC | SLAB_ACCOUNT);
> }
> +
> +static struct file *__pidfd_fget(struct task_struct *task, int fd)
> +{
> + struct file *file;
> + int ret;
> +
> + ret = mutex_lock_killable(&task->signal->cred_guard_mutex);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + if (!ptrace_may_access(task, PTRACE_MODE_ATTACH_REALCREDS)) {
> + file = ERR_PTR(-EPERM);
> + goto out;
> + }
> +
> + file = fget_task(task, fd);
> + if (!file)
> + file = ERR_PTR(-EBADF);
> +
> +out:
> + mutex_unlock(&task->signal->cred_guard_mutex);
> + return file;
> +}
Looking at this code now a bit closer, ptrace_may_access() and
fget_task() both take task_lock(task) so this currently does:
task_lock();
/* check access */
task_unlock();
task_lock();
/* get fd */
task_unlock();
which doesn't seem great.
I would prefer if we could do:
task_lock();
/* check access */
/* get fd */
task_unlock();
But ptrace_may_access() doesn't export an unlocked variant so _shrug_.
But we can write this a little cleaner without the goto as:
static struct file *__pidfd_fget(struct task_struct *task, int fd)
{
struct file *file;
int ret;
ret = mutex_lock_killable(&task->signal->cred_guard_mutex);
if (ret)
return ERR_PTR(ret);
if (ptrace_may_access(task, PTRACE_MODE_ATTACH_REALCREDS))
file = fget_task(task, fd);
else
file = ERR_PTR(-EPERM);
mutex_unlock(&task->signal->cred_guard_mutex);
return file ?: ERR_PTR(-EBADF);
}
If you don't like the ?: just do:
if (!file)
return ERR_PTR(-EBADF);
return file;
though I prefer the shorter ?: syntax which is perfect for shortcutting
returns.
> +
> +static int pidfd_getfd(struct pid *pid, int fd)
> +{
> + struct task_struct *task;
> + struct file *file;
> + int ret, retfd;
> +
> + task = get_pid_task(pid, PIDTYPE_PID);
> + if (!task)
> + return -ESRCH;
> +
> + file = __pidfd_fget(task, fd);
> + put_task_struct(task);
> + if (IS_ERR(file))
> + return PTR_ERR(file);
> +
> + retfd = get_unused_fd_flags(O_CLOEXEC);
> + if (retfd < 0) {
> + ret = retfd;
> + goto out;
> + }
> +
> + /*
> + * security_file_receive must come last since it may have side effects
> + * and cannot be reversed.
> + */
> + ret = security_file_receive(file);
So I don't understand the comment here. Can you explain what the side
effects are?
security_file_receive() is called in two places: net/core/scm.c and
net/compat.c. In both places it is called _before_ get_unused_fd_flags()
so I don't know what's special here that would prevent us from doing the
same. If there's no actual reason, please rewrite this functions as:
static int pidfd_getfd(struct pid *pid, int fd)
{
int ret;
struct task_struct *task;
struct file *file;
task = get_pid_task(pid, PIDTYPE_PID);
if (!task)
return -ESRCH;
file = __pidfd_fget(task, fd);
put_task_struct(task);
if (IS_ERR(file))
return PTR_ERR(file);
ret = security_file_receive(file);
if (ret) {
fput(file);
return ret;
}
ret = get_unused_fd_flags(O_CLOEXEC);
if (ret < 0)
fput(file);
else
fd_install(ret, file);
return ret;
}
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox