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 [off-list ref] wrote:

quoted

quoted

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

quoted

quoted

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