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

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

[David Müller]

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 <dave.mueller@gmx.ch>

diff -dpurN a/drivers/ata/pata_oldpiix.c b/drivers/ata/pata_oldpiix.c
--- a/drivers/ata/pata_oldpiix.c   2008-07-18 08:13:38.000000000 +0200
+++ b/drivers/ata/pata_oldpiix.c   2008-07-18 08:18:45.000000000 +0200
@@ -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);

Re: [PATCH] libata: fix out-of-bounds access in pata_oldpiix.c

[Alan Cox]

On Fri, 18 Jul 2008 09:06:12 +0200
David Müller <dave.mueller@gmx.ch> 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

Re: [PATCH] libata: fix out-of-bounds access in pata_oldpiix.c

[Bartlomiej Zolnierkiewicz]

On Friday 18 July 2008, Alan Cox wrote:
> On Fri, 18 Jul 2008 09:06:12 +0200
> David Müller <dave.mueller@gmx.ch> 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

->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

Re: [PATCH] libata: fix out-of-bounds access in pata_oldpiix.c

[Tejun Heo]

Bartlomiej Zolnierkiewicz wrote:
> On Friday 18 July 2008, Alan Cox wrote:
>> On Fri, 18 Jul 2008 09:06:12 +0200
>> David Müller <dave.mueller@gmx.ch> 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
> 
> ->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?

-- 
tejun

Re: [PATCH] libata: fix out-of-bounds access in pata_oldpiix.c

[Alan Cox]

> 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)

Re: [PATCH] libata: fix out-of-bounds access in pata_oldpiix.c

[Tejun Heo]

Alan Cox wrote:
>> 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.

-- 
tejun