linux-kernel - Re: [PATCH 1/3] lib, include/linux: add usercopy failure capability

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20200821133120.GA3145341@elver.google.com>
Date:   Fri, 21 Aug 2020 15:31:20 +0200
From:   Marco Elver <elver@...gle.com>
To:     Dmitry Vyukov <dvyukov@...gle.com>
Cc:     albert.linde@...il.com, Andrew Morton <akpm@...ux-foundation.org>,
        Borislav Petkov <bp@...en8.de>, Ingo Molnar <mingo@...hat.com>,
        Jonathan Corbet <corbet@....net>,
        Thomas Gleixner <tglx@...utronix.de>,
        Arnd Bergmann <arnd@...db.de>,
        Akinobu Mita <akinobu.mita@...il.com>,
        "H. Peter Anvin" <hpa@...or.com>,
        Al Viro <viro@...iv.linux.org.uk>,
        Alexander Potapenko <glider@...gle.com>,
        Andrey Konovalov <andreyknvl@...gle.com>,
        "open list:DOCUMENTATION" <linux-doc@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-arch <linux-arch@...r.kernel.org>,
        the arch/x86 maintainers <x86@...nel.org>,
        Albert van der Linde <alinde@...gle.com>
Subject: Re: [PATCH 1/3] lib, include/linux: add usercopy failure capability

On Fri, Aug 21, 2020 at 01:51PM +0200, Dmitry Vyukov wrote:
...
> > +++ b/lib/fault-inject-usercopy.c
> > @@ -0,0 +1,66 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +#include <linux/fault-inject.h>
> > +#include <linux/fault-inject-usercopy.h>
> > +#include <linux/random.h>
> > +
> > +static struct {
> > +       struct fault_attr attr;
> > +       u32 failsize;
> > +} fail_usercopy = {
> > +       .attr = FAULT_ATTR_INITIALIZER,
> > +       .failsize = 0,
> > +};
> > +
> > +static int __init setup_fail_usercopy(char *str)
> > +{
> > +       return setup_fault_attr(&fail_usercopy.attr, str);
> > +}
> > +__setup("fail_usercopy=", setup_fail_usercopy);
> > +
> > +#ifdef CONFIG_FAULT_INJECTION_DEBUG_FS
> > +
> > +static int __init fail_usercopy_debugfs(void)
> > +{
> > +       umode_t mode = S_IFREG | 0600;
> > +       struct dentry *dir;
> > +
> > +       dir = fault_create_debugfs_attr("fail_usercopy", NULL,
> > +                                       &fail_usercopy.attr);
> > +       if (IS_ERR(dir))
> > +               return PTR_ERR(dir);
> > +
> > +       debugfs_create_u32("failsize", mode, dir,
> > +                          &fail_usercopy.failsize);
> 
> Marco, what's the right way to annotate these concurrent accesses for KCSAN?

For debugfs variables that are accessed concurrently, the only
non-data-racy option (currently) is to use debugfs_create_atomic_t() and
make the variable an atomic_t.

If it's read-mostly as is the case here, and given that atomic_read() is
cheap (it maps to READ_ONCE on x86 and arm64), that'd be reasonable even
if performance is a concern.

> > +       return 0;
> > +}
> > +
> > +late_initcall(fail_usercopy_debugfs);
> > +
> > +#endif /* CONFIG_FAULT_INJECTION_DEBUG_FS */
> > +
> > +/**
> > + * should_fail_usercopy() - Failure code or amount of bytes not to copy.
> > + * @n: Size of the original copy call.
> > + *
> > + * The general idea is to have a method which returns the amount of bytes not
> > + * to copy, a failure to return, or 0 if the calling function should progress
> > + * without a failure. E.g., copy_{to,from}_user should NOT copy the amount of
> > + * bytes returned by should_fail_usercopy, returning this value (in addition
> > + * to any bytes that could actually not be copied) or a failure.
> > + *
> > + * Return: one of:
> > + * negative, failure to return;
> > + * 0, progress normally;
> > + * a number in ]0, n], the number of bytes not to copy.
> > + *
> > + */
> > +long should_fail_usercopy(unsigned long n)
> > +{
> > +       if (should_fail(&fail_usercopy.attr, n)) {
> > +               if (fail_usercopy.failsize > 0)
> > +                       return fail_usercopy.failsize % (n + 1);

If you wanted to retain the u32 in debugfs, you can mark this
'data_race(fail_usercopy.failsize)' -- since what we're doing here is
probabilistic anyway, reading a garbage value won't affect things much.

Alternatively, just switch to atomic_t and it'll just be an
atomic_read().

Thanks,
-- Marco