[PATCH] ohci: Turn remote DMA support into a module parameter

[PATCH] ohci: Turn remote DMA support into a module parameter

[Lubomir Rintel]

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>
---
 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);
+MODULE_PARM_DESC(remote_dma, "Enable unfiltered remote DMA (default = 0)");
+
 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
-- 
1.8.4.2

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

[Stefan Richter]

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/

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

[Stefan Richter]

On Dec 23 Stefan Richter wrote:
> 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?

Since there were no objections, I committed it to linux1394.git master
and for-next now.
-- 
Stefan Richter
-=====-====- ---= -==--
http://arcgraph.de/sr/