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

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

[Sergey Shtylyov]

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@omp.ru>

---
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;

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

[Damien Le Moal]

On 6/18/22 04:30, 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@omp.ru>
> 
> ---
> 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
that type since the val argument of the ioctl is also used as a bool.

-- 
Damien Le Moal
Western Digital Research

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

[Sergey Shtylyov]

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@omp.ru>
>>
>> ---
>> 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()?

> 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*)?

MBR, Sergey

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

[Damien Le Moal]

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@omp.ru>
>>>
>>> ---
>>> 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

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

[Sergey Shtylyov]

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@omp.ru>
>>>>
>>>> ---
>>>> 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

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

[Sergey Shtylyov]

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@omp.ru>
>>>>>
>>>>> ---
>>>>> 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

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

[Damien Le Moal]

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@omp.ru>
>>>>>>
>>>>>> ---
>>>>>> 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