1. 13 Jul, 2010 1 commit
    • Clemens Ladisch's avatar
      firewire: cdev: check write quadlet request length to avoid buffer overflow · a8e93f3d
      Clemens Ladisch authored
      Check that the data length of a write quadlet request actually is large
      enough for a quadlet.  Otherwise, fw_fill_request could access the four
      bytes after the end of the outbound_transaction_event structure.
      Signed-off-by: default avatarClemens Ladisch <clemens@ladisch.de>
      
      Modification of Clemens' change:  Consolidate the check into
      init_request() which is used by the affected ioctl_send_request() and
      ioctl_send_broadcast_request() and the unaffected
      ioctl_send_stream_packet(), to save a few lines of code.
      
      Note, since struct outbound_transaction_event *e is slab-allocated, such
      an out-of-bounds access won't hit unallocated memory but may result in a
      (virtually impossible to exploit) information disclosure.
      Signed-off-by: default avatarStefan Richter <stefanr@s5r6.in-berlin.de>
      a8e93f3d
  2. 08 Jul, 2010 2 commits
  3. 20 Jun, 2010 9 commits
    • Stefan Richter's avatar
      firewire: cdev: extend fw_cdev_event_iso_interrupt documentation · 3b2b65d6
      Stefan Richter authored
      Add information regarding the 2.6.32 update to the xmit variant of
      fw_cdev_event_iso_interrupt.
      Signed-off-by: default avatarStefan Richter <stefanr@s5r6.in-berlin.de>
      3b2b65d6
    • Stefan Richter's avatar
      firewire: cdev: fix ABI for FCP and address range mapping, add fw_cdev_event_request2 · e205597d
      Stefan Richter authored
      The problem:
      
      A target-like userspace driver, e.g. AV/C target or SBP-2/3 target,
      needs to be able to act as responder and requester.  In the latter role,
      it needs to send requests to nods from which it received requests.  This
      is currently impossible because fw_cdev_event_request lacks information
      about sender node ID.
      Reported-by: default avatarJay Fenlason <fenlason@redhat.com>
      
      Libffado + libraw1394 + firewire-core is currently unable to drive two
      or more audio devices on the same bus.
      Reported-by: default avatarArnold Krille <arnold@arnoldarts.de>
      
      This is because libffado requires destination node ID of FCP requests
      and sender node ID of FCP responses to match.  It even prohibits
      libffado from working with a bus on which libraw1394 opens a /dev/fw* as
      default ioctl device that does not correspond with the audio device.
      This is because libraw1394 does not receive the sender node ID from the
      kernel.
      
      Moreover, fw_cdev_event_request makes it impossible to tell unicast and
      broadcast write requests apart.
      
      The fix:
      
      Add a replacement of struct fw_cdev_event_request request, boringly
      called struct fw_cdev_event_request2.  The new event will be sent to a
      userspace client instead of the old one if the client claims
      compatibility with <linux/firewire-cdev.h> ABI version 4 or later.
      
      libraw1394 needs to be extended to make use of the new event, in order
      to properly support libffado and other FCP or address range mapping
      users who require correct sender node IDs.
      
      Further notes:
      
      While we are at it, change back the range of possible values of
      fw_cdev_event_request.tcode to 0x0...0xb like in ABI version <= 3.
      The preceding change "firewire: expose extended tcode of incoming lock
      requests to (userspace) drivers" expanded it to 0x0...0x17 which could
      catch sloppily coded clients by surprise.  The extended range of codes
      is only used in the new fw_cdev_event_request2.tcode.
      
      Jay and I also suggested an alternative approach to fix the ABI for
      incoming requests:  Add an FW_CDEV_IOC_GET_REQUEST_INFO ioctl which can
      be called after reception of an fw_cdev_event_request, before issuing of
      the closing FW_CDEV_IOC_SEND_RESPONSE ioctl.  The new ioctl would reveal
      the vital information about a request that fw_cdev_event_request lacks.
      Jay showed an implementation of this approach.
      
      The former event approach adds 27 LOC of rather trivial code to
      core-cdev.c, the ioctl approach 34 LOC, some of which is nontrivial.
      The ioctl approach would certainly also add more LOC to userspace
      programs which require the expanded information on inbound requests.
      This approach is probably only on the lighter-weight side in case of
      clients that want to be compatible with kernels that lack the new
      capability, like libraw1394.  However, the code to be added to such
      libraw1394-like clients in case of the event approach is a straight-
      forward additional switch () case in its event handler.
      Signed-off-by: default avatarStefan Richter <stefanr@s5r6.in-berlin.de>
      e205597d
    • Jay Fenlason's avatar
      firewire: expose extended tcode of incoming lock requests to (userspace) drivers · c82f91f2
      Jay Fenlason authored
      When a remote device does a LOCK_REQUEST, the core does not pass
      the extended tcode to userspace.  This patch makes it use the
      juju-specific tcodes listed in firewire-constants.h for incoming
      requests.
      Signed-off-by: default avatarJay Fenlason <fenlason@redhat.com>
      
      This matches how tcode in the API for outbound requests is treated.
      Affects kernelspace and userspace drivers alike, but at the moment there
      are no kernespace drivers that receive lock requests.
      
      Split out from a combo patch, slightly reordered, changelog reworded.
      Signed-off-by: default avatarStefan Richter <stefanr@s5r6.in-berlin.de>
      c82f91f2
    • Stefan Richter's avatar
      firewire: cdev: freeze FW_CDEV_VERSION due to libraw1394 bug · 604f4516
      Stefan Richter authored
      libraw1394 v2.0.0...v2.0.5 takes FW_CDEV_VERSION from an externally
      installed header file and uses it to declare its own implementation
      level in FW_CDEV_IOC_GET_INFO.  This is wrong; it should set the real
      version for which it was actually written.
      
      If we add features to the kernel ABI that require the kernel to check
      a client's implementation level, we can not trust the client version if
      it was set from FW_CDEV_VERSION.
      
      Hence freeze FW_CDEV_VERSION at the current value (no damage has been
      done yet), clearly document FW_CDEV_VERSION as a dummy version and what
      clients are expected to do with fw_cdev_get_info.version, and use a new
      defined constant (which is not placed into the exported header file) as
      kernel implementation level.
      
      Note, in order to check in client program source code which features are
      present in an externally installed linux/firewire-cdev.h, use
      preprocessor directives like
        #ifdef FW_CDEV_IOC_ALLOCATE_ISO_RESOURCE
      or
        #ifdef FW_CDEV_EVENT_ISO_RESOURCE_ALLOCATED
      instead of a check of FW_CDEV_VERSION.
      Signed-off-by: default avatarStefan Richter <stefanr@s5r6.in-berlin.de>
      604f4516
    • Stefan Richter's avatar
      firewire: cdev: count references of cards during inbound transactions · 0244f573
      Stefan Richter authored
      If a request comes in to an address range managed by a userspace driver
      i.e. <linux/firewire-cdev.h> client, the card instance of request and
      response may differ from the card instance of the client device.
      Therefore we need to take a reference of the card until the response was
      sent.
      
      I thought about putting the reference counting into core-transaction.c,
      but the various high-level drivers besides cdev clients (firewire-net,
      firewire-sbp2, firedtv) use the card pointer in their fw_address_handler
      address_callback method only to look up devices of which they already
      hold the necessary references.  So this seems to be a specific
      firewire-cdev issue which is better addressed locally.
      
      We do not need the reference
        - in case of FCP_REQUEST or FCP_RESPONSE requests because then the
          firewire-core will send the split transaction response for us
          already in the context of the request handler,
        - if it is the same card as the client device's because we hold a
          card reference indirectly via teh client->device reference.
      To keep things simple, we take the reference nevertheless.
      
      Jay Fenlason wrote:
      > there's no way for the core to tell cdev "this card is gone,
      > kill any inbound transactions on it", while cdev holds the transaction
      > open until userspace issues a SEND_RESPONSE ioctl, which may be a very,
      > very long time.  But when it does, it calls fw_send_response(), which
      > will dereference the card...
      >
      > So how unhappy are we about userspace potentially holding a fw_card
      > open forever?
      
      While termination of inbound transcations at card removal could be
      implemented, it is IMO not worth the effort.  Currently, the effect of
      holding a reference of a card that has been removed is to block the
      process that called the pci_remove of the card.  This is
        - either a user process ran by root.  Root can find and kill processes
          that have /dev/fw* open, if desired.
        - a kernel thread (which one?) in case of hot removal of a PCCard or
          ExpressCard.
      The latter case could be a problem indeed.  firewire-core's card
      shutdown and card release should probably be improved not to block in
      shutdown, just to defer freeing of memory until release.
      
      This is not a new problem though; the same already always happens with
      the client->device->card without the need of inbound transactions or
      other special conditions involved, other than the client not closing the
      file.
      Signed-off-by: default avatarStefan Richter <stefanr@s5r6.in-berlin.de>
      0244f573
    • Jay Fenlason's avatar
      firewire: cdev: fix responses to nodes at different card · 08bd34c9
      Jay Fenlason authored
      My box has two firewire cards in it: card0 and card1.
      My application opens /dev/fw0 (card 0) and allocates an address space.
      The core makes the address space available on both cards.
      Along comes the remote device, which sends a READ_QUADLET_REQUEST to
      card1.  The request gets passed up to my application, which calls
      ioctl_send_response().
      
      ioctl_send_response() then calls fw_send_response() with card0,
      because that's the card it's bound to.
      Card0's driver drops the response, because it isn't part of
      a transaction that it has outstanding.
      
      So in core-cdev: handle_request(), we need to stash the
      card of the inbound request in the struct inbound_transaction_resource and
      use that card to send the response to.
      
      The hard part will be refcounting the card correctly
      so it can't get deallocated while we hold a pointer to it.
      
      Here's a trivial patch, which does not do the card refcounting, but at
      least demonstrates what the problem is.
      
      Note that we can't depend on the fact that the core-cdev:client
      structure holds a card open, because in this case the card it holds
      open is not the card the request came in on.
      
      ..and there's no way for the core to tell cdev "this card is gone,
      kill any inbound transactions on it", while cdev holds the transaction
      open until userspace issues a SEND_RESPONSE ioctl, which may be a very,
      very long time.  But when it does, it calls fw_send_response(), which
      will dereference the card...
      
      So how unhappy are we about userspace potentially holding a fw_card
      open forever?
      Signed-off-by: default avatarJay Fenlason <fenlason@redhat.com>
      
      Reference counting to be addressed in a separate change.
      
      Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de> (whitespace)
      08bd34c9
    • Clemens Ladisch's avatar
      firewire: cdev: fix race in iso context creation · bdfe273e
      Clemens Ladisch authored
      Protect the client's iso context pointer against a race that can happen
      when more than one creation call is executed at the same time.
      Signed-off-by: default avatarClemens Ladisch <clemens@ladisch.de>
      Signed-off-by: default avatarStefan Richter <stefanr@s5r6.in-berlin.de>
      bdfe273e
    • Stefan Richter's avatar
      firewire: remove an unused function argument · 33e553fe
      Stefan Richter authored
      void (*fw_address_callback_t)(..., int speed, ...) is the speed that a
      remote node chose to transmit a request to us.  In case of split
      transactions, firewire-core will transmit the response at that speed.
      
      Upper layer drivers on the other hand (firewire-net, -sbp2, firedtv, and
      userspace drivers) cannot do anything useful with that speed datum,
      except log it for debug purposes.  But data that is merely potentially
      (not even actually) used for debug purposes does not belong into the API.
      Signed-off-by: default avatarStefan Richter <stefanr@s5r6.in-berlin.de>
      33e553fe
    • Stefan Richter's avatar
      firewire: core: remove an unnecessary zero initialization · 56d04cb1
      Stefan Richter authored
      All of the fields of the iso_interrupt_event instance are overwritten
      right after it was allocated.
      Signed-off-by: default avatarStefan Richter <stefanr@s5r6.in-berlin.de>
      56d04cb1
  4. 19 Jun, 2010 8 commits
    • Stefan Richter's avatar
      firewire: core: remove unused variable · ae86e81e
      Stefan Richter authored
      which caused gcc 4.6 to warn about
          variable 'destination' set but not used.
      
      Since the hardware ensures that we receive only response packets with
      proper destination node ID (in a given bus generation), we have no use
      for destination here in the core as well as in upper layers.
      
      (This is different with request packets.  There we pass destination node
      ID to upper layers because they may for example need to check whether
      this was an unicast or broadcast request.)
      Reported-and-Tested-By: default avatarJustin P. Mattock <justinmattock@gmail.com>
      Signed-off-by: default avatarStefan Richter <stefanr@s5r6.in-berlin.de>
      ae86e81e
    • Stefan Richter's avatar
      ieee1394: remove unused variables · 5030c807
      Stefan Richter authored
      which caused gcc 4.6 to warn about
          variable 'XYZ' set but not used.
      
      sbp2.c, unit_characteristics:
      
      The underlying problem which was spotted here --- an incomplete
      implementation --- is already 50% fixed in drivers/firewire/sbp2.c which
      observes mgt_ORB_timeout but not yet ORB_size.
      
      raw1394.c, length_conflict; dv1394.c, ts_off:
      
      Impossible to tell why these variables are there.  We can safely remove
      them though because we don't need a compiler warning to realize that we
      are dealing with (at least stylistically) flawed code here.
      
      dv1394.c, packet_time:
      
      This was used in debug macro that is only compiled in with
      DV1394_DEBUG_LEVEL >= 2 defined at compile-time.  Just drop it since
      nobody debugs dv1394 anymore.  Avoids noise in regular kernel builds.
      
      dv1394.c, ohci; eth1394.c, priv:
      
      These variables clearly can go away.  Somebody wanted to use them but
      then didn't (or not anymore).
      
      Note, all of this code is considered to be at its end of life and is
      thus not really meant to receive janitorial updates anymore.  But if we
      can easily remove noisy warnings from kernel builds, we should.
      Reported-by: default avatarJustin P. Mattock <justinmattock@gmail.com>
      Signed-off-by: default avatarStefan Richter <stefanr@s5r6.in-berlin.de>
      5030c807
    • Stefan Richter's avatar
      firewire: rename CSR access driver methods · 0fcff4e3
      Stefan Richter authored
      Rather than "read a Control and Status Registers (CSR) Architecture
      register" I prefer to say "read a Control and Status Register".
      Signed-off-by: default avatarStefan Richter <stefanr@s5r6.in-berlin.de>
      0fcff4e3
    • Stefan Richter's avatar
      firewire: core: combine some repeated code · b384cf18
      Stefan Richter authored
      All of these CSRs have the same read/ write/ aynthing-else handling,
      except for CSR_PRIORITY_BUDGET which might not be implemented.
      
      The CSR_CYCLE_TIME read handler implementation accepted 4-byte-sized
      block write requests before this change but this is just silly; the
      register is only required to support quadlet read and write requests
      like the other r/w CSR core and Serial-Bus-dependent registers.
      Signed-off-by: default avatarStefan Richter <stefanr@s5r6.in-berlin.de>
      b384cf18
    • Stefan Richter's avatar
      firewire: normalize STATE_CLEAR/SET CSR access interface · c8a94ded
      Stefan Richter authored
      Push the maintenance of STATE_CLEAR/SET.abdicate down into the card
      driver.  This way, the read/write_csr_reg driver method works uniformly
      across all CSR offsets.
      Signed-off-by: default avatarStefan Richter <stefanr@s5r6.in-berlin.de>
      c8a94ded
    • Stefan Richter's avatar
      firewire: replace get_features card driver hook · db3c9cc1
      Stefan Richter authored
      by feature variables in the fw_card struct.  The hook appeared to be an
      unnecessary abstraction in the card driver interface.
      
      Cleaner would be to pass those feature flags as arguments to
      fw_card_initialize() or fw_card_add(), but the FairnessControl register
      is in the SCLK domain and may therefore not be accessible while Link
      Power Status is off, i.e. before the card->driver->enable call from
      fw_card_add().
      Signed-off-by: default avatarStefan Richter <stefanr@s5r6.in-berlin.de>
      db3c9cc1
    • Stefan Richter's avatar
      firewire: drop sizeof expressions from some request size arguments · e847cc83
      Stefan Richter authored
      In case of fw_card_bm_work()'s lock request, the present sizeof
      expression is going to be wrong if somebody changes the fw_card's DMA
      scratch buffer's size in the future.
      
      In case of quadlet write requests, sizeof(u32) is just silly; it's 4.
      
      In case of SBP-2 ORB pointer write requests, 8 is arguably quicker to
      understand as the correct and only possible value than
      sizeof(some_datum).
      Signed-off-by: default avatarStefan Richter <stefanr@s5r6.in-berlin.de>
      e847cc83
    • Stefan Richter's avatar
      firewire: 'add CSR_... support' addendum · 65b2742a
      Stefan Richter authored
      Add a comment on which of the conflicting NODE_IDS specifications we
      implement.  Reduce a comment on rather irrelevant register bits that can
      all be looked up in the spec (or from now on in the code history).
      Directly include the required indirectly included bug.h.
      Signed-off-by: default avatarStefan Richter <stefanr@s5r6.in-berlin.de>
      65b2742a
  5. 10 Jun, 2010 16 commits
  6. 09 Jun, 2010 4 commits