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: <CAAH4kHYNhiewBZ8Z_yF2F+ogkv_1UV8=Gu0KVDdTJN4iQKyNMg@mail.gmail.com>
Date:   Thu, 31 Aug 2023 14:42:56 -0700
From:   Dionna Amalie Glaze <dionnaglaze@...gle.com>
To:     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

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?

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."

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.

-- 
-Dionna Glaze, PhD (she/her)