From: Ard Biesheuvel <[email protected]>
Commit cbebd68f59f0 ("x86/mm: Fix use of uninitialized buffer in
sme_enable()") 'fixed' an issue in sme_enable() detected by static
analysis, and broke the common case in the process.
cmdline_find_option() will return < 0 on an error, or when the command
line argument does not appear at all. In this particular case, the
latter is not an error condition, and so the early exit is wrong.
Instead, without mem_encrypt= on the command line, the compile time
default should be honoured, which could be to enable memory encryption,
and this is currently broken.
Fix it by setting sme_me_mask to a preliminary value based on the
compile time default, and only omitting the command line argument test
when cmdline_find_option() returns an error.
Cc: Borislav Petkov (AMD) <[email protected]>
Cc: Tom Lendacky <[email protected]>
Cc: Nikita Zhandarovich <[email protected]>
Cc: Kevin Loughlin <[email protected]>
Fixes: cbebd68f59f0 ("x86/mm: Fix use of uninitialized buffer in sme_enable()")
Signed-off-by: Ard Biesheuvel <[email protected]>
---
arch/x86/mm/mem_encrypt_identity.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/arch/x86/mm/mem_encrypt_identity.c b/arch/x86/mm/mem_encrypt_identity.c
index d73aeb16417f..30df4f1725f4 100644
--- a/arch/x86/mm/mem_encrypt_identity.c
+++ b/arch/x86/mm/mem_encrypt_identity.c
@@ -600,15 +600,14 @@ void __init sme_enable(struct boot_params *bp)
cmdline_ptr = (const char *)((u64)bp->hdr.cmd_line_ptr |
((u64)bp->ext_cmd_line_ptr << 32));
+ sme_me_mask = active_by_default ? me_mask : 0;
if (cmdline_find_option(cmdline_ptr, cmdline_arg, buffer, sizeof(buffer)) < 0)
- return;
+ goto out;
if (!strncmp(buffer, cmdline_on, sizeof(buffer)))
sme_me_mask = me_mask;
else if (!strncmp(buffer, cmdline_off, sizeof(buffer)))
sme_me_mask = 0;
- else
- sme_me_mask = active_by_default ? me_mask : 0;
out:
if (sme_me_mask) {
physical_mask &= ~sme_me_mask;
--
2.43.0.429.g432eaa2c6b-goog
On 1/26/24 10:39, Ard Biesheuvel wrote:
> From: Ard Biesheuvel <[email protected]>
>
> Commit cbebd68f59f0 ("x86/mm: Fix use of uninitialized buffer in
> sme_enable()") 'fixed' an issue in sme_enable() detected by static
> analysis, and broke the common case in the process.
>
> cmdline_find_option() will return < 0 on an error, or when the command
> line argument does not appear at all. In this particular case, the
> latter is not an error condition, and so the early exit is wrong.
>
> Instead, without mem_encrypt= on the command line, the compile time
> default should be honoured, which could be to enable memory encryption,
> and this is currently broken.
>
> Fix it by setting sme_me_mask to a preliminary value based on the
> compile time default, and only omitting the command line argument test
> when cmdline_find_option() returns an error.
>
> Cc: Borislav Petkov (AMD) <[email protected]>
> Cc: Tom Lendacky <[email protected]>
> Cc: Nikita Zhandarovich <[email protected]>
> Cc: Kevin Loughlin <[email protected]>
> Fixes: cbebd68f59f0 ("x86/mm: Fix use of uninitialized buffer in sme_enable()")
> Signed-off-by: Ard Biesheuvel <[email protected]>
Nice catch.
Reviewed-by: Tom Lendacky <[email protected]>
> ---
> arch/x86/mm/mem_encrypt_identity.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/mm/mem_encrypt_identity.c b/arch/x86/mm/mem_encrypt_identity.c
> index d73aeb16417f..30df4f1725f4 100644
> --- a/arch/x86/mm/mem_encrypt_identity.c
> +++ b/arch/x86/mm/mem_encrypt_identity.c
> @@ -600,15 +600,14 @@ void __init sme_enable(struct boot_params *bp)
> cmdline_ptr = (const char *)((u64)bp->hdr.cmd_line_ptr |
> ((u64)bp->ext_cmd_line_ptr << 32));
>
> + sme_me_mask = active_by_default ? me_mask : 0;
> if (cmdline_find_option(cmdline_ptr, cmdline_arg, buffer, sizeof(buffer)) < 0)
> - return;
> + goto out;
>
> if (!strncmp(buffer, cmdline_on, sizeof(buffer)))
> sme_me_mask = me_mask;
> else if (!strncmp(buffer, cmdline_off, sizeof(buffer)))
> sme_me_mask = 0;
> - else
> - sme_me_mask = active_by_default ? me_mask : 0;
> out:
> if (sme_me_mask) {
> physical_mask &= ~sme_me_mask;
On Fri, Jan 26, 2024 at 05:39:19PM +0100, Ard Biesheuvel wrote:
> From: Ard Biesheuvel <[email protected]>
>
> Commit cbebd68f59f0 ("x86/mm: Fix use of uninitialized buffer in
> sme_enable()") 'fixed' an issue in sme_enable() detected by static
> analysis, and broke the common case in the process.
>
> cmdline_find_option() will return < 0 on an error, or when the command
> line argument does not appear at all.
Is it just me or cmdline_find_option() should be fixed to return 0 when
there's no cmdline argument and < 0 only when there was a real error
parsing?
Hohumm, sounds like a TODO for me or someone who wants to audit all
callers and fix them up accordingly.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
The following commit has been merged into the x86/sev branch of tip:
Commit-ID: aa8eff72842021f52600392b245fb82d113afa8a
Gitweb: https://git.kernel.org/tip/aa8eff72842021f52600392b245fb82d113afa8a
Author: Ard Biesheuvel <[email protected]>
AuthorDate: Fri, 26 Jan 2024 17:39:19 +01:00
Committer: Borislav Petkov (AMD) <[email protected]>
CommitterDate: Sat, 27 Jan 2024 12:17:26 +01:00
x86/sme: Fix memory encryption setting if enabled by default and not overridden
Commit
cbebd68f59f0 ("x86/mm: Fix use of uninitialized buffer in sme_enable()")
'fixed' an issue in sme_enable() detected by static analysis, and broke
the common case in the process.
cmdline_find_option() will return < 0 on an error, or when the command
line argument does not appear at all. In this particular case, the
latter is not an error condition, and so the early exit is wrong.
Instead, without mem_encrypt= on the command line, the compile time
default should be honoured, which could be to enable memory encryption,
and this is currently broken.
Fix it by setting sme_me_mask to a preliminary value based on the
compile time default, and only omitting the command line argument test
when cmdline_find_option() returns an error.
[ bp: Drop active_by_default while at it. ]
Fixes: cbebd68f59f0 ("x86/mm: Fix use of uninitialized buffer in sme_enable()")
Signed-off-by: Ard Biesheuvel <[email protected]>
Signed-off-by: Borislav Petkov (AMD) <[email protected]>
Reviewed-by: Tom Lendacky <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
arch/x86/mm/mem_encrypt_identity.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/arch/x86/mm/mem_encrypt_identity.c b/arch/x86/mm/mem_encrypt_identity.c
index d73aeb1..7f72472 100644
--- a/arch/x86/mm/mem_encrypt_identity.c
+++ b/arch/x86/mm/mem_encrypt_identity.c
@@ -507,7 +507,6 @@ void __init sme_enable(struct boot_params *bp)
const char *cmdline_ptr, *cmdline_arg, *cmdline_on, *cmdline_off;
unsigned int eax, ebx, ecx, edx;
unsigned long feature_mask;
- bool active_by_default;
unsigned long me_mask;
char buffer[16];
bool snp;
@@ -593,22 +592,19 @@ void __init sme_enable(struct boot_params *bp)
: "p" (sme_cmdline_off));
if (IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT_ACTIVE_BY_DEFAULT))
- active_by_default = true;
- else
- active_by_default = false;
+ sme_me_mask = me_mask;
cmdline_ptr = (const char *)((u64)bp->hdr.cmd_line_ptr |
((u64)bp->ext_cmd_line_ptr << 32));
if (cmdline_find_option(cmdline_ptr, cmdline_arg, buffer, sizeof(buffer)) < 0)
- return;
+ goto out;
if (!strncmp(buffer, cmdline_on, sizeof(buffer)))
sme_me_mask = me_mask;
else if (!strncmp(buffer, cmdline_off, sizeof(buffer)))
sme_me_mask = 0;
- else
- sme_me_mask = active_by_default ? me_mask : 0;
+
out:
if (sme_me_mask) {
physical_mask &= ~sme_me_mask;
On Sat, 27 Jan 2024 at 11:53, Borislav Petkov <[email protected]> wrote:
>
> On Fri, Jan 26, 2024 at 05:39:19PM +0100, Ard Biesheuvel wrote:
> > From: Ard Biesheuvel <[email protected]>
> >
> > Commit cbebd68f59f0 ("x86/mm: Fix use of uninitialized buffer in
> > sme_enable()") 'fixed' an issue in sme_enable() detected by static
> > analysis, and broke the common case in the process.
> >
> > cmdline_find_option() will return < 0 on an error, or when the command
> > line argument does not appear at all.
>
> Is it just me or cmdline_find_option() should be fixed to return 0 when
> there's no cmdline argument and < 0 only when there was a real error
> parsing?
>
> Hohumm, sounds like a TODO for me or someone who wants to audit all
> callers and fix them up accordingly.
>
I intend to propose removing this occurrence entirely, and move it
into the EFI stub/decompressor.
Parsing external input in security related code when the entire kernel
is still mapped writable is kind of gross, and since I'm cleaning up
the early PIC code anyway, might just as well rip this out.
Another issue here is that it does not honor
CONFIG_CMDLINE_BOOL/CONFIG_CMDLINE_OVERRIDE. I.e., if memory
encryption is enabled by default, and the kernel is configured to
ignore the command line, the current code will still disable memory
encryption when mem_encrypt=off is passed. Probably not a big deal,
but unintuitive nonetheless.
For the other cases, I agree that this would be a good thing to clean
up. Note that someone has been looking into the handling of the
command line recently.
https://lore.kernel.org/all/[email protected]/T/#m817c573e25f6f7da237272178a8f6b116192a6ad
but I am not sure what the state of the code is, or whether the
approach is the right one. I was meaning to look at that but haven't
found the time yet.
The following commit has been merged into the x86/sev branch of tip:
Commit-ID: e814b59e6c2b11f5a3d007b2e61f7d550c354c3a
Gitweb: https://git.kernel.org/tip/e814b59e6c2b11f5a3d007b2e61f7d550c354c3a
Author: Ard Biesheuvel <[email protected]>
AuthorDate: Fri, 26 Jan 2024 17:39:19 +01:00
Committer: Borislav Petkov (AMD) <[email protected]>
CommitterDate: Mon, 29 Jan 2024 17:08:33 +01:00
x86/sme: Fix memory encryption setting if enabled by default and not overridden
Commit
cbebd68f59f0 ("x86/mm: Fix use of uninitialized buffer in sme_enable()")
'fixed' an issue in sme_enable() detected by static analysis, and broke
the common case in the process.
cmdline_find_option() will return < 0 on an error, or when the command
line argument does not appear at all. In this particular case, the
latter is not an error condition, and so the early exit is wrong.
Instead, without mem_encrypt= on the command line, the compile time
default should be honoured, which could be to enable memory encryption,
and this is currently broken.
Fix it by setting sme_me_mask to a preliminary value based on the
compile time default, and only omitting the command line argument test
when cmdline_find_option() returns an error.
[ bp: Drop active_by_default while at it. ]
Fixes: cbebd68f59f0 ("x86/mm: Fix use of uninitialized buffer in sme_enable()")
Signed-off-by: Ard Biesheuvel <[email protected]>
Signed-off-by: Borislav Petkov (AMD) <[email protected]>
Reviewed-by: Tom Lendacky <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
arch/x86/mm/mem_encrypt_identity.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/arch/x86/mm/mem_encrypt_identity.c b/arch/x86/mm/mem_encrypt_identity.c
index d73aeb1..7f72472 100644
--- a/arch/x86/mm/mem_encrypt_identity.c
+++ b/arch/x86/mm/mem_encrypt_identity.c
@@ -507,7 +507,6 @@ void __init sme_enable(struct boot_params *bp)
const char *cmdline_ptr, *cmdline_arg, *cmdline_on, *cmdline_off;
unsigned int eax, ebx, ecx, edx;
unsigned long feature_mask;
- bool active_by_default;
unsigned long me_mask;
char buffer[16];
bool snp;
@@ -593,22 +592,19 @@ void __init sme_enable(struct boot_params *bp)
: "p" (sme_cmdline_off));
if (IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT_ACTIVE_BY_DEFAULT))
- active_by_default = true;
- else
- active_by_default = false;
+ sme_me_mask = me_mask;
cmdline_ptr = (const char *)((u64)bp->hdr.cmd_line_ptr |
((u64)bp->ext_cmd_line_ptr << 32));
if (cmdline_find_option(cmdline_ptr, cmdline_arg, buffer, sizeof(buffer)) < 0)
- return;
+ goto out;
if (!strncmp(buffer, cmdline_on, sizeof(buffer)))
sme_me_mask = me_mask;
else if (!strncmp(buffer, cmdline_off, sizeof(buffer)))
sme_me_mask = 0;
- else
- sme_me_mask = active_by_default ? me_mask : 0;
+
out:
if (sme_me_mask) {
physical_mask &= ~sme_me_mask;