* [PATCH] ata: libata-scsi: fix sloppy result type of ata_ioc32()
@ 2022-06-17 19:30 Sergey Shtylyov
2022-06-19 23:12 ` Damien Le Moal
0 siblings, 1 reply; 7+ messages in thread
From: Sergey Shtylyov @ 2022-06-17 19:30 UTC (permalink / raw)
To: Damien Le Moal, linux-ide
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;
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ata: libata-scsi: fix sloppy result type of ata_ioc32()
2022-06-17 19:30 [PATCH] ata: libata-scsi: fix sloppy result type of ata_ioc32() Sergey Shtylyov
@ 2022-06-19 23:12 ` Damien Le Moal
2022-06-20 20:26 ` Sergey Shtylyov
0 siblings, 1 reply; 7+ messages in thread
From: Damien Le Moal @ 2022-06-19 23:12 UTC (permalink / raw)
To: Sergey Shtylyov, linux-ide
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ata: libata-scsi: fix sloppy result type of ata_ioc32()
2022-06-19 23:12 ` Damien Le Moal
@ 2022-06-20 20:26 ` Sergey Shtylyov
2022-06-20 22:44 ` Damien Le Moal
0 siblings, 1 reply; 7+ messages in thread
From: Sergey Shtylyov @ 2022-06-20 20:26 UTC (permalink / raw)
To: Damien Le Moal, linux-ide
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ata: libata-scsi: fix sloppy result type of ata_ioc32()
2022-06-20 20:26 ` Sergey Shtylyov
@ 2022-06-20 22:44 ` Damien Le Moal
2022-06-21 19:14 ` Sergey Shtylyov
0 siblings, 1 reply; 7+ messages in thread
From: Damien Le Moal @ 2022-06-20 22:44 UTC (permalink / raw)
To: Sergey Shtylyov, linux-ide
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] ata: libata-scsi: fix sloppy result type of ata_ioc32()
2022-06-20 22:44 ` Damien Le Moal
@ 2022-06-21 19:14 ` Sergey Shtylyov
2022-06-21 20:37 ` Sergey Shtylyov
0 siblings, 1 reply; 7+ messages in thread
From: Sergey Shtylyov @ 2022-06-21 19:14 UTC (permalink / raw)
To: Damien Le Moal, linux-ide
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ata: libata-scsi: fix sloppy result type of ata_ioc32()
2022-06-21 19:14 ` Sergey Shtylyov
@ 2022-06-21 20:37 ` Sergey Shtylyov
2022-06-21 22:15 ` Damien Le Moal
0 siblings, 1 reply; 7+ messages in thread
From: Sergey Shtylyov @ 2022-06-21 20:37 UTC (permalink / raw)
To: Damien Le Moal, linux-ide
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ata: libata-scsi: fix sloppy result type of ata_ioc32()
2022-06-21 20:37 ` Sergey Shtylyov
@ 2022-06-21 22:15 ` Damien Le Moal
0 siblings, 0 replies; 7+ messages in thread
From: Damien Le Moal @ 2022-06-21 22:15 UTC (permalink / raw)
To: Sergey Shtylyov, linux-ide
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-06-21 22:15 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-17 19:30 [PATCH] ata: libata-scsi: fix sloppy result type of ata_ioc32() Sergey Shtylyov
2022-06-19 23:12 ` Damien Le Moal
2022-06-20 20:26 ` Sergey Shtylyov
2022-06-20 22:44 ` Damien Le Moal
2022-06-21 19:14 ` Sergey Shtylyov
2022-06-21 20:37 ` Sergey Shtylyov
2022-06-21 22:15 ` Damien Le Moal
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.