Re: [PATCH] random: vDSO: Redefine PAGE_SIZE and PAGE_MASK

[Vincenzo Frascino <vincenzo.frascino@arm.com>]

[-- Attachment #1: Type: text/plain, Size: 2865 bytes --]

Hi Christophe,

On 27/08/2024 11:49, Christophe Leroy wrote:

...


>>
>> These are still two headers outside of the vdso/ namespace. For arm64
>> we had concluded that this is never safe, and any vdso header should
>> only include other vdso headers so we never pull in anything that
>> e.g. depends on memory management headers that are in turn broken
>> for the compat vdso.
>>
>> The array_size.h header is really small, so that one could
>> probably just be moved into the vdso/ namespace. The minmax.h
>> header is already rather complex, so it may be better to just
>> open-code the usage of MIN/MAX where needed?
> 
> It is used at two places only so yes can to that.
>

Could you please clarify where minmax is needed? I tried to build Jason's master
tree for x86, commenting the header and it seems building fine. I might be
missing something.

> Same for ARRAY_SIZE(->reserved) by the way, easy to do opencode, we also have it
> only once
> 

I have a similar issue to figure out why linux/array_size.h and
uapi/linux/random.h are needed. It seems that I can build the object without
them. Could you please explain?

In general, the philosophy adopted to split the headers is to extract the set of
functions used by vDSOs from the linux headers and place them in the vdso headers.
Consequently the linux header includes to vdso one. E.g.: linux/time64.h
includes vdso/time64.h.

IMHO we should follow the same approach, if at all for consistency, unless there
is a valid reason.

...

>>
>> Including uapi/linux/mman.h may still be problematic on
>> some architectures if they change it in a way that is
>> incompatible with compat vdso, but at least that can't
>> accidentally rely on CONFIG_64BIT or something else that
>> would be wrong there.
> 
> Yes that one is tricky. Because uapi/linux/mman.h includes asm/mman.h with the
> intention to include uapi/asm/mman.h but when built from the kernel in reality
> you get arch/powerpc/include/asm/mman.h and I had to add some ifdefery to
> kick-out kernel oddities it contains that pull additional kernel headers.
> 
> diff --git a/arch/powerpc/include/asm/mman.h b/arch/powerpc/include/asm/mman.h
> index 17a77d47ed6d..42a51a993d94 100644
> --- a/arch/powerpc/include/asm/mman.h
> +++ b/arch/powerpc/include/asm/mman.h
> @@ -6,7 +6,7 @@
> 
>  #include <uapi/asm/mman.h>
> 
> -#ifdef CONFIG_PPC64
> +#if defined(CONFIG_PPC64) && !defined(BUILD_VDSO)
> 
>  #include <asm/cputable.h>
>  #include <linux/mm.h>
> 

I agree with Arnd here. uapi/linux/mman.h can cause us problems in the long run.

I am attaching a patch to provide my view on how to minimize the headers
included and use only the vdso/ namespace. Please, before using the code,
consider that I conducted very limited testing.

Note: It should apply clean on Jason's tree.

Let me know your thoughts.

> 
> Christophe

-- 
Regards,
Vincenzo

[-- Attachment #2: 0001-random-VDSO-Use-only-headers-from-the-vdso-namespace.patch --]
[-- Type: text/x-patch, Size: 5622 bytes --]

From 1933028eaeebc5c053309379c43ddea5b460c25e Mon Sep 17 00:00:00 2001
From: Vincenzo Frascino <vincenzo.frascino@arm.com>
Date: Tue, 27 Aug 2024 16:44:32 +0100
Subject: [PATCH] random: VDSO: Use only headers from the vdso/ namespace

The VDSO implementation includes headers from outside of the
vdso/ namespace.

Refactor the code to make sure that the generic library uses only the
allowed namespace.

Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
---
 arch/x86/include/asm/vdso/getrandom.h |  2 ++
 arch/x86/include/asm/vdso/mman.h      | 15 +++++++++++++++
 arch/x86/include/asm/vdso/page.h      | 15 +++++++++++++++
 include/vdso/getrandom.h              |  1 +
 include/vdso/mman.h                   |  7 +++++++
 include/vdso/page.h                   |  7 +++++++
 lib/vdso/getrandom.c                  | 20 ++++++--------------
 7 files changed, 53 insertions(+), 14 deletions(-)
 create mode 100644 arch/x86/include/asm/vdso/mman.h
 create mode 100644 arch/x86/include/asm/vdso/page.h
 create mode 100644 include/vdso/mman.h
 create mode 100644 include/vdso/page.h

diff --git a/arch/x86/include/asm/vdso/getrandom.h b/arch/x86/include/asm/vdso/getrandom.h
index b96e674cafde..457f237bd602 100644
--- a/arch/x86/include/asm/vdso/getrandom.h
+++ b/arch/x86/include/asm/vdso/getrandom.h
@@ -7,6 +7,8 @@
 
 #ifndef __ASSEMBLY__
 
+#include <vdso/datapage.h>
+
 #include <asm/unistd.h>
 #include <asm/vvar.h>
 
diff --git a/arch/x86/include/asm/vdso/mman.h b/arch/x86/include/asm/vdso/mman.h
new file mode 100644
index 000000000000..4c936c9d11ab
--- /dev/null
+++ b/arch/x86/include/asm/vdso/mman.h
@@ -0,0 +1,15 @@
+
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __ASM_VDSO_MMAN_H
+#define __ASM_VDSO_MMAN_H
+
+#ifndef __ASSEMBLY__
+
+#include <uapi/linux/mman.h>
+
+#define VDSO_MMAP_PROT	PROT_READ | PROT_WRITE
+#define VDSO_MMAP_FLAGS	MAP_DROPPABLE | MAP_ANONYMOUS
+
+#endif /* !__ASSEMBLY__ */
+
+#endif /* __ASM_VDSO_MMAN_H */
diff --git a/arch/x86/include/asm/vdso/page.h b/arch/x86/include/asm/vdso/page.h
new file mode 100644
index 000000000000..b0af8fbef27c
--- /dev/null
+++ b/arch/x86/include/asm/vdso/page.h
@@ -0,0 +1,15 @@
+
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __ASM_VDSO_PAGE_H
+#define __ASM_VDSO_PAGE_H
+
+#ifndef __ASSEMBLY__
+
+#include <asm/page_types.h>
+
+#define VDSO_PAGE_MASK	PAGE_MASK
+#define VDSO_PAGE_SIZE	PAGE_SIZE
+
+#endif /* !__ASSEMBLY__ */
+
+#endif /* __ASM_VDSO_PAGE_H */
diff --git a/include/vdso/getrandom.h b/include/vdso/getrandom.h
index a8b7c14b0ae0..9cf65252ee88 100644
--- a/include/vdso/getrandom.h
+++ b/include/vdso/getrandom.h
@@ -7,6 +7,7 @@
 #define _VDSO_GETRANDOM_H
 
 #include <linux/types.h>
+#include <asm/vdso/getrandom.h>
 
 #define CHACHA_KEY_SIZE         32
 #define CHACHA_BLOCK_SIZE       64
diff --git a/include/vdso/mman.h b/include/vdso/mman.h
new file mode 100644
index 000000000000..95e3da714c64
--- /dev/null
+++ b/include/vdso/mman.h
@@ -0,0 +1,7 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __VDSO_MMAN_H
+#define __VDSO_MMAN_H
+
+#include <asm/vdso/mman.h>
+
+#endif	/* __VDSO_MMAN_H */
diff --git a/include/vdso/page.h b/include/vdso/page.h
new file mode 100644
index 000000000000..f18e304941cb
--- /dev/null
+++ b/include/vdso/page.h
@@ -0,0 +1,7 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __VDSO_PAGE_H
+#define __VDSO_PAGE_H
+
+#include <asm/vdso/page.h>
+
+#endif	/* __VDSO_PAGE_H */
diff --git a/lib/vdso/getrandom.c b/lib/vdso/getrandom.c
index 938ca539aaa6..0936fd1ca6ce 100644
--- a/lib/vdso/getrandom.c
+++ b/lib/vdso/getrandom.c
@@ -3,19 +3,11 @@
  * Copyright (C) 2022-2024 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
  */
 
-#include <linux/array_size.h>
-#include <linux/minmax.h>
 #include <vdso/datapage.h>
 #include <vdso/getrandom.h>
 #include <vdso/unaligned.h>
-#include <asm/vdso/getrandom.h>
-#include <uapi/linux/mman.h>
-#include <uapi/linux/random.h>
-
-#undef PAGE_SIZE
-#undef PAGE_MASK
-#define PAGE_SIZE (1UL << CONFIG_PAGE_SHIFT)
-#define PAGE_MASK (~(PAGE_SIZE - 1))
+#include <vdso/mman.h>
+#include <vdso/page.h>
 
 #define MEMCPY_AND_ZERO_SRC(type, dst, src, len) do {				\
 	while (len >= sizeof(type)) {						\
@@ -68,7 +60,7 @@ static __always_inline ssize_t
 __cvdso_getrandom_data(const struct vdso_rng_data *rng_info, void *buffer, size_t len,
 		       unsigned int flags, void *opaque_state, size_t opaque_len)
 {
-	ssize_t ret = min_t(size_t, INT_MAX & PAGE_MASK /* = MAX_RW_COUNT */, len);
+	ssize_t ret = min_t(size_t, INT_MAX & VDSO_PAGE_MASK /* = MAX_RW_COUNT */, len);
 	struct vgetrandom_state *state = opaque_state;
 	size_t batch_len, nblocks, orig_len = len;
 	bool in_use, have_retried = false;
@@ -79,15 +71,15 @@ __cvdso_getrandom_data(const struct vdso_rng_data *rng_info, void *buffer, size_
 	if (unlikely(opaque_len == ~0UL && !buffer && !len && !flags)) {
 		struct vgetrandom_opaque_params *params = opaque_state;
 		params->size_of_opaque_state = sizeof(*state);
-		params->mmap_prot = PROT_READ | PROT_WRITE;
-		params->mmap_flags = MAP_DROPPABLE | MAP_ANONYMOUS;
+		params->mmap_prot = VDSO_MMAP_PROT;
+		params->mmap_flags = VDSO_MMAP_FLAGS;
 		for (size_t i = 0; i < ARRAY_SIZE(params->reserved); ++i)
 			params->reserved[i] = 0;
 		return 0;
 	}
 
 	/* The state must not straddle a page, since pages can be zeroed at any time. */
-	if (unlikely(((unsigned long)opaque_state & ~PAGE_MASK) + sizeof(*state) > PAGE_SIZE))
+	if (unlikely(((unsigned long)opaque_state & ~VDSO_PAGE_MASK) + sizeof(*state) > VDSO_PAGE_SIZE))
 		return -EFAULT;
 
 	/* Handle unexpected flags by falling back to the kernel. */
-- 
2.34.1