security review needed - Re: [PATCH] ohci: Turn remote DMA support into a module parameter

[Stefan Richter <stefanr@s5r6.in-berlin.de>]

On Dec 22 Lubomir Rintel wrote:
> This makes it possible to debug kernel over FireWire without the need to
> recompile it.
> 
> Cc: Stefan Richter <stefanr@s5r6.in-berlin.de>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>

Looks good to me.  A load-time option is preferable over a compile-time
option not only from the POV of the debugging use case, but also from the
maintenance POV.

It weakens security in two scenarios though, AFAICS:

A)
  - There are firewire-ohci and firewire-sbp2 installed on the machine,
  - the attacker cannot upload code
  - but can load kernel modules
  - and has physical access to a 1394 port
  - and is not able to run a minimal SBP-2 target on the remote 1394 end.

B)
  - There is firewire-ohci but not firewire-sbp2 installed on the machine,
  - the attacker cannot upload code
  - but can load kernel modules
  - and has physical access to a 1394 port.

(In both scenarios, the attacker additionally has to be able to /un/load
kernel modules if firewire-ohci was loaded already before the attack.)

That's both quite specific.  Hence the security impact of this patch is
negligible in my opinion.  Any other opinions or insights into it?

> ---
>  Documentation/debugging-via-ohci1394.txt |  4 +---
>  drivers/firewire/ohci.c                  | 19 +++++++++++--------
>  lib/Kconfig.debug                        | 11 -----------
>  3 files changed, 12 insertions(+), 22 deletions(-)
> 
> diff --git a/Documentation/debugging-via-ohci1394.txt b/Documentation/debugging-via-ohci1394.txt
> index 14d1944..73473aa 100644
> --- a/Documentation/debugging-via-ohci1394.txt
> +++ b/Documentation/debugging-via-ohci1394.txt
> @@ -38,9 +38,7 @@ Drivers
>  
>  The firewire-ohci driver in drivers/firewire uses filtered physical
>  DMA by default, which is more secure but not suitable for remote debugging.
> -Compile the driver with CONFIG_FIREWIRE_OHCI_REMOTE_DMA (Kernel hacking menu:
> -Remote debugging over FireWire with firewire-ohci) to get unfiltered physical
> -DMA.
> +Pass the remote_dma=1 parameter to the driver to get unfiltered physical DMA.
>  
>  Because the firewire-ohci driver depends on the PCI enumeration to be
>  completed, an initialization routine which runs pretty early has been
> diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c
> index 6aa8a86..6313735 100644
> --- a/drivers/firewire/ohci.c
> +++ b/drivers/firewire/ohci.c
> @@ -370,6 +370,10 @@ MODULE_PARM_DESC(debug, "Verbose logging (default = 0"
>  	", busReset events = "	__stringify(OHCI_PARAM_DEBUG_BUSRESETS)
>  	", or a combination, or all = -1)");
>  
> +static bool param_remote_dma;
> +module_param_named(remote_dma, param_remote_dma, bool, 0444);

Permissions could also be 0644; then it would not be required to unload
firewire-ohci before altering the mode.  A hot change of this parameter
would only become effective after a 1394 bus reset though.  0444 keep
things simpler to understand though.

> +MODULE_PARM_DESC(remote_dma, "Enable unfiltered remote DMA (default = 0)");

Trivial comment:  I suggest the wording 'default = N' here.
0/1, n/y, or N/Y can be used to set the parameter value, but N/Y will be
shown as value in sysfs.

> +
>  static void log_irqs(struct fw_ohci *ohci, u32 evt)
>  {
>  	if (likely(!(param_debug &
> @@ -2050,10 +2054,10 @@ static void bus_reset_work(struct work_struct *work)
>  			  be32_to_cpu(ohci->next_header));
>  	}
>  
> -#ifdef CONFIG_FIREWIRE_OHCI_REMOTE_DMA
> -	reg_write(ohci, OHCI1394_PhyReqFilterHiSet, ~0);
> -	reg_write(ohci, OHCI1394_PhyReqFilterLoSet, ~0);
> -#endif
> +	if (param_remote_dma) {
> +		reg_write(ohci, OHCI1394_PhyReqFilterHiSet, ~0);
> +		reg_write(ohci, OHCI1394_PhyReqFilterLoSet, ~0);
> +	}
>  
>  	spin_unlock_irq(&ohci->lock);
>  
> @@ -2587,13 +2591,13 @@ static int ohci_cancel_packet(struct fw_card *card, struct fw_packet *packet)
>  static int ohci_enable_phys_dma(struct fw_card *card,
>  				int node_id, int generation)
>  {
> -#ifdef CONFIG_FIREWIRE_OHCI_REMOTE_DMA
> -	return 0;
> -#else
>  	struct fw_ohci *ohci = fw_ohci(card);
>  	unsigned long flags;
>  	int n, ret = 0;
>  
> +	if (param_remote_dma)
> +		return 0;
> +
>  	/*
>  	 * FIXME:  Make sure this bitmask is cleared when we clear the busReset
>  	 * interrupt bit.  Clear physReqResourceAllBuses on bus reset.
> @@ -2622,7 +2626,6 @@ static int ohci_enable_phys_dma(struct fw_card *card,
>  	spin_unlock_irqrestore(&ohci->lock, flags);
>  
>  	return ret;
> -#endif /* CONFIG_FIREWIRE_OHCI_REMOTE_DMA */
>  }
>  
>  static u32 ohci_read_csr(struct fw_card *card, int csr_offset)
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index db25707..dc30284 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -1547,17 +1547,6 @@ config PROVIDE_OHCI1394_DMA_INIT
>  
>  	  See Documentation/debugging-via-ohci1394.txt for more information.
>  
> -config FIREWIRE_OHCI_REMOTE_DMA
> -	bool "Remote debugging over FireWire with firewire-ohci"
> -	depends on FIREWIRE_OHCI
> -	help
> -	  This option lets you use the FireWire bus for remote debugging
> -	  with help of the firewire-ohci driver. It enables unfiltered
> -	  remote DMA in firewire-ohci.
> -	  See Documentation/debugging-via-ohci1394.txt for more information.
> -
> -	  If unsure, say N.
> -
>  config BUILD_DOCSRC
>  	bool "Build targets in Documentation/ tree"
>  	depends on HEADERS_CHECK



-- 
Stefan Richter
-=====-===-= ==-- =-===
http://arcgraph.de/sr/