On 6/22/22 05:37, Sergey Shtylyov wrote:
> 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...
If there are no issues with the bool type conversion, I would really
prefer that rather than the unsigned long route. The latter is really
about silencing a static analyzer rather than ideal code. Given the name
of the function, returning an unsigned long is really strange.
>
> MBR, Sergey
--
Damien Le Moal
Western Digital Research
