Skip to contentSkip to conversation

libata: Fix division by zero

PATCH In review Alan 2 participants 4 messages Apr 8, 2015 libata

lore
Share & tools
lore

Alan

author
#0 lore raw

PATCH 1/1 libata: Fix division by zero

(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 <[email protected]>

---

 0 files changed
drivers/ata/libata-core.c +2 -1
index f6cb1f1..6b1bd36 100644
@@ -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,
#1 lore raw

Hi,

Quoted 9 lines · 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.

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

Quoted 18 lines
Signed-off-by Alan Cox <[email protected]>

---

 0 files changed
drivers/ata/libata-core.c +2 -1
index f6cb1f1..6b1bd36 100644
@@ -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,
#2 lore raw
Quoted 12 lines · Bartlomiej Zolnierkiewicz

Hi,

Quoted 8 lines · 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".

Quoted 5 lines
Quoted 1 line

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

Quoted 32 lines

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

Quoted 18 lines
Signed-off-by Alan Cox <[email protected]>

---

 0 files changed
drivers/ata/libata-core.c +2 -1
index f6cb1f1..6b1bd36 100644
@@ -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,
author
#3 lore raw
In reply to Bartlomiej Zolnierkiewicz
Quoted 7 lines

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