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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAHmME9oGffi539nEAJkOjC-2yAR+0Ra+wQw5oG6Y6+CRr5e62g@mail.gmail.com>
Date:   Wed, 30 Nov 2022 16:47:25 +0100
From:   "Jason A. Donenfeld" <Jason@...c4.com>
To:     Arnd Bergmann <arnd@...db.de>
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

Hi Arnd,

On Wed, Nov 30, 2022 at 4:29 PM Arnd Bergmann <arnd@...db.de> wrote:
> > 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.

Except ideally, these are word-sized accesses (where only compat code
has to suffer I suppose).

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

The vvar page starts out zeroed, no?

Jason