Message ID | 20230818234903.9226-2-schmitzmic@gmail.com |
---|---|
State | New |
Headers | show |
Series | Q40 IDE fixes | expand |
Hi Michael, On Sat, Aug 19, 2023 at 1:49 AM Michael Schmitz <schmitzmic@gmail.com> wrote: > With commit 44b1fbc0f5f3 ("m68k/q40: Replace q40ide driver > with pata_falcon and falconide"), the Q40 IDE driver was > replaced by pata_falcon.c. > > Both IO and memory resources were defined for the Q40 IDE > platform device, but definition of the IDE register addresses > was modeled after the Falcon case, both in use of the memory > resources and in including register scale and byte vs. word > offset in the address. > > This was correct for the Falcon case, which does not apply > any address translation to the register addresses. In the > Q40 case, all of device base address, byte access offset > and register scaling is included in the platform specific > ISA access translation (in asm/mm_io.h). > > As a consequence, such address translation gets applied > twice, and register addresses are mangled. > > Use the device base address from the platform IO resource, > and use standard register offsets from that base in order > to calculate register addresses (the IO address translation > will then apply the correct ISA window base and scaling). > > Encode PIO_OFFSET into IO port addresses for all registers > except the data transfer register. Encode the MMIO offset > there (pata_falcon_data_xfer() directly uses raw IO with > no address translation). > > Reported-by: William R Sowerbutts <will@sowerbutts.com> > Closes: https://lore.kernel.org/r/CAMuHMdUU62jjunJh9cqSqHT87B0H0A4udOOPs=WN7WZKpcagVA@mail.gmail.com > Link: https://lore.kernel.org/r/CAMuHMdUU62jjunJh9cqSqHT87B0H0A4udOOPs=WN7WZKpcagVA@mail.gmail.com > Fixes: 44b1fbc0f5f3 ("m68k/q40: Replace q40ide driver with pata_falcon and falconide") > Cc: stable@vger.kernel.org > Cc: Finn Thain <fthain@linux-m68k.org> > Cc: Geert Uytterhoeven <geert@linux-m68k.org> > Tested-by: William R Sowerbutts <will@sowerbutts.com> > Signed-off-by: Michael Schmitz <schmitzmic@gmail.com> Thanks for the update! > --- a/drivers/ata/pata_falcon.c > +++ b/drivers/ata/pata_falcon.c > @@ -165,26 +165,39 @@ static int __init pata_falcon_init_one(struct platform_device *pdev) > ap->pio_mask = ATA_PIO4; > ap->flags |= ATA_FLAG_SLAVE_POSS | ATA_FLAG_NO_IORDY; > > - base = (void __iomem *)base_mem_res->start; > /* N.B. this assumes data_addr will be used for word-sized I/O only */ > - ap->ioaddr.data_addr = base + 0 + 0 * 4; > - ap->ioaddr.error_addr = base + 1 + 1 * 4; > - ap->ioaddr.feature_addr = base + 1 + 1 * 4; > - ap->ioaddr.nsect_addr = base + 1 + 2 * 4; > - ap->ioaddr.lbal_addr = base + 1 + 3 * 4; > - ap->ioaddr.lbam_addr = base + 1 + 4 * 4; > - ap->ioaddr.lbah_addr = base + 1 + 5 * 4; > - ap->ioaddr.device_addr = base + 1 + 6 * 4; > - ap->ioaddr.status_addr = base + 1 + 7 * 4; > - ap->ioaddr.command_addr = base + 1 + 7 * 4; > - > - base = (void __iomem *)ctl_mem_res->start; > - ap->ioaddr.altstatus_addr = base + 1; > - ap->ioaddr.ctl_addr = base + 1; > - > - ata_port_desc(ap, "cmd 0x%lx ctl 0x%lx", > - (unsigned long)base_mem_res->start, > - (unsigned long)ctl_mem_res->start); > + ap->ioaddr.data_addr = (void __iomem *)base_mem_res->start; > + > + if (base_res) { /* only Q40 has IO resources */ > + io_offset = 0x10000; > + reg_scale = 1; > + base = (void __iomem *)base_res->start; > + ctl_base = (void __iomem *)ctl_res->start; > + > + ata_port_desc(ap, "cmd %pa ctl %pa", > + &base_res->start, > + &ctl_res->start); This can be moved outside the else, using %px to format base and ctl_base. > + } else { > + base = (void __iomem *)base_mem_res->start; > + ctl_base = (void __iomem *)ctl_mem_res->start; > + > + ata_port_desc(ap, "cmd %pa ctl %pa", > + &base_mem_res->start, > + &ctl_mem_res->start); > + } Gr{oetje,eeting}s, Geert
Hi Geert, On 21/08/23 19:50, Geert Uytterhoeven wrote: > Hi Michael, > > On Sat, Aug 19, 2023 at 1:49 AM Michael Schmitz <schmitzmic@gmail.com> wrote: >> With commit 44b1fbc0f5f3 ("m68k/q40: Replace q40ide driver >> with pata_falcon and falconide"), the Q40 IDE driver was >> replaced by pata_falcon.c. >> >> Both IO and memory resources were defined for the Q40 IDE >> platform device, but definition of the IDE register addresses >> was modeled after the Falcon case, both in use of the memory >> resources and in including register scale and byte vs. word >> offset in the address. >> >> This was correct for the Falcon case, which does not apply >> any address translation to the register addresses. In the >> Q40 case, all of device base address, byte access offset >> and register scaling is included in the platform specific >> ISA access translation (in asm/mm_io.h). >> >> As a consequence, such address translation gets applied >> twice, and register addresses are mangled. >> >> Use the device base address from the platform IO resource, >> and use standard register offsets from that base in order >> to calculate register addresses (the IO address translation >> will then apply the correct ISA window base and scaling). >> >> Encode PIO_OFFSET into IO port addresses for all registers >> except the data transfer register. Encode the MMIO offset >> there (pata_falcon_data_xfer() directly uses raw IO with >> no address translation). >> >> Reported-by: William R Sowerbutts <will@sowerbutts.com> >> Closes: https://lore.kernel.org/r/CAMuHMdUU62jjunJh9cqSqHT87B0H0A4udOOPs=WN7WZKpcagVA@mail.gmail.com >> Link: https://lore.kernel.org/r/CAMuHMdUU62jjunJh9cqSqHT87B0H0A4udOOPs=WN7WZKpcagVA@mail.gmail.com >> Fixes: 44b1fbc0f5f3 ("m68k/q40: Replace q40ide driver with pata_falcon and falconide") >> Cc: stable@vger.kernel.org >> Cc: Finn Thain <fthain@linux-m68k.org> >> Cc: Geert Uytterhoeven <geert@linux-m68k.org> >> Tested-by: William R Sowerbutts <will@sowerbutts.com> >> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com> > Thanks for the update! > >> --- a/drivers/ata/pata_falcon.c >> +++ b/drivers/ata/pata_falcon.c >> @@ -165,26 +165,39 @@ static int __init pata_falcon_init_one(struct platform_device *pdev) >> ap->pio_mask = ATA_PIO4; >> ap->flags |= ATA_FLAG_SLAVE_POSS | ATA_FLAG_NO_IORDY; >> >> - base = (void __iomem *)base_mem_res->start; >> /* N.B. this assumes data_addr will be used for word-sized I/O only */ >> - ap->ioaddr.data_addr = base + 0 + 0 * 4; >> - ap->ioaddr.error_addr = base + 1 + 1 * 4; >> - ap->ioaddr.feature_addr = base + 1 + 1 * 4; >> - ap->ioaddr.nsect_addr = base + 1 + 2 * 4; >> - ap->ioaddr.lbal_addr = base + 1 + 3 * 4; >> - ap->ioaddr.lbam_addr = base + 1 + 4 * 4; >> - ap->ioaddr.lbah_addr = base + 1 + 5 * 4; >> - ap->ioaddr.device_addr = base + 1 + 6 * 4; >> - ap->ioaddr.status_addr = base + 1 + 7 * 4; >> - ap->ioaddr.command_addr = base + 1 + 7 * 4; >> - >> - base = (void __iomem *)ctl_mem_res->start; >> - ap->ioaddr.altstatus_addr = base + 1; >> - ap->ioaddr.ctl_addr = base + 1; >> - >> - ata_port_desc(ap, "cmd 0x%lx ctl 0x%lx", >> - (unsigned long)base_mem_res->start, >> - (unsigned long)ctl_mem_res->start); >> + ap->ioaddr.data_addr = (void __iomem *)base_mem_res->start; >> + >> + if (base_res) { /* only Q40 has IO resources */ >> + io_offset = 0x10000; >> + reg_scale = 1; >> + base = (void __iomem *)base_res->start; >> + ctl_base = (void __iomem *)ctl_res->start; >> + >> + ata_port_desc(ap, "cmd %pa ctl %pa", >> + &base_res->start, >> + &ctl_res->start); > This can be moved outside the else, using %px to format base and > ctl_base. Right - do we need some additional message spelling out what address Q40 uses for data transfers? (Redundant for Falcon, of course ...) Though that could be handled outside the else, too: ata_port_desc(ap, "cmd %px ctl %px data %pa", base, ctl_base, &ap->ioaddr.data_addr); Cheers, Michael >> + } else { >> + base = (void __iomem *)base_mem_res->start; >> + ctl_base = (void __iomem *)ctl_mem_res->start; >> + >> + ata_port_desc(ap, "cmd %pa ctl %pa", >> + &base_mem_res->start, >> + &ctl_mem_res->start); >> + } > Gr{oetje,eeting}s, > > Geert >
Hi Michael, On Tue, Aug 22, 2023 at 1:57 AM Michael Schmitz <schmitzmic@gmail.com> wrote: > On 21/08/23 19:50, Geert Uytterhoeven wrote: > > On Sat, Aug 19, 2023 at 1:49 AM Michael Schmitz <schmitzmic@gmail.com> wrote: > >> With commit 44b1fbc0f5f3 ("m68k/q40: Replace q40ide driver > >> with pata_falcon and falconide"), the Q40 IDE driver was > >> replaced by pata_falcon.c. > >> > >> Both IO and memory resources were defined for the Q40 IDE > >> platform device, but definition of the IDE register addresses > >> was modeled after the Falcon case, both in use of the memory > >> resources and in including register scale and byte vs. word > >> offset in the address. > >> > >> This was correct for the Falcon case, which does not apply > >> any address translation to the register addresses. In the > >> Q40 case, all of device base address, byte access offset > >> and register scaling is included in the platform specific > >> ISA access translation (in asm/mm_io.h). > >> > >> As a consequence, such address translation gets applied > >> twice, and register addresses are mangled. > >> > >> Use the device base address from the platform IO resource, > >> and use standard register offsets from that base in order > >> to calculate register addresses (the IO address translation > >> will then apply the correct ISA window base and scaling). > >> > >> Encode PIO_OFFSET into IO port addresses for all registers > >> except the data transfer register. Encode the MMIO offset > >> there (pata_falcon_data_xfer() directly uses raw IO with > >> no address translation). > >> > >> Reported-by: William R Sowerbutts <will@sowerbutts.com> > >> Closes: https://lore.kernel.org/r/CAMuHMdUU62jjunJh9cqSqHT87B0H0A4udOOPs=WN7WZKpcagVA@mail.gmail.com > >> Link: https://lore.kernel.org/r/CAMuHMdUU62jjunJh9cqSqHT87B0H0A4udOOPs=WN7WZKpcagVA@mail.gmail.com > >> Fixes: 44b1fbc0f5f3 ("m68k/q40: Replace q40ide driver with pata_falcon and falconide") > >> Cc: stable@vger.kernel.org > >> Cc: Finn Thain <fthain@linux-m68k.org> > >> Cc: Geert Uytterhoeven <geert@linux-m68k.org> > >> Tested-by: William R Sowerbutts <will@sowerbutts.com> > >> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com> > > Thanks for the update! > > > >> --- a/drivers/ata/pata_falcon.c > >> +++ b/drivers/ata/pata_falcon.c > >> @@ -165,26 +165,39 @@ static int __init pata_falcon_init_one(struct platform_device *pdev) > >> ap->pio_mask = ATA_PIO4; > >> ap->flags |= ATA_FLAG_SLAVE_POSS | ATA_FLAG_NO_IORDY; > >> > >> - base = (void __iomem *)base_mem_res->start; > >> /* N.B. this assumes data_addr will be used for word-sized I/O only */ > >> - ap->ioaddr.data_addr = base + 0 + 0 * 4; > >> - ap->ioaddr.error_addr = base + 1 + 1 * 4; > >> - ap->ioaddr.feature_addr = base + 1 + 1 * 4; > >> - ap->ioaddr.nsect_addr = base + 1 + 2 * 4; > >> - ap->ioaddr.lbal_addr = base + 1 + 3 * 4; > >> - ap->ioaddr.lbam_addr = base + 1 + 4 * 4; > >> - ap->ioaddr.lbah_addr = base + 1 + 5 * 4; > >> - ap->ioaddr.device_addr = base + 1 + 6 * 4; > >> - ap->ioaddr.status_addr = base + 1 + 7 * 4; > >> - ap->ioaddr.command_addr = base + 1 + 7 * 4; > >> - > >> - base = (void __iomem *)ctl_mem_res->start; > >> - ap->ioaddr.altstatus_addr = base + 1; > >> - ap->ioaddr.ctl_addr = base + 1; > >> - > >> - ata_port_desc(ap, "cmd 0x%lx ctl 0x%lx", > >> - (unsigned long)base_mem_res->start, > >> - (unsigned long)ctl_mem_res->start); > >> + ap->ioaddr.data_addr = (void __iomem *)base_mem_res->start; > >> + > >> + if (base_res) { /* only Q40 has IO resources */ > >> + io_offset = 0x10000; > >> + reg_scale = 1; > >> + base = (void __iomem *)base_res->start; > >> + ctl_base = (void __iomem *)ctl_res->start; > >> + > >> + ata_port_desc(ap, "cmd %pa ctl %pa", > >> + &base_res->start, > >> + &ctl_res->start); > > This can be moved outside the else, using %px to format base and > > ctl_base. > > Right - do we need some additional message spelling out what address Q40 > uses for data transfers? (Redundant for Falcon, of course ...) > > Though that could be handled outside the else, too: > > ata_port_desc(ap, "cmd %px ctl %px data %pa", > base, ctl_base, &ap->ioaddr.data_addr); I guess that wouldn't hurt. Gr{oetje,eeting}s, Geert
Hi Geert, Am 22.08.2023 um 19:05 schrieb Geert Uytterhoeven: >>>> + if (base_res) { /* only Q40 has IO resources */ >>>> + io_offset = 0x10000; >>>> + reg_scale = 1; >>>> + base = (void __iomem *)base_res->start; >>>> + ctl_base = (void __iomem *)ctl_res->start; >>>> + >>>> + ata_port_desc(ap, "cmd %pa ctl %pa", >>>> + &base_res->start, >>>> + &ctl_res->start); >>> This can be moved outside the else, using %px to format base and >>> ctl_base. >> >> Right - do we need some additional message spelling out what address Q40 >> uses for data transfers? (Redundant for Falcon, of course ...) >> >> Though that could be handled outside the else, too: >> >> ata_port_desc(ap, "cmd %px ctl %px data %pa", >> base, ctl_base, &ap->ioaddr.data_addr); > > I guess that wouldn't hurt. Done - I'll send out v4 tomorrow. Cheers, Michael > > Gr{oetje,eeting}s, > > Geert >
Hi Geert, Am 21.08.2023 um 19:50 schrieb Geert Uytterhoeven: >> + ap->ioaddr.data_addr = (void __iomem *)base_mem_res->start; >> + >> + if (base_res) { /* only Q40 has IO resources */ >> + io_offset = 0x10000; >> + reg_scale = 1; >> + base = (void __iomem *)base_res->start; >> + ctl_base = (void __iomem *)ctl_res->start; >> + >> + ata_port_desc(ap, "cmd %pa ctl %pa", >> + &base_res->start, >> + &ctl_res->start); > > This can be moved outside the else, using %px to format base and > ctl_base. I get a checkpatch warning for %px, but not for %pa (used for . &ap->ioaddr.data_addr). What gives? WARNING: Using vsprintf specifier '%px' potentially exposes the kernel memory layout, if you don't really need the address please consider using '%p'. #148: FILE: drivers/ata/pata_falcon.c:194: + ata_port_desc(ap, "cmd %px ctl %px data %pa", + base, ctl_base, &ap->ioaddr.data_addr); Using %pa and &base, &ctl_base just to shut that up seems a little silly ... Cheers, Michael > >> + } else { >> + base = (void __iomem *)base_mem_res->start; >> + ctl_base = (void __iomem *)ctl_mem_res->start; >> + >> + ata_port_desc(ap, "cmd %pa ctl %pa", >> + &base_mem_res->start, >> + &ctl_mem_res->start); >> + } > > Gr{oetje,eeting}s, > > Geert >
Hi Michael, On Tue, Aug 22, 2023 at 10:27 AM Michael Schmitz <schmitzmic@gmail.com> wrote: > Am 21.08.2023 um 19:50 schrieb Geert Uytterhoeven: > >> + ap->ioaddr.data_addr = (void __iomem *)base_mem_res->start; > >> + > >> + if (base_res) { /* only Q40 has IO resources */ > >> + io_offset = 0x10000; > >> + reg_scale = 1; > >> + base = (void __iomem *)base_res->start; > >> + ctl_base = (void __iomem *)ctl_res->start; > >> + > >> + ata_port_desc(ap, "cmd %pa ctl %pa", > >> + &base_res->start, > >> + &ctl_res->start); > > > > This can be moved outside the else, using %px to format base and > > ctl_base. > > I get a checkpatch warning for %px, but not for %pa (used for . > &ap->ioaddr.data_addr). What gives? > > WARNING: Using vsprintf specifier '%px' potentially exposes the kernel > memory layout, if you don't really need the address please consider > using '%p'. > #148: FILE: drivers/ata/pata_falcon.c:194: > + ata_port_desc(ap, "cmd %px ctl %px data %pa", > + base, ctl_base, &ap->ioaddr.data_addr); You can ignore this. %p prints obfuscated pointer values, so they are useless here. %px prints an unobfuscated pointer value, which is fine here, as it's not a pointer to kernel memory that an attacker can use, but just the virtual address of an I/O device mapped one-to-one. Gr{oetje,eeting}s, Geert
diff --git a/drivers/ata/pata_falcon.c b/drivers/ata/pata_falcon.c index 996516e64f13..346259e3bbc8 100644 --- a/drivers/ata/pata_falcon.c +++ b/drivers/ata/pata_falcon.c @@ -123,8 +123,8 @@ static int __init pata_falcon_init_one(struct platform_device *pdev) struct resource *base_res, *ctl_res, *irq_res; struct ata_host *host; struct ata_port *ap; - void __iomem *base; - int irq = 0; + void __iomem *base, *ctl_base; + int irq = 0, io_offset = 1, reg_scale = 4; dev_info(&pdev->dev, "Atari Falcon and Q40/Q60 PATA controller\n"); @@ -165,26 +165,39 @@ static int __init pata_falcon_init_one(struct platform_device *pdev) ap->pio_mask = ATA_PIO4; ap->flags |= ATA_FLAG_SLAVE_POSS | ATA_FLAG_NO_IORDY; - base = (void __iomem *)base_mem_res->start; /* N.B. this assumes data_addr will be used for word-sized I/O only */ - ap->ioaddr.data_addr = base + 0 + 0 * 4; - ap->ioaddr.error_addr = base + 1 + 1 * 4; - ap->ioaddr.feature_addr = base + 1 + 1 * 4; - ap->ioaddr.nsect_addr = base + 1 + 2 * 4; - ap->ioaddr.lbal_addr = base + 1 + 3 * 4; - ap->ioaddr.lbam_addr = base + 1 + 4 * 4; - ap->ioaddr.lbah_addr = base + 1 + 5 * 4; - ap->ioaddr.device_addr = base + 1 + 6 * 4; - ap->ioaddr.status_addr = base + 1 + 7 * 4; - ap->ioaddr.command_addr = base + 1 + 7 * 4; - - base = (void __iomem *)ctl_mem_res->start; - ap->ioaddr.altstatus_addr = base + 1; - ap->ioaddr.ctl_addr = base + 1; - - ata_port_desc(ap, "cmd 0x%lx ctl 0x%lx", - (unsigned long)base_mem_res->start, - (unsigned long)ctl_mem_res->start); + ap->ioaddr.data_addr = (void __iomem *)base_mem_res->start; + + if (base_res) { /* only Q40 has IO resources */ + io_offset = 0x10000; + reg_scale = 1; + base = (void __iomem *)base_res->start; + ctl_base = (void __iomem *)ctl_res->start; + + ata_port_desc(ap, "cmd %pa ctl %pa", + &base_res->start, + &ctl_res->start); + } else { + base = (void __iomem *)base_mem_res->start; + ctl_base = (void __iomem *)ctl_mem_res->start; + + ata_port_desc(ap, "cmd %pa ctl %pa", + &base_mem_res->start, + &ctl_mem_res->start); + } + + ap->ioaddr.error_addr = base + io_offset + 1 * reg_scale; + ap->ioaddr.feature_addr = base + io_offset + 1 * reg_scale; + ap->ioaddr.nsect_addr = base + io_offset + 2 * reg_scale; + ap->ioaddr.lbal_addr = base + io_offset + 3 * reg_scale; + ap->ioaddr.lbam_addr = base + io_offset + 4 * reg_scale; + ap->ioaddr.lbah_addr = base + io_offset + 5 * reg_scale; + ap->ioaddr.device_addr = base + io_offset + 6 * reg_scale; + ap->ioaddr.status_addr = base + io_offset + 7 * reg_scale; + ap->ioaddr.command_addr = base + io_offset + 7 * reg_scale; + + ap->ioaddr.altstatus_addr = ctl_base + io_offset; + ap->ioaddr.ctl_addr = ctl_base + io_offset; irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); if (irq_res && irq_res->start > 0) {