[PATCH] libata: Fix division by zero

[PATCH] libata: Fix division by zero

[Alan]

(Coverity 1192289, 1192292, 1192294)

We have several controllers that in some cases use ata_timing_compute but do
not do UDMA. They pass 0 for UT, which ends up with us doing a division by
zero. We could pass some other bogus value in or we could make the libata
code do the sensible thing and treat a UT of 0 as meaning "I'm not asking
about UDMA".

This patches does the latter which is IMHO the more robust option.

Signed-off-by: Alan Cox <alan@linux.intel.com>
---
 0 files changed

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index f6cb1f1..6b1bd36 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2966,7 +2966,8 @@ static void ata_timing_quantize(const struct ata_timing *t, struct ata_timing *q
 	q->recover	= EZ(t->recover    * 1000,  T);
 	q->dmack_hold	= EZ(t->dmack_hold * 1000,  T);
 	q->cycle	= EZ(t->cycle      * 1000,  T);
-	q->udma		= EZ(t->udma       * 1000, UT);
+	if (UT)
+		q->udma		= EZ(t->udma       * 1000, UT);
 }
 
 void ata_timing_merge(const struct ata_timing *a, const struct ata_timing *b,

Re: [PATCH] libata: Fix division by zero

[Bartlomiej Zolnierkiewicz]

Hi,

On Wednesday, April 08, 2015 01:51:36 PM Alan wrote:
> (Coverity 1192289, 1192292, 1192294)
> 
> We have several controllers that in some cases use ata_timing_compute but do
> not do UDMA. They pass 0 for UT, which ends up with us doing a division by
> zero. We could pass some other bogus value in or we could make the libata
> code do the sensible thing and treat a UT of 0 as meaning "I'm not asking
> about UDMA".
> 
> This patches does the latter which is IMHO the more robust option.

Please note that we should not end up with the division by zero with
existing host drivers.  EZ(v, unit) checks for v being zero and t->udma
is non-zero only for XFER_UDMA_* xfer modes.  Currently none of host
drivers calls ata_timing_compute() on xfer mode >= XFER_UDMA_0 with
the zero UT argument.  Such usage will be an error and while the old
code explodes immediately, the new one will silently accept it (which
could theoretically result in the wrong timing being set)..

Could you please enhance your patch with a validity code requiring
non-zero UT when speed >= XFER_UDMA_0 is used in ata_timing_compute()?

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

> Signed-off-by: Alan Cox <alan@linux.intel.com>
> ---
>  0 files changed
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index f6cb1f1..6b1bd36 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -2966,7 +2966,8 @@ static void ata_timing_quantize(const struct ata_timing *t, struct ata_timing *q
>  	q->recover	= EZ(t->recover    * 1000,  T);
>  	q->dmack_hold	= EZ(t->dmack_hold * 1000,  T);
>  	q->cycle	= EZ(t->cycle      * 1000,  T);
> -	q->udma		= EZ(t->udma       * 1000, UT);
> +	if (UT)
> +		q->udma		= EZ(t->udma       * 1000, UT);
>  }
>  
>  void ata_timing_merge(const struct ata_timing *a, const struct ata_timing *b,

Re: [PATCH] libata: Fix division by zero

[Bartlomiej Zolnierkiewicz]

On Wednesday, April 08, 2015 03:52:58 PM Bartlomiej Zolnierkiewicz wrote:
> 
> Hi,
> 
> On Wednesday, April 08, 2015 01:51:36 PM Alan wrote:
> > (Coverity 1192289, 1192292, 1192294)
> > 
> > We have several controllers that in some cases use ata_timing_compute but do
> > not do UDMA. They pass 0 for UT, which ends up with us doing a division by
> > zero. We could pass some other bogus value in or we could make the libata
> > code do the sensible thing and treat a UT of 0 as meaning "I'm not asking
> > about UDMA".
> > 
> > This patches does the latter which is IMHO the more robust option.
> 
> Please note that we should not end up with the division by zero with
> existing host drivers.  EZ(v, unit) checks for v being zero and t->udma
> is non-zero only for XFER_UDMA_* xfer modes.  Currently none of host

A bit more detailed explanation for xfer_modes < XFER_UDMA_0:

#define ENOUGH(v, unit)		(((v)-1)/(unit)+1)
#define EZ(v, unit)		((v)?ENOUGH(v, unit):0)

		q->udma		= EZ(t->udma       * 1000, UT);

for xfer modes < XFER_UDMA_0 t->udma is always zero so EZ() will
return zero without any division.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

> drivers calls ata_timing_compute() on xfer mode >= XFER_UDMA_0 with
> the zero UT argument.  Such usage will be an error and while the old
> code explodes immediately, the new one will silently accept it (which
> could theoretically result in the wrong timing being set)..
> 
> Could you please enhance your patch with a validity code requiring
> non-zero UT when speed >= XFER_UDMA_0 is used in ata_timing_compute()?
> 
> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics
> 
> > Signed-off-by: Alan Cox <alan@linux.intel.com>
> > ---
> >  0 files changed
> > 
> > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> > index f6cb1f1..6b1bd36 100644
> > --- a/drivers/ata/libata-core.c
> > +++ b/drivers/ata/libata-core.c
> > @@ -2966,7 +2966,8 @@ static void ata_timing_quantize(const struct ata_timing *t, struct ata_timing *q
> >  	q->recover	= EZ(t->recover    * 1000,  T);
> >  	q->dmack_hold	= EZ(t->dmack_hold * 1000,  T);
> >  	q->cycle	= EZ(t->cycle      * 1000,  T);
> > -	q->udma		= EZ(t->udma       * 1000, UT);
> > +	if (UT)
> > +		q->udma		= EZ(t->udma       * 1000, UT);
> >  }
> >  
> >  void ata_timing_merge(const struct ata_timing *a, const struct ata_timing *b,

Re: [PATCH] libata: Fix division by zero

[One Thousand Gnomes]

> Please note that we should not end up with the division by zero with
> existing host drivers.  EZ(v, unit) checks for v being zero and t->udma
> is non-zero only for XFER_UDMA_* xfer modes.  Currently none of host
> drivers calls ata_timing_compute() on xfer mode >= XFER_UDMA_0 with
> the zero UT argument.  Such usage will be an error and while the old
> code explodes immediately, the new one will silently accept it (which
> could theoretically result in the wrong timing being set)..

Having gone back over them with your additional point I missed noted all
seems to be good. I'll drop the patch from my queue. I don't see any
point adding extra checks - it's not as if anyone is writing many new
PATA drivers.

Alan