linux-kernel - Re: [PATCH v3 2/5] configfs-tsm: Introduce a shared ABI for attestation reports

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <64f11081985e_31c2db29440@dwillia2-xfh.jf.intel.com.notmuch>
Date:   Thu, 31 Aug 2023 15:13:21 -0700
From:   Dan Williams <dan.j.williams@...el.com>
To:     Dionna Amalie Glaze <dionnaglaze@...gle.com>,
        Dan Williams <dan.j.williams@...el.com>
CC:     <linux-coco@...ts.linux.dev>,
        Kuppuswamy Sathyanarayanan 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>,
        James Bottomley <James.Bottomley@...senpartnership.com>,
        Peter Gonda <pgonda@...gle.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Samuel Ortiz <sameo@...osinc.com>, <peterz@...radead.org>,
        <linux-kernel@...r.kernel.org>, <tglx@...utronix.de>
Subject: Re: [PATCH v3 2/5] configfs-tsm: Introduce a shared ABI for
 attestation reports

Dionna Amalie Glaze wrote:
> This is clean and approachable. Thanks for your implementation.
> 
> > +static int try_advance_write_generation(struct tsm_report *report)
> > +{
> > +       lockdep_assert_held_write(&tsm_rwsem);
> > +
> > +       /*
> > +        * malicious or broken userspace is attempting to wrap read_generation,
> > +        * stop accepting updates until current report configuration is read.
> > +        */
> > +       if (report->write_generation == report->read_generation - 1)
> > +               return -EBUSY;
> > +       report->write_generation++;
> > +       return 0;
> > +}
> > +
> 
> This took me a while to wrap my head around.
> The property we want is that when we read outblob, it is the result of
> the attribute changes since the last read. If we write to an attribute
> 2^64 times, we could get write_generation to wrap around to equal
> read_generation without actually reading outblob. So we could end up
> given a badly cached result when we read outblob? Is that what this is
> preventing?

Correct. The criticism of kernfs based interfaces like sysfs and
configfs is that there is no facility to atomically modify a set of
attributes at once. The expectated mitigation is simply that userspace
is well behaved. For example, 2 invocations of fdisk can confuse each
other, so userspace is expected to run them serially and the kernel
otherwise lets userspace do silly things.

If a tool has any concern that it has exclusive ownership of the
interface this 'generation' attribute allows for a flow like:

gen=$(cat $report/generation)
dd if=userdata > $report/inblob
cat $report/outblob > report
gen2=$(cat $report/generation)

...and if $gen2 is not $((gen + 1)) then tooling can detect that the
"userspace is well behaved" expectation was violated.

Now configs is slightly better in this regard because objects can be
atomically created. So if 2 threads happen to pick the same name for the
report object then 'mkdir' will only succeed for one while the other
gets an EEXIST error. So for containers each can get their own
submission interface without worrying about other containers.

> I think I would word this to say,
> 
> "Malicious or broken userspace has written enough times for
> read_generation == write_generation by modular arithmetic without an
> interim read. Stop accepting updates until the current report
> configuration is read."

That works for me.

> I think that at least in the SEV-SNP case, we can double-check from
> userspace that the report has values that we expected to configure the
> get_report with, so this isn't really an issue. I'm not sure what the
> use is of configuration that doesn't lead to observable (and
> checkable) differences, but I suppose this check doesn't hurt.

Right, the tool can also just double check that all the
parameters are the expected value in the output report, but if you want
to trust the kernel output without necessarily trusting other tools to
not stomp on your config item instance then 'generation' helps.