On 6/21/22 10:14 PM, Sergey Shtylyov wrote:
[...]
>>>>> While ata_ioc32() returns *int*, its result gets assigned to and compared
>>>>> with the *unsigned long* variable 'val' in ata_sas_scsi_ioctl(), its only
>>>>> caller, which implies a problematic implicit cast -- fix that by returning
>>>>> *unsigned long* instead.
>>>>>
>>>>> Found by Linux Verification Center (linuxtesting.org) with the SVACE static
>>>>> analysis tool.
>>>>>
>>>>> Signed-off-by: Sergey Shtylyov <s.shtylyov@xxxxxx>
>>>>>
>>>>> ---
>>>>> This patch is against the 'for-next' branch of Damien's 'libata.git' repo.
>>>>>
>>>>> drivers/ata/libata-scsi.c | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> Index: libata/drivers/ata/libata-scsi.c
>>>>> ===================================================================
>>>>> --- libata.orig/drivers/ata/libata-scsi.c
>>>>> +++ libata/drivers/ata/libata-scsi.c
>>>>> @@ -539,7 +539,7 @@ int ata_task_ioctl(struct scsi_device *s
>>>>> return rc;
>>>>> }
>>>>>
>>>>> -static int ata_ioc32(struct ata_port *ap)
>>>>> +static unsigned long ata_ioc32(struct ata_port *ap)
>>>>> {
>>>>> if (ap->flags & ATA_FLAG_PIO_DMA)
>>>>> return 1;
>>>
>>>> Actually, this should be a bool I think and the ioctl code cleaned to use
>>>
>>> By the ioctl code you mean ata_sas_scsi_ioctl()?
>>
>> yes.
>>
>>>> that type since the val argument of the ioctl is also used as a bool.
>>>
>>> As for HDIO_SET_32BIT, that's prolly OK but what to do with HDIO_GET_32BIT
>>> (it calls put_user() on *unsigned long*)?
>>
>> Something like this should work fine:
>>
>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
>> index 86dbb1cdfabd..ec7f79cbb135 100644
>> --- a/drivers/ata/libata-scsi.c
>> +++ b/drivers/ata/libata-scsi.c
> [...]
>> @@ -571,16 +572,16 @@ int ata_sas_scsi_ioctl(struct ata_port *ap, struct
>> scsi_device *scsidev,
>> return put_user(val, (unsigned long __user *)arg);
>>
>> case HDIO_SET_32BIT:
>
> Hmm, I told you this *case* is prolly OK -- it was HDIO_GET_32BIT *case* that
> I was concerned about... So you mean that HDIO_GET_32BIT handling should remain
> intact?
>
>> - val = (unsigned long) arg;
>> + pio32 = !!((unsigned long) arg);
No, this one won't do -- it changes the behavior in case ATA_PFLAG_PIO32CHANGE
isn't set... :-/
>> rc = 0;
>> spin_lock_irqsave(ap->lock, flags);
>> if (ap->pflags & ATA_PFLAG_PIO32CHANGE) {
>> - if (val)
>> + if (pio32)
>> ap->pflags |= ATA_PFLAG_PIO32;
>> else
>> ap->pflags &= ~ATA_PFLAG_PIO32;
>> } else {
>> - if (val != ata_ioc32(ap))
>> + if (pio32 != ata_ioc32(ap))
>> rc = -EINVAL;
>> }
>> spin_unlock_irqrestore(ap->lock, flags);
>
> Not really sure this is worth it... *unsigned long* result type for
> ata_ioc32() seems simpler.
Actually, even just modifying ata_ioc32() to return 'bool' produces
a seemingly correct code. Note that ata_ioc32() is inlined in any case...
MBR, Sergey
