| Message ID | 4a695619-2de7-671e-7b67-2afddf22423f@omp.ru |
|---|---|
| State | New |
| Headers | show |
| Series | ata: libata-scsi: fix sloppy result type of ata_ioc32() | expand |
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.
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
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
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
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
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
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;
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(-)