Re: [PATCH 20/32] drivers/ata: Stop using 32-bit MSR interfaces

["Jürgen Groß" <jgross@suse.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 2610 bytes --]

On 29.06.26 08:23, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Critical] The change to `wrmsrq()` in `cs5535_set_piomode` introduces a critical sign-extension bug, writing `0xFFFFFFFF` to the upper 32 bits of the MSR.
> - [Low] The commit message is incomplete and truncated mid-sentence.
> --
> 
> commit 9a4c6821b4ef60316ec2b36e0f505f4148809966
> Author: Juergen Gross <jgross@suse.com>
> 
> drivers/ata: Stop using 32-bit MSR interfaces
> 
> The 32-bit MSR interfaces rdmsr() and wrmsr() are planned to be removed.
> This patch switches the pata_cs5535 and pata_cs5536 drivers to use the
> 64-bit variants instead.
> 
>>      In drivers/ata/pata_cs5536.c don't redefine rdmsr() and wrmsr() as
>>      empty macros for avoiding misuse, but guard the affected code via
> 
> [Severity: Low]
> This isn't a bug, but the commit message appears to be truncated mid-sentence.

Oh, sorry, will update.

> 
>> diff --git a/drivers/ata/pata_cs5535.c b/drivers/ata/pata_cs5535.c
>> index d793fc441b467..da98390cc49e8 100644
>> --- a/drivers/ata/pata_cs5535.c
>> +++ b/drivers/ata/pata_cs5535.c
>> @@ -102,16 +102,16 @@ static void cs5535_set_piomode(struct ata_port *ap, struct ata_device *adev)
>>   		cmdmode = min(mode, pairmode);
>>   		/* Write the other drive timing register if it changed */
>>   		if (cmdmode < pairmode)
>> -			wrmsr(ATAC_CH0D0_PIO + 2 * pair->devno,
>> -				pio_cmd_timings[cmdmode] << 16 | pio_timings[pairmode], 0);
>> +			wrmsrq(ATAC_CH0D0_PIO + 2 * pair->devno,
>> +				pio_cmd_timings[cmdmode] << 16 | pio_timings[pairmode]);
>>   	}
>>   	/* Write the drive timing register */
>> -	wrmsr(ATAC_CH0D0_PIO + 2 * adev->devno,
>> -		pio_cmd_timings[cmdmode] << 16 | pio_timings[mode], 0);
>> +	wrmsrq(ATAC_CH0D0_PIO + 2 * adev->devno,
>> +		pio_cmd_timings[cmdmode] << 16 | pio_timings[mode]);
> 
> [Severity: Critical]
> Will these wrmsrq() calls write 0xFFFFFFFF into the upper 32 bits of the
> MSR for PIO mode 0?
> 
> Because pio_cmd_timings contains u16 values, C integer promotion rules will
> promote pio_cmd_timings[cmdmode] to a signed 32-bit int before the shift.
> 
> For PIO mode 0 (0xF7F4), the shift results in a negative 32-bit integer
> (0xF7F40000).
> 
> Since wrmsrq() takes a u64 parameter, this negative 32-bit int is implicitly
> sign-extended to 64 bits (0xFFFFFFFFF7F4XXXX). This corrupts the upper
> 32 bits of the timing registers, which could trigger a General Protection
> Fault.
> 

Right, will fix that.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]