[PATCH 0/3] ide: Fix use of paired device

[PATCH 0/3] ide: Fix use of paired device

[Benjamin Herrenschmidt]

At least 2 drivers (siimage and cs5535) have a bug where they use
the construct:

	ide_drive_t *pair	= &hwif->drives[drive->dn ^ 1];

To access the other drive in a master/slave pair. This is bogus
because drive->dn is not the unit number, but the global drive
number, thus can be 2 & 3 for ide1, 4 & 5 for ide2 etc...

This causes the driver to access beyond the drive array into lalaland
for any other interface than ide0 and in some case, actually crash :-)

These 3 patches fix those by introducing a ide_get_paired_drive()
helper that does the right thing and then using it.

Please apply to 2.6.24 if no objection.

[PATCH 1/3] ide: Add ide_get_paired_drive() helper

[Benjamin Herrenschmidt]

This adds a helper to get to the "other" drive on a pair connected
to a given hwif.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---

Note: You may want to use the 1 - select.b.unit construct instead,
as you prefer, I just used what was there and fixed it.

 include/linux/ide.h |    7 +++++++
 1 file changed, 7 insertions(+)

Index: linux-work/include/linux/ide.h
===================================================================
--- linux-work.orig/include/linux/ide.h	2007-10-18 10:41:42.000000000 +1000
+++ linux-work/include/linux/ide.h	2007-10-18 10:42:40.000000000 +1000
@@ -1454,4 +1454,11 @@ static inline int hwif_to_node(ide_hwif_
 	return dev ? pcibus_to_node(dev->bus) : -1;
 }
 
+static inline ide_drive_t *ide_get_paired_drive(ide_drive_t *drive)
+{
+	ide_hwif_t *hwif	= HWIF(drive);
+
+	return &hwif->drives[(drive->dn ^ 1) & 1];
+}
+
 #endif /* _IDE_H */

Re: [PATCH 1/3] ide: Add ide_get_paired_drive() helper

[Sergei Shtylyov]

Benjamin Herrenschmidt wrote:

> This adds a helper to get to the "other" drive on a pair connected
> to a given hwif.

    Maibe "mate drive" would've been a better name that "paired"...

> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Acked-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

MBR, Sergei

Re: [PATCH 1/3] ide: Add ide_get_paired_drive() helper

[Bartlomiej Zolnierkiewicz]

On Thursday 18 October 2007, Benjamin Herrenschmidt wrote:
> This adds a helper to get to the "other" drive on a pair connected
> to a given hwif.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

applied

-stable team: please include this patch series in 2.6.23.2

> ---
> 
> Note: You may want to use the 1 - select.b.unit construct instead,
> as you prefer, I just used what was there and fixed it.
> 
>  include/linux/ide.h |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> Index: linux-work/include/linux/ide.h
> ===================================================================
> --- linux-work.orig/include/linux/ide.h	2007-10-18 10:41:42.000000000 +1000
> +++ linux-work/include/linux/ide.h	2007-10-18 10:42:40.000000000 +1000
> @@ -1454,4 +1454,11 @@ static inline int hwif_to_node(ide_hwif_
>  	return dev ? pcibus_to_node(dev->bus) : -1;
>  }
>  
> +static inline ide_drive_t *ide_get_paired_drive(ide_drive_t *drive)
> +{
> +	ide_hwif_t *hwif	= HWIF(drive);
> +
> +	return &hwif->drives[(drive->dn ^ 1) & 1];
> +}
> +
>  #endif /* _IDE_H */

[PATCH 2/3] ide: Fix siimage driver accessing beyond array boundary

[Benjamin Herrenschmidt]

The siimage use an incorrect construct to access the other drive
of a pair, causing it to access beyond an array boundary on non-0
interfaces. This fixes it by using the new ide_get_paired_drive()
hepler instead.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---

 drivers/ide/pci/siimage.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-work/drivers/ide/pci/siimage.c
===================================================================
--- linux-work.orig/drivers/ide/pci/siimage.c	2007-10-18 10:42:56.000000000 +1000
+++ linux-work/drivers/ide/pci/siimage.c	2007-10-18 10:43:09.000000000 +1000
@@ -180,7 +180,7 @@ static void sil_set_pio_mode(ide_drive_t
 	const u16 data_speed[]	= { 0x328a, 0x2283, 0x1104, 0x10c3, 0x10c1 };
 
 	ide_hwif_t *hwif	= HWIF(drive);
-	ide_drive_t *pair	= &hwif->drives[drive->dn ^ 1];
+	ide_drive_t *pair	= ide_get_paired_drive(drive);
 	u32 speedt		= 0;
 	u16 speedp		= 0;
 	unsigned long addr	= siimage_seldev(drive, 0x04);

Re: [PATCH 2/3] ide: Fix siimage driver accessing beyond array boundary

[Sergei Shtylyov]

Benjamin Herrenschmidt wrote:

> The siimage use an incorrect construct to access the other drive
> of a pair, causing it to access beyond an array boundary on non-0
> interfaces. This fixes it by using the new ide_get_paired_drive()
> hepler instead.

> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

    Please update the driver version in the driver's heading comment. Otherwise

Acked-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

MBR, Sergei

Re: [PATCH 2/3] ide: Fix siimage driver accessing beyond array boundary

[Benjamin Herrenschmidt]

On Thu, 2007-10-18 at 16:02 +0400, Sergei Shtylyov wrote:
> Benjamin Herrenschmidt wrote:
> 
> > The siimage use an incorrect construct to access the other drive
> > of a pair, causing it to access beyond an array boundary on non-0
> > interfaces. This fixes it by using the new ide_get_paired_drive()
> > hepler instead.
> 
> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> 
>     Please update the driver version in the driver's heading comment. Otherwise
> 
> Acked-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

I don't believe much in driver versions in tree... the kernel version is
what matter... Bart, what's your stance there ?

Ben.

Re: [PATCH 2/3] ide: Fix siimage driver accessing beyond array boundary

[Bartlomiej Zolnierkiewicz]

On Thursday 18 October 2007, Benjamin Herrenschmidt wrote:
> 
> On Thu, 2007-10-18 at 16:02 +0400, Sergei Shtylyov wrote:
> > Benjamin Herrenschmidt wrote:
> > 
> > > The siimage use an incorrect construct to access the other drive
> > > of a pair, causing it to access beyond an array boundary on non-0
> > > interfaces. This fixes it by using the new ide_get_paired_drive()
> > > hepler instead.
> > 
> > > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > 
> >     Please update the driver version in the driver's heading comment. Otherwise
> > 
> > Acked-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>
> 
> I don't believe much in driver versions in tree... the kernel version is
> what matter... Bart, what's your stance there ?

Similar opinion here but sometimes driver versions are useful, i.e.:

* during development or while debugging regressions to distinguish
  real changes (which have to update driver version) from the core
  changes and trivial cleanups (which don't do this)

* bugreports with partial dmesg info (hmm, doesn't matter for IDE
  currently since we don't print this info - patches are welcomed)

* maybe some other that I forgot :)

I updated siimage driver version in separate commit
(no need to do it for cs5535 since it lacks driver version).

Thanks,
Bart

Re: [PATCH 2/3] ide: Fix siimage driver accessing beyond array boundary

[Bartlomiej Zolnierkiewicz]

On Thursday 18 October 2007, Benjamin Herrenschmidt wrote:
> The siimage use an incorrect construct to access the other drive
> of a pair, causing it to access beyond an array boundary on non-0
> interfaces. This fixes it by using the new ide_get_paired_drive()
> hepler instead.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

applied with patch description fixes

> ---
> 
>  drivers/ide/pci/siimage.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Index: linux-work/drivers/ide/pci/siimage.c
> ===================================================================
> --- linux-work.orig/drivers/ide/pci/siimage.c	2007-10-18 10:42:56.000000000 +1000
> +++ linux-work/drivers/ide/pci/siimage.c	2007-10-18 10:43:09.000000000 +1000
> @@ -180,7 +180,7 @@ static void sil_set_pio_mode(ide_drive_t
>  	const u16 data_speed[]	= { 0x328a, 0x2283, 0x1104, 0x10c3, 0x10c1 };
>  
>  	ide_hwif_t *hwif	= HWIF(drive);
> -	ide_drive_t *pair	= &hwif->drives[drive->dn ^ 1];
> +	ide_drive_t *pair	= ide_get_paired_drive(drive);
>  	u32 speedt		= 0;
>  	u16 speedp		= 0;
>  	unsigned long addr	= siimage_seldev(drive, 0x04);

[PATCH 3/3] ide: Fix cs5535 driver accessing beyond array boundary

[Benjamin Herrenschmidt]

The cs5535 use an incorrect construct to access the other drive
of a pair, causing it to access beyond an array boundary on non-0
interfaces. This fixes it by using the new ide_get_paired_drive()
hepler instead.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---

 drivers/ide/pci/cs5535.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-work/drivers/ide/pci/cs5535.c
===================================================================
--- linux-work.orig/drivers/ide/pci/cs5535.c	2007-10-18 10:43:39.000000000 +1000
+++ linux-work/drivers/ide/pci/cs5535.c	2007-10-18 10:44:00.000000000 +1000
@@ -84,7 +84,7 @@ static void cs5535_set_speed(ide_drive_t
 
 	/* Set the PIO timings */
 	if ((speed & XFER_MODE) == XFER_PIO) {
-		ide_drive_t *pair = &drive->hwif->drives[drive->dn ^ 1];
+		ide_drive_t *pair = ide_get_paired_drive(drive);
 		u8 cmd, pioa;
 
 		cmd = pioa = speed - XFER_PIO_0;

Re: [PATCH 3/3] ide: Fix cs5535 driver accessing beyond array boundary

[Sergei Shtylyov]

Benjamin Herrenschmidt wrote:

> The cs5535 use an incorrect construct to access the other drive
> of a pair, causing it to access beyond an array boundary on non-0
> interfaces. This fixes it by using the new ide_get_paired_drive()
> hepler instead.

> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

    Please update the driver version in the driver's heading comment. It also 
would be preferrable to fold this patch together with siimage path, or even 
fold all three together. Otherwise

Acked-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

MBR, Sergei

Re: [PATCH 3/3] ide: Fix cs5535 driver accessing beyond array boundary

[Bartlomiej Zolnierkiewicz]

On Thursday 18 October 2007, Benjamin Herrenschmidt wrote:
> The cs5535 use an incorrect construct to access the other drive
> of a pair, causing it to access beyond an array boundary on non-0
> interfaces. This fixes it by using the new ide_get_paired_drive()
> hepler instead.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

applied with patch description fixes

> ---
> 
>  drivers/ide/pci/cs5535.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Index: linux-work/drivers/ide/pci/cs5535.c
> ===================================================================
> --- linux-work.orig/drivers/ide/pci/cs5535.c	2007-10-18 10:43:39.000000000 +1000
> +++ linux-work/drivers/ide/pci/cs5535.c	2007-10-18 10:44:00.000000000 +1000
> @@ -84,7 +84,7 @@ static void cs5535_set_speed(ide_drive_t
>  
>  	/* Set the PIO timings */
>  	if ((speed & XFER_MODE) == XFER_PIO) {
> -		ide_drive_t *pair = &drive->hwif->drives[drive->dn ^ 1];
> +		ide_drive_t *pair = ide_get_paired_drive(drive);
>  		u8 cmd, pioa;
>  
>  		cmd = pioa = speed - XFER_PIO_0;

Re: [PATCH 0/3] ide: Fix use of paired device

[Sergei Shtylyov]

Hello.

Benjamin Herrenschmidt wrote:

> At least 2 drivers (siimage and cs5535) have a bug where they use
> the construct:

> 	ide_drive_t *pair	= &hwif->drives[drive->dn ^ 1];

> To access the other drive in a master/slave pair. This is bogus
> because drive->dn is not the unit number, but the global drive
> number, thus can be 2 & 3 for ide1, 4 & 5 for ide2 etc...

    Huh? With drive->dn calculated as:

         for (unit = 0; unit < MAX_DRIVES; ++unit) {
                 ide_drive_t *drive = &hwif->drives[unit];
                 drive->dn = (hwif->channel ? 2 : 0) + unit;

(with MAX_DRIVES always being 2) how comes it may be 4 or 5?!

> Please apply to 2.6.24 if no objection.

    I object. :-)

MBR, Sergei

Re: [PATCH 0/3] ide: Fix use of paired device

[Sergei Shtylyov]

Hello, I wrote:

>> At least 2 drivers (siimage and cs5535) have a bug where they use
>> the construct:

>>     ide_drive_t *pair    = &hwif->drives[drive->dn ^ 1];

>> To access the other drive in a master/slave pair. This is bogus
>> because drive->dn is not the unit number, but the global drive
>> number, thus can be 2 & 3 for ide1, 4 & 5 for ide2 etc...

>    Huh? With drive->dn calculated as:

>         for (unit = 0; unit < MAX_DRIVES; ++unit) {
>                 ide_drive_t *drive = &hwif->drives[unit];
>                 drive->dn = (hwif->channel ? 2 : 0) + unit;

> (with MAX_DRIVES always being 2) how comes it may be 4 or 5?!

>> Please apply to 2.6.24 if no objection.

>    I object. :-)

    Well, actually no objections about the patch itself since it deals with 
the cases of drive->dn being 2 and 3. But this probably be better to be done 
all in one patch, or at least in 2 patches (please also update driver version 
in the heading comment).

MBR, Sergei

Re: [PATCH 0/3] ide: Fix use of paired device

[Benjamin Herrenschmidt]

On Thu, 2007-10-18 at 15:54 +0400, Sergei Shtylyov wrote:
> Hello.
> 
> Benjamin Herrenschmidt wrote:
> 
> > At least 2 drivers (siimage and cs5535) have a bug where they use
> > the construct:
> 
> > 	ide_drive_t *pair	= &hwif->drives[drive->dn ^ 1];
> 
> > To access the other drive in a master/slave pair. This is bogus
> > because drive->dn is not the unit number, but the global drive
> > number, thus can be 2 & 3 for ide1, 4 & 5 for ide2 etc...
> 
>     Huh? With drive->dn calculated as:
> 
>          for (unit = 0; unit < MAX_DRIVES; ++unit) {
>                  ide_drive_t *drive = &hwif->drives[unit];
>                  drive->dn = (hwif->channel ? 2 : 0) + unit;
> 
> (with MAX_DRIVES always being 2) how comes it may be 4 or 5?!
> 
> > Please apply to 2.6.24 if no objection.
> 
>     I object. :-)

allright, I should have read the code more closely :-)

I saw drive->dn being 2 on ide1, so the code misbehaving because of
that, and thus drawn an incorrect conclusion.

Still.. the code is bogus and having an helper to get the paired device
makes sense, so all we need to do is fix the implementation of that
helper. The good thing is that I think my patch is still correct, though
the comment is not :-)

Ben.

Re: [PATCH 0/3] ide: Fix use of paired device

[Bartlomiej Zolnierkiewicz]

On Thursday 18 October 2007, Benjamin Herrenschmidt wrote:
> At least 2 drivers (siimage and cs5535) have a bug where they use
> the construct:
> 
> 	ide_drive_t *pair	= &hwif->drives[drive->dn ^ 1];
> 
> To access the other drive in a master/slave pair. This is bogus
> because drive->dn is not the unit number, but the global drive
> number, thus can be 2 & 3 for ide1, 4 & 5 for ide2 etc...
> 
> This causes the driver to access beyond the drive array into lalaland
> for any other interface than ide0 and in some case, actually crash :-)
> 
> These 3 patches fix those by introducing a ide_get_paired_drive()
> helper that does the right thing and then using it.
> 
> Please apply to 2.6.24 if no objection.

Thanks for debugging and fixing this!

All three patches applied.

Bart