Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <d6c2455c-aff5-a135-2610-53dd6b586b59@amd.com>
Date:   Tue, 10 Jan 2023 11:16:30 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: [PATCH] KVM: sev: Fix int overflow in send|recieve_update_data
 ioctls
Content-Language: en-US
To:     Peter Gonda <pgonda@google.com>
Cc:     kvm@vger.kernel.org, Andy Nguyen <theflow@google.com>,
        David Rientjes <rientjes@google.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Sean Christopherson <seanjc@google.com>,
        stable@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20230109160808.3618132-1-pgonda@google.com>
 <74745684-785e-71b2-288e-91fbcf1b555b@amd.com>
 <CAMkAt6q_E-+VV=KOs9LbDzawirWR7M4xL2pCF9fR2kMuBuFM-A@mail.gmail.com>
From:   Tom Lendacky <thomas.lendacky@amd.com>
In-Reply-To: <CAMkAt6q_E-+VV=KOs9LbDzawirWR7M4xL2pCF9fR2kMuBuFM-A@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?UENTMk5DOWNoakpSNHk3SFlia2pVZHJCUnpGWk5ic01YMmJ5NUR1V3BiMWZL?=
 =?utf-8?B?bDRKSlVhQ1llNmhsQ1hqSEh6cUVYOUxKbjJ3WTI1U3pJeHczbEhIRlNadEw3?=
 =?utf-8?B?MW5QVnhsM3dSelZ5MHZnaXg2VnJHSE53NDcrcklMck9tVFVZTmc5ZE9jSzVy?=
 =?utf-8?B?cGF2R3FBWHlxWGhCTWdVMng1N21kbitXM3U2YUJCVUZqMHBmbDFzU1VKd2Zs?=
 =?utf-8?B?S002TjZkMjZZLzd6czFub2N2RzhnRGdkdmRlTStmMjFVbStxNlAwUGNpUzB0?=
 =?utf-8?B?UHFKWm9NaHdDc2twUUJueG0xMXJRbVhZOHRBWEloVHlIMDlCSTNXWWw0eUVW?=
 =?utf-8?B?Rk9MK0lsK0FIdkFHOUJ5RDduVzFYdytTZVIySTl1bElURWlwUWdQNDZ3QUZH?=
 =?utf-8?B?TWExMkoyZFVTcXhIL0lkNmRZcFhDMEl3bXZmWVJpV0JlTG1nbmFEUDd1bmo0?=
 =?utf-8?B?Rjhib0RBV0J1aDhzUzJyVHhZaXducVc4UE03emp6d1BUZmRuaUNIWFZsT25S?=
 =?utf-8?B?bkFhZWJwaC9MeCtWbkZ6K05KK0NxYXRMbURSdXZDemsvbTZWRlVpM0lIZ3ZO?=
 =?utf-8?B?RjVjSUM2VHkwSzhsZEJMbU12SlhVODE3ZzBQYjNra3QrV0dkS3YxeElkZ2xT?=
 =?utf-8?B?S01UMG5LMVdEYzcvZUhiQUgxRDQvTHE4cWNGb3E0YUNGU2hBWVJuWDhxTmJV?=
 =?utf-8?B?eTRRVy9uYlhlRkVMMXZ0S2laSHRqS2h3REFoei82d1YrOUhFRWFhRy9GNU5y?=
 =?utf-8?B?SXB5bGJMeWVQMHFuQk5yemJFL1k4Q3hHMFJZNU9SQ3hTcExFU25NRUlxVVFm?=
 =?utf-8?B?c0FucFRsT081WlB3d0J6anFhc3grL0dNTGdVMU9XODdhRHNPUDYrVFM4aUFT?=
 =?utf-8?B?STA4OUpGVlVaZzBEck0zaWdGTFl3ak04allyWTZyUDhkK1JxSlNGUTJPWEgw?=
 =?utf-8?B?bDIwTk5ZYncySjlrNElaUDIwUFFUTnhDK1Q5YW5FeU8vQWdMU0t5ZzN0cldV?=
 =?utf-8?B?L2JucGhsdytrSFIzRXFnTDZ4RUtFanZBL3YrUWdwVEt2MlJuZXlLTlE3THFP?=
 =?utf-8?B?TStHY2tJMFBjNHA0dS9FT3F2NkRwNXpiZTdjaFM2SHk5eUlLZUpmbG10dlNm?=
 =?utf-8?B?RkpRcVdKRVlTR3R0UUpxcUM5bEx6aG5BczVXZ1ZIT3JCa0xUckV6YTc1WE5L?=
 =?utf-8?B?UjU5ZGhYWFVjYllqeDBQeFY1REJMMlRVcktvRnpoNENuQjIzSHRtOHpnNm13?=
 =?utf-8?B?Q3lHQnlKcU81SW54UlZqeHVYeFJrTCtYc2ZUUWc4TGJtcFVxVStKOEVIWDly?=
 =?utf-8?B?YVRXdnJvckNKWkdkVmIxTEdEVE5yMWdCZzlIT0pNaHc2WlQyTy9rclVucldE?=
 =?utf-8?B?MEdFcEJIUVRQcG9CSTlqbHg1NkhUaFc1a2VadGJNdHo5L0JJeHZIZ3JhUXJJ?=
 =?utf-8?B?S3lyUWowT0taa1RJOGhTZ3IyS3c2RnBWL1laVDhsRi95am5nYmJKZkFnVHZM?=
 =?utf-8?B?TVpCaTFnSmZWYjdHODZTRWZXRXhVZkxHY1ZVcUlST0tzRmZpMGlySXJVK1F6?=
 =?utf-8?B?Mzh1NGoycnExSnA5N1lHQnpES3dSaEJWM2RwTHNPTWU2NGdYK3ZyN0hpZHR4?=
 =?utf-8?B?QnlyQ0MzMFB1OGFHcjY3MVFQemJVSHNkL29XV2tBQUNSVitJYlFWN1dNUldI?=
 =?utf-8?B?d2pqSGdDTGxob0VnYTNFNy9wOVRCWnkyNHlORjBVRUFkUHd1MzUzK0xsMTdV?=
 =?utf-8?B?RFZPak90bkdPS3NjMzUxMVdTcUJaTFE3djhQNnNieGlBaWFmTW5rdUFLbmpV?=
 =?utf-8?B?dTJ2Y05WZUEyclRFWitkT2E3clNRR1MwcC9hTzVMenEzQndFWTNFNlpyUlp1?=
 =?utf-8?B?aldpdWFKM01ncjd5Sit1ckoyazc3Y2p1MzNoaHZxUEh5QjdJTlpOSGNpbU0y?=
 =?utf-8?B?UElscFJNUDRmYTV5cUNFamZhd0NDRDdEWXF1Kzg3eHlCNHE4dVp6ME5naWp3?=
 =?utf-8?B?VlQxNjJCa05lc0NucVVMWW1UWDBSN2hRSXFKMC83U0I0ZFlIVlErS2VRNFVJ?=
 =?utf-8?B?TjhVeXVLUkJDVllMalA4Y1lFSTJNRG9uT25GTncyTFd3aUJ4dGZMZUE0NWww?=
 =?utf-8?Q?dOmE6u2qyrEdnITjrlW4Xms9g?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a56a3be6-af81-4976-5b38-08daf32e6c02
X-MS-Exchange-CrossTenant-AuthSource: DM4PR12MB5229.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Jan 2023 17:16:33.4672
 (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: sMDbQGpkhhKucKxUM4xa3RV9sEUEgQHCvcKJNpgo9/QucfvPf3Dh46aJ2uPZEiIZ4wu14G2pUTB4bgmLbqS0rA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR12MB6853
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 1/10/23 10:44, Peter Gonda wrote:
>>>
>>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>>> index 273cba809328..9451de72f917 100644
>>> --- a/arch/x86/kvm/svm/sev.c
>>> +++ b/arch/x86/kvm/svm/sev.c
>>> @@ -1294,7 +1294,7 @@ static int sev_send_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
>>>
>>>        /* Check if we are crossing the page boundary */
>>>        offset = params.guest_uaddr & (PAGE_SIZE - 1);
>>> -     if ((params.guest_len + offset > PAGE_SIZE))
>>> +     if (params.guest_len > PAGE_SIZE || (params.guest_len + offset > PAGE_SIZE))
>>
>> I see the original if statement had double parentheses, which looks
>> strange. Should this if (and the one below) be:
>>
>>          if (params.guest_len > PAGE_SIZE || (params.guest_len + offset) > PAGE_SIZE)
> 
> Isn't the order of operations here: '+' and then '>'. So is the patch
> correct and matches the old conditional? I am fine adding additional

But what was the purpose of them in the old conditional? They weren't
necessary.

But, yes, that order of operations is correct and those are both before
'||'. So the extra parentheses around the second condition check are still
strange then, right?

Given that, then:

	if (params.guest_len > PAGE_SIZE || params.guest_len + offset > PAGE_SIZE)

> () for clarity though.

I do like the look and clarity of the parentheses around the addition.

Thanks,
Tom