On 64-bit platforms, the pfn_to_kaddr() macro requires that the input
value is 64-bits in order to ensure that valid address bits don't get
lost when shifting that input by PAGE_SHIFT to calculate the physical
address to provide a virtual address for.
One such example is in pvalidate_pages() (used by SEV-SNP guests), where
the GFN in the struct used for page-state change requests is a 40-bit
bit-field, so attempts to pass this GFN field directly into
pfn_to_kaddr() ends up causing guest crashes when dealing with addresses
above the 1TB range due to the above.
Fix this issue with SEV-SNP guests, as well as any similar cases that
might cause issues in current/future code, by using a inline function,
instead of the macro, so that the input is implicitly cast to the
expected 64-bit input type prior to performing the shift operation.
While it might be argued that the issue is on the caller side, other
archs/macros have taken similar approaches to deal with instances like
this, such as commit e48866647b48 ("ARM: 8396/1: use phys_addr_t in
pfn_to_kaddr()").
Suggested-by: Dave Hansen <[email protected]>
Suggested-by: H. Peter Anvin <[email protected]>
Fixes: 6c3211796326 ("x86/sev: Add SNP-specific unaccepted memory support")
Signed-off-by: Michael Roth <[email protected]>
---
v3:
- Use an inline function instead of a macro to handle the cast (Peter)
v2:
- Move the cast down into pfn_to_kaddr() to fix other possible
instances (Dave)
- Rename from "x86/sev: Fix overflow when computing address for
PVALIDATE"
arch/x86/include/asm/page.h | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/page.h b/arch/x86/include/asm/page.h
index d18e5c332cb9..aa8930da42fd 100644
--- a/arch/x86/include/asm/page.h
+++ b/arch/x86/include/asm/page.h
@@ -66,10 +66,14 @@ static inline void copy_user_page(void *to, void *from, unsigned long vaddr,
* virt_addr_valid(kaddr) returns true.
*/
#define virt_to_page(kaddr) pfn_to_page(__pa(kaddr) >> PAGE_SHIFT)
-#define pfn_to_kaddr(pfn) __va((pfn) << PAGE_SHIFT)
extern bool __virt_addr_valid(unsigned long kaddr);
#define virt_addr_valid(kaddr) __virt_addr_valid((unsigned long) (kaddr))
+static inline void *pfn_to_kaddr(unsigned long pfn)
+{
+ return __va(pfn << PAGE_SHIFT);
+}
+
static __always_inline u64 __canonical_address(u64 vaddr, u8 vaddr_bits)
{
return ((s64)vaddr << (64 - vaddr_bits)) >> (64 - vaddr_bits);
--
2.25.1
* Michael Roth <[email protected]> wrote:
> On 64-bit platforms, the pfn_to_kaddr() macro requires that the input
> value is 64-bits in order to ensure that valid address bits don't get
> lost when shifting that input by PAGE_SHIFT to calculate the physical
> address to provide a virtual address for.
>
> One such example is in pvalidate_pages() (used by SEV-SNP guests), where
> the GFN in the struct used for page-state change requests is a 40-bit
> bit-field, so attempts to pass this GFN field directly into
> pfn_to_kaddr() ends up causing guest crashes when dealing with addresses
> above the 1TB range due to the above.
>
> Fix this issue with SEV-SNP guests, as well as any similar cases that
> might cause issues in current/future code, by using a inline function,
> instead of the macro, so that the input is implicitly cast to the
> expected 64-bit input type prior to performing the shift operation.
>
> While it might be argued that the issue is on the caller side, other
> archs/macros have taken similar approaches to deal with instances like
> this, such as commit e48866647b48 ("ARM: 8396/1: use phys_addr_t in
> pfn_to_kaddr()").
>
> Suggested-by: Dave Hansen <[email protected]>
> Suggested-by: H. Peter Anvin <[email protected]>
> Fixes: 6c3211796326 ("x86/sev: Add SNP-specific unaccepted memory support")
> Signed-off-by: Michael Roth <[email protected]>
> ---
> v3:
> - Use an inline function instead of a macro to handle the cast (Peter)
> v2:
> - Move the cast down into pfn_to_kaddr() to fix other possible
> instances (Dave)
> - Rename from "x86/sev: Fix overflow when computing address for
> PVALIDATE"
>
> arch/x86/include/asm/page.h | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/page.h b/arch/x86/include/asm/page.h
> index d18e5c332cb9..aa8930da42fd 100644
> --- a/arch/x86/include/asm/page.h
> +++ b/arch/x86/include/asm/page.h
> @@ -66,10 +66,14 @@ static inline void copy_user_page(void *to, void *from, unsigned long vaddr,
> * virt_addr_valid(kaddr) returns true.
> */
> #define virt_to_page(kaddr) pfn_to_page(__pa(kaddr) >> PAGE_SHIFT)
> -#define pfn_to_kaddr(pfn) __va((pfn) << PAGE_SHIFT)
> extern bool __virt_addr_valid(unsigned long kaddr);
> #define virt_addr_valid(kaddr) __virt_addr_valid((unsigned long) (kaddr))
>
> +static inline void *pfn_to_kaddr(unsigned long pfn)
> +{
> + return __va(pfn << PAGE_SHIFT);
> +}
Makes sense I guess.
Note that in the tip:x86/mm commit I changed it to __always_inline, as we
typically do for such low level MM helpers. Delta patch below.
Thanks,
Ingo
==============>
arch/x86/include/asm/page.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/page.h b/arch/x86/include/asm/page.h
index aa8930da42fd..1b93ff80b43b 100644
--- a/arch/x86/include/asm/page.h
+++ b/arch/x86/include/asm/page.h
@@ -69,7 +69,7 @@ static inline void copy_user_page(void *to, void *from, unsigned long vaddr,
extern bool __virt_addr_valid(unsigned long kaddr);
#define virt_addr_valid(kaddr) __virt_addr_valid((unsigned long) (kaddr))
-static inline void *pfn_to_kaddr(unsigned long pfn)
+static __always_inline void *pfn_to_kaddr(unsigned long pfn)
{
return __va(pfn << PAGE_SHIFT);
}
The following commit has been merged into the x86/mm branch of tip:
Commit-ID: 8e5647a723c49d73b9f108a8bb38e8c29d3948ea
Gitweb: https://git.kernel.org/tip/8e5647a723c49d73b9f108a8bb38e8c29d3948ea
Author: Michael Roth <[email protected]>
AuthorDate: Wed, 22 Nov 2023 10:37:00 -06:00
Committer: Ingo Molnar <[email protected]>
CommitterDate: Thu, 23 Nov 2023 11:13:21 +01:00
x86/mm: Ensure input to pfn_to_kaddr() is treated as a 64-bit type
On 64-bit platforms, the pfn_to_kaddr() macro requires that the input
value is 64 bits in order to ensure that valid address bits don't get
lost when shifting that input by PAGE_SHIFT to calculate the physical
address to provide a virtual address for.
One such example is in pvalidate_pages() (used by SEV-SNP guests), where
the GFN in the struct used for page-state change requests is a 40-bit
bit-field, so attempts to pass this GFN field directly into
pfn_to_kaddr() ends up causing guest crashes when dealing with addresses
above the 1TB range due to the above.
Fix this issue with SEV-SNP guests, as well as any similar cases that
might cause issues in current/future code, by using an inline function,
instead of a macro, so that the input is implicitly cast to the
expected 64-bit input type prior to performing the shift operation.
While it might be argued that the issue is on the caller side, other
archs/macros have taken similar approaches to deal with instances like
this, such as ARM explicitly casting the input to phys_addr_t:
e48866647b48 ("ARM: 8396/1: use phys_addr_t in pfn_to_kaddr()")
A C inline function is even better though.
[ mingo: Refined the changelog some more & added __always_inline. ]
Fixes: 6c3211796326 ("x86/sev: Add SNP-specific unaccepted memory support")
Suggested-by: Dave Hansen <[email protected]>
Suggested-by: H. Peter Anvin <[email protected]>
Signed-off-by: Michael Roth <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Cc: Andy Lutomirski <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Rik van Riel <[email protected]>
Cc: Linus Torvalds <[email protected]>
---
arch/x86/include/asm/page.h | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/page.h b/arch/x86/include/asm/page.h
index d18e5c3..1b93ff8 100644
--- a/arch/x86/include/asm/page.h
+++ b/arch/x86/include/asm/page.h
@@ -66,10 +66,14 @@ static inline void copy_user_page(void *to, void *from, unsigned long vaddr,
* virt_addr_valid(kaddr) returns true.
*/
#define virt_to_page(kaddr) pfn_to_page(__pa(kaddr) >> PAGE_SHIFT)
-#define pfn_to_kaddr(pfn) __va((pfn) << PAGE_SHIFT)
extern bool __virt_addr_valid(unsigned long kaddr);
#define virt_addr_valid(kaddr) __virt_addr_valid((unsigned long) (kaddr))
+static __always_inline void *pfn_to_kaddr(unsigned long pfn)
+{
+ return __va(pfn << PAGE_SHIFT);
+}
+
static __always_inline u64 __canonical_address(u64 vaddr, u8 vaddr_bits)
{
return ((s64)vaddr << (64 - vaddr_bits)) >> (64 - vaddr_bits);
On Thu, 23 Nov 2023 at 02:31, tip-bot2 for Michael Roth
<[email protected]> wrote:
>
> On 64-bit platforms, the pfn_to_kaddr() macro requires that the input
> value is 64 bits in order to ensure that valid address bits don't get
> lost when shifting that input by PAGE_SHIFT to calculate the physical
> address to provide a virtual address for.
Bah. The commit is obviously fine, but can we please just get rid of
that broken pfn_to_kaddr() thing entirely?
It's a bogus mis-spelling of pfn_to_virt(), and I don't know why that
horrid thing exists. In *all* other situations we talk about "virt"
for kernel virtual addresses, I don't know where that horrid "kaddr"
came from (ie "virt_to_page()" and friends).
Most notably, we have "virt_to_pfn()" being quite commonly used. We
don't even have that kaddr_to_pfn(), which just shows *how* bogus this
whole "pfn_to_kaddr()" crud is.
The good news is that there aren't a ton of users. Anybody willing to
just do a search-and-replace and get rid of this pointless and wrong
thing?
Using "pfn_to_virt()" has the added advantage that we have a generic
implementation of it that isn't duplicated pointlessly for N
architectures, and that didn't have this bug:
static inline void *pfn_to_virt(unsigned long pfn)
{
return __va(pfn) << PAGE_SHIFT;
}
#define pfn_to_virt pfn_to_virt
Hmm?
Amusingly (or sadly), we have s390 holding up the flag of sanity, and having
#define pfn_to_kaddr(pfn) pfn_to_virt(pfn)
and then we'd only need to fix the hexagon version of that macro
(since Hexagon made its own version, with the old bug - but I guess
Hexagon is 32-bit only and hopefully never grows 64-bit (??) so maybe
nobody cares).
Linus