On 6/21/22 1:44 AM, Damien Le Moal 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);
> 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.
MBR, Sergey
