cvs commit: src/sys/dev/firewire firewirereg.h fwohci.c sbp.c

[Nate Lawson]

Hidetoshi Shimokawa wrote:
> On 6/8/07, Nate Lawson <nate at root.org> wrote:
>> Hidetoshi Shimokawa wrote:
>> > simokawa    2007-06-07 13:20:49 UTC
>> >
>> >   FreeBSD src repository
>> >
>> >   Modified files:
>> >     sys/dev/firewire     firewirereg.h fwohci.c sbp.c
>> >   Log:
>> >   Add a tunable hw.firewire.phydma_enable.
>> >
>> >   This is enabled by default.  It should be disabled for
>> >   those who are uneasy with peeking/poking from FireWire.
>> >
>> >   Please note sbp(4) and dcons(4) over FireWire need
>> >   this feature.
>> >
>> >   Revision  Changes    Path
>> >   1.48      +1 -0      src/sys/dev/firewire/firewirereg.h
>> >   1.91      +11 -6     src/sys/dev/firewire/fwohci.c
>> >   1.94      +7 -0      src/sys/dev/firewire/sbp.c
>> >
>> >
>> > Index: src/sys/dev/firewire/firewirereg.h
>> > diff -u src/sys/dev/firewire/firewirereg.h:1.47
>> src/sys/dev/firewire/firewirereg.h:1.48
>> > --- src/sys/dev/firewire/firewirereg.h:1.47   Wed Jun  6 14:31:36 2007
>> > +++ src/sys/dev/firewire/firewirereg.h        Thu Jun  7 13:20:48 2007
>> > @@ -299,6 +299,7 @@
>> >
>> >  extern int firewire_debug;
>> >  extern devclass_t firewire_devclass;
>> > +extern int firewire_phydma_enable;
>> >
>> >  #ifdef __DragonFly__
>> >  #define              FWPRI           PCATCH
>> > Index: src/sys/dev/firewire/fwohci.c
>> > diff -u src/sys/dev/firewire/fwohci.c:1.90
>> src/sys/dev/firewire/fwohci.c:1.91
>> > --- src/sys/dev/firewire/fwohci.c:1.90        Wed Jun  6 14:31:36 2007
>> > +++ src/sys/dev/firewire/fwohci.c     Thu Jun  7 13:20:48 2007
>> > @@ -79,9 +79,13 @@
>> >  #undef OHCI_DEBUG
>> >
>> >  static int nocyclemaster = 0;
>> > +int firewire_phydma_enable = 1;
>> >  SYSCTL_DECL(_hw_firewire);
>> >  SYSCTL_INT(_hw_firewire, OID_AUTO, nocyclemaster, CTLFLAG_RW,
>> &nocyclemaster, 0,
>> >          "Do not send cycle start packets");
>> > +SYSCTL_INT(_hw_firewire, OID_AUTO, phydma_enable, CTLFLAG_RW,
>> > +     &firewire_phydma_enable, 1, "Allow physical request DMA from
>> firewire");
>> > +TUNABLE_INT("hw.firewire.phydma_enable", &firewire_phydma_enable);
>> >
>> >  static char dbcode[16][0x10]={"OUTM", "OUTL","INPM","INPL",
>> >               "STOR","LOAD","NOP ","STOP",};
>> > @@ -1862,12 +1866,13 @@
>> >
>> >               /* Allow async. request to us */
>> >               OWRITE(sc, OHCI_AREQHI, 1 << 31);
>> > -             /* XXX insecure ?? */
>> > -             /* allow from all nodes */
>> > -             OWRITE(sc, OHCI_PREQHI, 0x7fffffff);
>> > -             OWRITE(sc, OHCI_PREQLO, 0xffffffff);
>> > -             /* 0 to 4GB regison */
>> > -             OWRITE(sc, OHCI_PREQUPPER, 0x10000);
>> > +             if (firewire_phydma_enable) {
>> > +                     /* allow from all nodes */
>> > +                     OWRITE(sc, OHCI_PREQHI, 0x7fffffff);
>> > +                     OWRITE(sc, OHCI_PREQLO, 0xffffffff);
>> > +                     /* 0 to 4GB region */
>> > +                     OWRITE(sc, OHCI_PREQUPPER, 0x10000);
>> > +             }
>> >               /* Set ATRetries register */
>> >               OWRITE(sc, OHCI_ATRETRY, 1<<(13+16) | 0xfff);
>>
>> I see this runs each time a PHY_SID intr arrives.  Is that enough to
>> really prevent access if a pre-existing host on the bus requests DMA?  I
>> don't think an intr is generated in that case.
>>
>> Should this be a separate function run directly by the sysctl that sets
>> the filters immediately?
>
> Yes,  that's true.
>
> I recommend you to put it in loader.conf


Since someone who is setting this is concerned about security, should
this at least be documented?  I'd prefer that the routine be changed to
set the filter registers synchronously with the sysctl.

-- 
Nate