Received-SPF: pass (google.com: domain of linux-kernel+bounces-40756-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <cf5ce480-275d-fd38-fcfe-579e3c4e8c22@amd.com>
Date: Fri, 26 Jan 2024 16:26:50 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [PATCH] x86/sme: Fix memory encryption if enabled by default and
 not overridden
Content-Language: en-US
To: Ard Biesheuvel <ardb+git@google.com>, linux-kernel@vger.kernel.org
Cc: x86@kernel.org, Ard Biesheuvel <ardb@kernel.org>,
 Borislav Petkov <bp@alien8.de>,
 Nikita Zhandarovich <n.zhandarovich@fintech.ru>,
 Kevin Loughlin <kevinloughlin@google.com>
References: <20240126163918.2908990-2-ardb+git@google.com>
From: Tom Lendacky <thomas.lendacky@amd.com>
In-Reply-To: <20240126163918.2908990-2-ardb+git@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?RFZDQXdvRWJDT3diT0dpMU5lMnBiQkpLT1lEcXZWQkw5NnV5VFJxTTZ0WXRO?=
 =?utf-8?B?aUN1ZElUSnV2QWJkc3lYbC9LSzBjV2o0N05KRDVOWEd5Q1RQdWxKcWczTGg1?=
 =?utf-8?B?dGd2RGRQUUFZSStjRjE2WnFoTVRaY0QvdFVWcFI0Zi9OaG1ESGF6eFdaVUln?=
 =?utf-8?B?cmZMbUxsVVlPTkNzZmRSekQyMlQrZ0hobHNRT1lqNVE1c1lzdTFjUm9DR2pF?=
 =?utf-8?B?WmxVakhtNTl0L2ZsNW1VL1hFaGRaTlBWdndJMkU1TUVFT2FadFVtb2ZUUHBM?=
 =?utf-8?B?ZXV6dTY3alk5Mk11QlJmbzhORTRmVW1yc3ZrTUtXMlZ2VERLbGxVb1Qyd2JI?=
 =?utf-8?B?cDRNbTlFMVNURnRZRjQvVys1M1RMQXNzdHJaQUpUK3hiUC9CZUVvZmswSUhC?=
 =?utf-8?B?SjVsZGZOS0hqc1Ezb2Z5TC8yRjFkL1NreWF1R2hFcW5UMm12dEZ4SjJ4Q0l2?=
 =?utf-8?B?VTBpSkQ0eGdFZ0doNGVwR1NMTXJXNE9ya2NvRzBQSDVRL2EzczFrQ0FCMDc4?=
 =?utf-8?B?WXprdVI4eW5HbngyUXNUUnN3NWN1VlhIeE9VSUpLYXZiRjFoTHdYTDlGckR4?=
 =?utf-8?B?K0lucVpTZHdRZXBGZFJMY3k4L3N5cnh0YWpDc1NyaXU1T1V1SWVaT0lrSHhV?=
 =?utf-8?B?ZVEwRGNXZ2pZSGF1U0l5WVZ2UWxGUTFrVDVTaEJaVDBNSm1BZmd3ZTgwcW5j?=
 =?utf-8?B?MHNjdjF0ajNHNGIyRjNKRlNtai91Tnp1QmR3dlNmWGlMS29EL1YwMTlyQUl1?=
 =?utf-8?B?MDFUdmN4djVhU0d3VmgyR2F5dzhaVnJxUFlFc3cvQjl6eEZlR3diaG9HZmVN?=
 =?utf-8?B?d3Nnc3pobnRycGNaUWZkMXJIZElPUll0ZXZ3Y3V4WnYwSjRpNXdnYjVVeHJp?=
 =?utf-8?B?NG1MTGUxbnlaeU9saThKZUpSYUNlaTh2UExpZG56RWVjc2tvMTBaem1uQWpi?=
 =?utf-8?B?RXNwZjdGdm9xaXYyZ2lvbWRlV2ZGV3BQcXU2ZE41VXNOeTVmOVhMVEx2L3hL?=
 =?utf-8?B?SGM1Z2lhRWZwUVNBZE9VaXNGbE9sU3MxeFhrVEd1S09PTDZrRHArYWFRc1hG?=
 =?utf-8?B?N0dzOVcxbjF1dXVRa2Q0ZHQ2dlEwSXhtUDdMb0xQbHVLMEtuQjliM2ViemIy?=
 =?utf-8?B?aVZkMzhUWUoyU2ZidmhkTUp1dHo0bEFhV3FiRjRpdUQ5OXJvZnVBSHZ5c1Y5?=
 =?utf-8?B?WXViNmM1NEZBNzJsUE5DbmlhanNkS2tlNVlLOEV4aTFQQld0SDJ3T1NNN2M4?=
 =?utf-8?B?ZkNOSFk1c1NRR2hGeXhmdS9vcnpTb3lmL21MMy9MODB2Vm02OXpiTllWQy9Y?=
 =?utf-8?B?RUFDb28veUhQeG9ERHZ4TDVWVmVyc3lyVVpsSUsxOWlTd2RUL3N5SzRPaGpD?=
 =?utf-8?B?T3d1dmlDVmFQVG9GUHJva3B6QStIS0xtKzR3RkJ3QWFxTTB1cU5XRnU0Sjln?=
 =?utf-8?B?endvMTE3bllyR1ZSY2ZQVXRTbDZqQlV2T0JJOEp2ZW9hVTNBcXRaK3lYWmVZ?=
 =?utf-8?B?V29mTDc0V1M5Si9Rb1hZcm5leTNsTllpeW90QnNkOGhSMWNBcFlId0wwM2hP?=
 =?utf-8?B?NHMvTVBrSWVvd09DM0lsOU1EOU95Q25WdVBNTnl2bEtYbXVWeG4yMW9kTXJS?=
 =?utf-8?B?WnhUQ3d5NXRIYWQ2QmlpUmtuTk4xWTRtaklGVVV2RXh1NVlpVHE0QUp5ZnhC?=
 =?utf-8?B?N0tvdFl4WmxoZWdUSTRkK1J0UXZsLzRuamhSWXJlVXQvaWtkeHJTSG1WSTd1?=
 =?utf-8?B?Qk9ZSHlLK3grRm02THY1cThwQzBwUXYvc3pBRTFwNUFYTi8zbTZrb0FlOTRL?=
 =?utf-8?B?QXR1TXdSNXJVNlo0NlBRV1R4OWdybXJObjJqSGRZZTlHVTFacFhscG50MVpF?=
 =?utf-8?B?Z3YzNlRvL0NkWEZmMXZtM21kb1E5eVltTUMxUkhTWTBDTTVqL2ZGZThhOXND?=
 =?utf-8?B?TjMvOWJwVXB3UTR0SlY5Q3VONDY3OU9lRW5KTTFmV3VBZm1RTU1nbXYzbTJD?=
 =?utf-8?B?OWcvWnY4VXRjejdjVFhERjRmRVg4YjdSTC9WeWkwRGNXNGM1aHpKTlZ1NWtV?=
 =?utf-8?B?SHp1SmVwMWtMQ2RLTWJsVUJaTjVtSVNCeVhxc3NsVGxRTWxJSjZxMFEwa2My?=
 =?utf-8?Q?914OMBE6GOKUUG0iyB703iQPl?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 6e9408c8-7fb2-4d19-2baa-08dc1ebde5bf
X-MS-Exchange-CrossTenant-AuthSource: BL1PR12MB5732.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Jan 2024 22:26:53.3255
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: rRSJLOBEBvpzWHmRjVbjRzDT8rI4re+LzZwX8K99QDlukmzQ1Y33ASXjORD4OD0xMXzTJaE3ts+q2LoH1lw0pQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR12MB7078

On 1/26/24 10:39, Ard Biesheuvel wrote:
> From: Ard Biesheuvel <ardb@kernel.org>
> 
> 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) <bp@alien8.de>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
> Cc: Kevin Loughlin <kevinloughlin@google.com>
> Fixes: cbebd68f59f0 ("x86/mm: Fix use of uninitialized buffer in sme_enable()")
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

Nice catch.

Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>

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