linux-kernel - Re: [PATCH v10 3/4] random: introduce generic vDSO getrandom() implementation

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]

Date:   Wed, 30 Nov 2022 16:29:26 +0100
From:   "Arnd Bergmann" <arnd@...db.de>
To:     "Jason A . Donenfeld" <Jason@...c4.com>
Cc:     "Florian Weimer" <fweimer@...hat.com>,
        linux-kernel@...r.kernel.org, patches@...ts.linux.dev,
        "Thomas Gleixner" <tglx@...utronix.de>,
        linux-crypto@...r.kernel.org, linux-api@...r.kernel.org,
        x86@...nel.org, "Greg Kroah-Hartman" <gregkh@...uxfoundation.org>,
        "Adhemerval Zanella Netto" <adhemerval.zanella@...aro.org>,
        "Carlos O'Donell" <carlos@...hat.com>,
        "Christian Brauner" <brauner@...nel.org>
Subject: Re: [PATCH v10 3/4] random: introduce generic vDSO getrandom() implementation

On Wed, Nov 30, 2022, at 16:12, Jason A. Donenfeld wrote:
> Hi Arnd,
>
> On Wed, Nov 30, 2022 at 4:07 PM Arnd Bergmann <arnd@...db.de> wrote:
>> > +#ifdef CONFIG_64BIT
>> > +typedef u64 vdso_kernel_ulong;
>> > +#else
>> > +typedef u32 vdso_kernel_ulong;
>> > +#endif
>>
>> This does not address the ABI concern: to allow 32-bit and 64-bit
>> tasks to share the same data page, it has to be the same width on
>> both, either u32 or 64, but not depending on a configuration
>> option.
>
> I think it does address the issue. CONFIG_64BIT is a .config setting,
> not a compiler-derived setting. So a 64-bit kernel will get a u64 in
> kernel mode, and then it will get a u64 for the 64-bit vdso usermode
> compile, and finally it will get a u64 for the 32-bit vdso usermode
> compile. So in all three cases, the size is the same.

I see what you mean now. However this means your vdso32 copies
are different between 32-bit and 64-bit kernels. If you need to
access one of the fields from assembler, it even ends up
different at source level, which adds a bit of complexity.

Making the interface configuration-independent makes it obvious
to the reader that none of these problems can happen.

>> > struct vdso_rng_data {
>> >       vdso_kernel_ulong       generation;
>> >       bool                    is_ready;
>> > };
>>
>> There is another problem with this: you have implicit padding
>> in the structure because the two members have different size
>> and alignment requirements. The easiest fix is to make them
>> both u64, or you could have a u32 is_ready and an explit u32
>> for the padding.
>
> There's padding at the end of the structure, yes. But both
> `generation` and `is_ready` will be at the same offset. If the
> structure grows, then sure, that'll have to be taken into account. But
> that's not a problem because this is a private implementation detail
> between the vdso code and the kernel.

I was not concerned about incompatibility here, but rather about
possibly leaking kernel data to the vdso page. Again, this probably
doesn't happen if your code is written correctly, but the rule for
kernel-user ABIs is to avoid implicit padding to ensure that
the padding bytes can never leak any information. Using structures
without padding at the minimum helps avoid having to think about
whether this can become a problem when inspecting the code for
possible issues, both from humans and from automated tools.

     Arnd