Received-SPF: pass (google.com: domain of linux-kernel+bounces-46857-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: <368248d0-d379-23c8-dedf-af7e1e8d23c7@oracle.com>
Date: Wed, 31 Jan 2024 09:02:11 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [PATCH] KVM: x86/pmu: Fix type length error when reading
 pmu->fixed_ctr_ctrl
To: Sean Christopherson <seanjc@google.com>,
        Mingwei Zhang <mizhang@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>, "H. Peter Anvin" <hpa@zytor.com>,
        kvm@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20240123221220.3911317-1-mizhang@google.com>
 <ZbpqoU49k44xR4zB@google.com>
Content-Language: en-US
From: Dongli Zhang <dongli.zhang@oracle.com>
In-Reply-To: <ZbpqoU49k44xR4zB@google.com>
Content-Type: text/plain; charset=UTF-8
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?SExzdjBiMFVXR0VzZ3FicDlERHRkT0prSVNkS0tqZStBeU96RmFSZXFaZ2tW?=
 =?utf-8?B?UG1YUmhkSSt5TThjV1lYNkdCWFFYMklPeENsWFRFSy9BUlVMQ2ZBOE02SEFI?=
 =?utf-8?B?enRNMXJkcXJQRDFQdmRPMU1XTjZBa2I1ZllrbnBmZlRwckRzMFV1Z0ZMK1Iv?=
 =?utf-8?B?MEVzK2x0QjZ2RlZoRWdsa2xhdTZqUWZQZUp0NDZrT3B0Tm0wMWFOZ0Vsc2Jo?=
 =?utf-8?B?YmxKRDV4NTdpUmY2MFE0RmFDdVZJWnk1YVJjbnpnbGhwTEpmVHlTUTl6SmxS?=
 =?utf-8?B?YU5Wem4rODVjV0FMSWFWWEkzVGt4STlma1V1V1RyT1BWSjVKa2hWcHcwRDZG?=
 =?utf-8?B?cVV5bHpvZ09kTEJnZ2F3L245d015cStTdzRKVDF6NTJCQ2RHS2RuS1JCT21M?=
 =?utf-8?B?ak1pZDJnWTFxRkxMNVJucjROMi9EUVpVTFk4RGRBV0d3aTg4cVBHbzNpU0xB?=
 =?utf-8?B?RzFleG9OOXJZemljRVNGMG53NDd0REFPbWhXMUxsU1FpZGQxUVBYNmxEZ3Vt?=
 =?utf-8?B?eUN6MHJZWmJqNHlVQjYwNFBLMFBWNms2Zy9ITVpYbHFYVWNDelJ1Ykl1YlBI?=
 =?utf-8?B?QzdSM1d6Q0JkSjhXVlNVNTJBRk1pTjBqTVhqdTB3Z3UwQitXSXlRK3hsSFpF?=
 =?utf-8?B?Y01abUdJeVU4enRyVXU2U1RoQUhzWTlITWdiSzZCdnNCclRxK3VPUGcxTkRo?=
 =?utf-8?B?MUg3RXV1NmhPVzdQYXNmQnZ5MGRVK0VnQTdtMGxZdmcvWTJHRkE0T2QzU0ow?=
 =?utf-8?B?WUhqRHdjR1A4VkVDZzdPOTI3MERxT3RvekdFaGZPSS9JWmdHM3dBeitTejBP?=
 =?utf-8?B?akJsMStMaTZmeEdITGgyb0p6dnFHSTZ6SWhNTmlrNFVtRDJyWFh2T2t1cVlI?=
 =?utf-8?B?YnlqSklXRnRFdjhNVkczZTZybzNYNUIxRWdqZFpqZ2xlUnBSY0JrbFhZemVD?=
 =?utf-8?B?blEzdGw0dUlaNUlaRkJ1aHpvckdOUGcyaFd0bEVXOFgyQ2pDRFhDdTVKbGZo?=
 =?utf-8?B?aVU5cTZFbUdLK1pkelpYYWpwUUd6RURsYjYyT0RIYkhrVmFyNk5iUTc2bkYx?=
 =?utf-8?B?aDJvcFZhODhKeVJLV09qbWpCOG9wNC9iR3Y1TlFlSFBWcC93VUxXdm5MWVIr?=
 =?utf-8?B?SzBPdnUwcnhrVUVmOGVjMTg3SDNSOUVCaUtxZU1idlRPZzNqanhWbDlzSm96?=
 =?utf-8?B?clcyU2pyaGV0WmVrbGtEejRvaVM0clJucEptTGtlYjVhYWVzMGpEYlg4R09p?=
 =?utf-8?B?TDRHS3c3OTlYMTJ2NWcrclBYbnZaQTVKOXdhL0g2S2I5MXlKaVd0ekFXSXlR?=
 =?utf-8?B?UUE0K2FzcE1id1dybnRaYUYwMmwzTHlndDRsVjhFN3V1ZHRrM3MraTc2V3RX?=
 =?utf-8?B?K0JPeE85NHhzR0Q5VU9jZ2krakJONWZpVFlpcWRkNkczZUJrTXNwNTF0aDND?=
 =?utf-8?B?cXAzOFVva1N2dGNBcXdLVUxJZ1RqbjVERDRZOEVGdnpJZXF1QU5CS2piWFJF?=
 =?utf-8?B?TVZuOW9rY3R6QmNsb0JnVm5kR1YzSXBreWNKRFRVTlF2K0FsN3Fzb3dTaExL?=
 =?utf-8?B?OURaaWVFa1ZYVW4yLzR6YUtUeE5RVTQ4d3lWcTRrNFUrWEtDZFJiekJDUUc3?=
 =?utf-8?B?ZllNM1ZlREg4bVBvWkRSS01CNml3LzMweHJOTE55ZlJKRVI3cEVSOTVjK2pq?=
 =?utf-8?B?b3o3ZkdCSTFzai93cVJSMXRrcnhsYzNHU01vM0QrNnpzN3dxMHZDVHVjSFh0?=
 =?utf-8?B?VEFpMHdCREt1bWhqUTV4UTgxMklOOUNwRTM2RkhKSWNQY3BvMFBCbExGazMr?=
 =?utf-8?B?cTRtbkRWVWE3NTlUVUdYazlNNHpjY0hQaVdTc1NybHI1bDFjYTRhOTMzWjhV?=
 =?utf-8?B?RHRaMXhXWitMN1ZUUEhQNDB0Nkc3ckQvaUZRYzl5end5bzRZaitUbGFRTGhK?=
 =?utf-8?B?Q1ROdng0SE9RNEdlUW9GMkdxcDg0RGZkTWxwb05JOGRCZ0o1am1qRHRpY203?=
 =?utf-8?B?aFp3MTdFZnc5Skt2TlpwUS84UEE2UXR4TmZkVG9NQTVBQ0lUaTdvNFNNNXBZ?=
 =?utf-8?B?WnBsa00rdUMwMFQxaEd2QThyWVN1S3dZT3JrZ2hsTFRpNWFrS0dSbXh5bk5K?=
 =?utf-8?Q?cUKKvvmZOGhMmiOML6JVYHEsv?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	MV3+dJ3zhJhXgHIFB283y1adJQokLHr0VPiSQIx8R39R/plMN4gLEYg6YGBYwHChprmalbz699gMggk3mkLgjnjClQnvsNE5G+Ytfi7YxS9htUc8Fa1x1HD5RV1elXT0CBb3BgtqWtPqq8MZ8zkD0m7fS9whmOj/FzwOquk1CqWy4588uxEseX90/9QKGJCv8GJ7nawo/uEzFLFtZFxGApF/yYa97aCgcBfznQNdmdeo59nYEKhmDxwt9WR9BRRZwOKNiEZDTocZGbvlOkzRMlJ/v7xGUNlYZV8pP1YCsSr/lDoeDV9Ugy/sXJ3Ag9h1S+tng2cJlldx+gdtYBgAOfvukiHcNsJr3g0NYsh+S4Vk3gpRIhuQLF78lXkyc6ATlkt1cCWUtPRHYDjBzHIdrLxVfUf1GwOf+ACQUgtq/yaZn8g9/smCJYufs38xPhNbOL4fpeO13Vm4L//Iz+OeGe0FqRLIqtphUTy7BYcQFGNbd5i7R3wTKoad0ryXhXD1qa8NSY2R2xYdByFIX+JOTDcRNCpL6do0rtTsPXnSporazCzYC7KD7vjoAG34MjKrLpGDU2MZdRQ5gV+9taZokzx/o485oGoGlceQS4d3gCI=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 9add1b59-a0a7-472e-7990-08dc227e60f3
X-MS-Exchange-CrossTenant-AuthSource: BYAPR10MB2663.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 Jan 2024 17:02:16.8580
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 9jn2IsuIrxBhvSjNfMrPm2ElsKl7Y7M4RlEOP425riUe3GsDdROF0Xh6tJkWMjB51hUAuqSaoDVORKpCOdR1Rw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR10MB4580
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-01-31_10,2024-01-31_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 bulkscore=0 malwarescore=0
 mlxscore=0 mlxlogscore=999 spamscore=0 adultscore=0 suspectscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2311290000
 definitions=main-2401310131
X-Proofpoint-ORIG-GUID: Qzq0wbZMLSfkPodrOW-N7NnwtEPuunXY
X-Proofpoint-GUID: Qzq0wbZMLSfkPodrOW-N7NnwtEPuunXY


On 1/31/24 07:43, Sean Christopherson wrote:
> On Tue, Jan 23, 2024, Mingwei Zhang wrote:
>> Fix type length error since pmu->fixed_ctr_ctrl is u64 but the local
>> variable old_fixed_ctr_ctrl is u8. Truncating the value leads to
>> information loss at runtime. This leads to incorrect value in old_ctrl
>> retrieved from each field of old_fixed_ctr_ctrl and causes incorrect code
>> execution within the for loop of reprogram_fixed_counters(). So fix this
>> type to u64.
> 
> But what is the actual fallout from this?  Stating that the bug causes incorrect
> code execution isn't helpful, that's akin to saying water is wet.
> 
> If I'm following the code correctly, the only fallout is that KVM may unnecessarily
> mark a fixed PMC as in use and reprogram it.  I.e. the bug can result in (minor?)
> performance issues, but it won't cause functional problems.

My this issue cause "Uhhuh. NMI received for unknown reason XX on CPU XX." at VM side?

The PMC is still active while the VM side handle_pmi_common() is not going to handle it?

Thank you very much!

Dongli Zhang

> 
> Understanding what actually goes wrong matters, because I'm trying to determine
> whether or not this needs to be fixed in 6.8 and backported to stable trees.  If
> the bug is relatively benign, then this is fodder for 6.9.
> 
>> Fixes: 76d287b2342e ("KVM: x86/pmu: Drop "u8 ctrl, int idx" for reprogram_fixed_counter()")
>> Signed-off-by: Mingwei Zhang <mizhang@google.com>
>> ---
>>  arch/x86/kvm/vmx/pmu_intel.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
>> index a6216c874729..315c7c2ba89b 100644
>> --- a/arch/x86/kvm/vmx/pmu_intel.c
>> +++ b/arch/x86/kvm/vmx/pmu_intel.c
>> @@ -71,7 +71,7 @@ static int fixed_pmc_events[] = {
>>  static void reprogram_fixed_counters(struct kvm_pmu *pmu, u64 data)
>>  {
>>  	struct kvm_pmc *pmc;
>> -	u8 old_fixed_ctr_ctrl = pmu->fixed_ctr_ctrl;
>> +	u64 old_fixed_ctr_ctrl = pmu->fixed_ctr_ctrl;
>>  	int i;
>>  
>>  	pmu->fixed_ctr_ctrl = data;
>>
>> base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d
>> -- 
>> 2.43.0.429.g432eaa2c6b-goog
>>
>