Re: [PATCH] ata: libata-scsi: fix sloppy result type of ata_ioc32()

[Damien Le Moal <damien.lemoal@xxxxxxxxxxxxxxxxxx>]

On 6/21/22 05:26, Sergey Shtylyov wrote:
> On 6/20/22 2:12 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
@@ -556,6 +556,7 @@ int ata_sas_scsi_ioctl(struct ata_port *ap, struct
scsi_device *scsidev,
                     unsigned int cmd, void __user *arg)
 {
        unsigned long val;
+       bool pio32;
        int rc = -EINVAL;
        unsigned long flags;

@@ -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:
-               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);


> 
> MBR, Sergey


-- 
Damien Le Moal
Western Digital Research