Skip to contentSkip to conversation

libata: fix out-of-bounds access in pata_oldpiix.c

PATCH In review David Müller 4 participants 6 messages Jul 18, 2008 – Aug 3, 2008 libata

lore
Share & tools
lore

David Müller

author
#0 lore raw

PATCH 1/1 libata: fix out-of-bounds access in pata_oldpiix.c

The "pata_oldpiix" driver in linux-2.6.26 is calling its "set_dmamode" routine also locally, but under different preconditions as the corresponding call in libata-core.c. This may cause an "out-of-array bounds" access in "oldpiix_set_dmamode".

Signed-off-by Dave Mueller <[email protected]>
drivers/ata/pata_oldpiix.c 2008-07-18 08:18:45.000000000 +0200 renamed from drivers/ata/pata_oldpiix.c 2008-07-18 08:13:38.000000000 +0200 +1 -1
diff -dpurN a/drivers/ata/pata_oldpiix.c b/drivers/ata/pata_oldpiix.c
@@ -198,7 +198,7 @@ static unsigned int oldpiix_qc_issue(str
 
        if (adev != ap->private_data) {
                oldpiix_set_piomode(ap, adev);
-               if (adev->dma_mode)
+               if (adev->dma_mode != 0xff)
                        oldpiix_set_dmamode(ap, adev);
        }
        return ata_sff_qc_issue(qc);
#1 lore raw
Quoted 4 lines · On Fri, 18 Jul 2008 09:06:12 +0200 David Müller <[email protected]> wrote:

The "pata_oldpiix" driver in linux-2.6.26 is calling its "set_dmamode" routine also locally, but under different preconditions as the corresponding call in libata-core.c. This may cause an "out-of-array bounds" access in "oldpiix_set_dmamode".

This looks wrong adev->dma_mode should never be invalid at this point. Are you not confusing dma_mask and dma_mode. Can you provide the backtraces of the failing case and the actual chip variant you are using ?

Either way the fix is not in oldpiix if this is appearing as 0xFF but in the core code so NAK

#2 lore raw
Quoted 8 lines · Alan Cox
Quoted 4 lines · On Fri, 18 Jul 2008 09:06:12 +0200 David Müller <[email protected]> wrote:

The "pata_oldpiix" driver in linux-2.6.26 is calling its "set_dmamode" routine also locally, but under different preconditions as the corresponding call in libata-core.c. This may cause an "out-of-array bounds" access in "oldpiix_set_dmamode".

Quoted 6 lines

This looks wrong adev->dma_mode should never be invalid at this point. Are you not confusing dma_mask and dma_mode. Can you provide the backtraces of the failing case and the actual chip variant you are using ?

Either way the fix is not in oldpiix if this is appearing as 0xFF but in the core code so NAK

->dma_mode == 0xff means that DMA is unsupported by a given device and according to the core code it is a valid ->dma_mode setting.

This may want to be changed in the future but as the things are right now David's patch is correct and fixes a real bug. Please UNNAK.

Thanks,
Bart

#3 lore raw
Quoted 12 lines · Bartlomiej Zolnierkiewicz
Quoted 11 lines · Alan Cox
Quoted 4 lines · On Fri, 18 Jul 2008 09:06:12 +0200 David Müller <[email protected]> wrote:

The "pata_oldpiix" driver in linux-2.6.26 is calling its "set_dmamode" routine also locally, but under different preconditions as the corresponding call in libata-core.c. This may cause an "out-of-array bounds" access in "oldpiix_set_dmamode".

This looks wrong adev->dma_mode should never be invalid at this point. Are you not confusing dma_mask and dma_mode. Can you provide the backtraces of the failing case and the actual chip variant you are using ?

Quoted 8 lines
Quoted 2 lines

Either way the fix is not in oldpiix if this is appearing as 0xFF but in the core code so NAK

->dma_mode == 0xff means that DMA is unsupported by a given device and according to the core code it is a valid ->dma_mode setting.

This may want to be changed in the future but as the things are right now David's patch is correct and fixes a real bug. Please UNNAK.

Alan, David's patch is correct. 0xff indicates unsupported transfer mode (mainly because 0x00 is valid PIO mode). ->set_pio/dmamode won't see it as libata-core won't call them if unsupported but qc_issue will see it. Can you please unnak?

Signature · 1 line
tejun
#4 lore raw
Quoted 3 lines

see it as libata-core won't call them if unsupported but qc_issue will see it. Can you please unnak?

No. Please see the patch I posted. It got a bit stuck as I got no reply at all to the original query so it was filed on the low priority pile. Simply quick fixing one case isn't the way to sort a bug like that.

No un-nak as I've now been through and fixed the lot including a couple of really nasty assumptions that >= UDMA_0 means UDMA mode (which isnt true as 0xFF is no DMA)

#5 lore raw
Quoted 10 lines · Alan Cox
Quoted 2 lines

see it as libata-core won't call them if unsupported but qc_issue will see it. Can you please unnak?

No. Please see the patch I posted. It got a bit stuck as I got no reply at all to the original query so it was filed on the low priority pile. Simply quick fixing one case isn't the way to sort a bug like that.

No un-nak as I've now been through and fixed the lot including a couple of really nasty assumptions that >= UDMA_0 means UDMA mode (which isnt true as 0xFF is no DMA)

Ah... okay. That looks like a much better way to handle this. Let's continue there. Thanks.

Signature · 1 line
tejun