[PATCH] i915: stack address leak when failing to read registers.

[PATCH] i915: stack address leak when failing to read registers.

[Quentin Casasnovas]

It is possible for the *_read*() functions to fail, in which case it'll
leave its third argument untouched.  Most of the code do not check the
return value of *_read*() functions, and will happily use garbage from the
stack to test various things.  For example, ch7xxx_dump_regs() will leak 1
byte from the stack in case of failure, ivch_dump_regs() 2 bytes, etc.

Instead of fixing every caller to check the return value, just clear the
output variable so this patch can easily go to -stable.  A proper fix would
probably to check the return value everywhere though, since it does not
make much sense to carry on talking to the ship after some error.

This issue was found by code review while preparing Ksplice updates.

Signed-off-by: Quentin Casasnovas <quentin.casasnovas@oracle.com>
---
 drivers/gpu/drm/i915/dvo_ch7017.c | 1 +
 drivers/gpu/drm/i915/dvo_ch7xxx.c | 1 +
 drivers/gpu/drm/i915/dvo_ivch.c   | 1 +
 drivers/gpu/drm/i915/dvo_ns2501.c | 2 +-
 drivers/gpu/drm/i915/dvo_sil164.c | 1 +
 drivers/gpu/drm/i915/dvo_tfp410.c | 1 +
 6 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/dvo_ch7017.c b/drivers/gpu/drm/i915/dvo_ch7017.c
index 86b27d1..826ac6c 100644
--- a/drivers/gpu/drm/i915/dvo_ch7017.c
+++ b/drivers/gpu/drm/i915/dvo_ch7017.c
@@ -181,6 +181,7 @@ static bool ch7017_read(struct intel_dvo_device *dvo, u8 addr, u8 *val)
 			.buf = val,
 		}
 	};
+	*val = 0;
 	return i2c_transfer(dvo->i2c_bus, msgs, 2) == 2;
 }
 
diff --git a/drivers/gpu/drm/i915/dvo_ch7xxx.c b/drivers/gpu/drm/i915/dvo_ch7xxx.c
index 80449f4..4ebbeef 100644
--- a/drivers/gpu/drm/i915/dvo_ch7xxx.c
+++ b/drivers/gpu/drm/i915/dvo_ch7xxx.c
@@ -166,6 +166,7 @@ static bool ch7xxx_readb(struct intel_dvo_device *dvo, int addr, uint8_t *ch)
 		DRM_DEBUG_KMS("Unable to read register 0x%02x from %s:%02x.\n",
 			  addr, adapter->name, dvo->slave_addr);
 	}
+	*ch = 0;
 	return false;
 }
 
diff --git a/drivers/gpu/drm/i915/dvo_ivch.c b/drivers/gpu/drm/i915/dvo_ivch.c
index 0f2587f..43604f0a 100644
--- a/drivers/gpu/drm/i915/dvo_ivch.c
+++ b/drivers/gpu/drm/i915/dvo_ivch.c
@@ -202,6 +202,7 @@ static bool ivch_read(struct intel_dvo_device *dvo, int addr, uint16_t *data)
 				"%s:%02x.\n",
 			  addr, adapter->name, dvo->slave_addr);
 	}
+	*data = 0;
 	return false;
 }
 
diff --git a/drivers/gpu/drm/i915/dvo_ns2501.c b/drivers/gpu/drm/i915/dvo_ns2501.c
index 44163043..b314f64 100644
--- a/drivers/gpu/drm/i915/dvo_ns2501.c
+++ b/drivers/gpu/drm/i915/dvo_ns2501.c
@@ -409,7 +409,7 @@ static bool ns2501_readb(struct intel_dvo_device *dvo, int addr, uint8_t * ch)
 		    ("Unable to read register 0x%02x from %s:0x%02x.\n", addr,
 		     adapter->name, dvo->slave_addr);
 	}
-
+	*ch = 0;
 	return false;
 }
 
diff --git a/drivers/gpu/drm/i915/dvo_sil164.c b/drivers/gpu/drm/i915/dvo_sil164.c
index fa01149..7d5a440 100644
--- a/drivers/gpu/drm/i915/dvo_sil164.c
+++ b/drivers/gpu/drm/i915/dvo_sil164.c
@@ -99,6 +99,7 @@ static bool sil164_readb(struct intel_dvo_device *dvo, int addr, uint8_t *ch)
 		DRM_DEBUG_KMS("Unable to read register 0x%02x from %s:%02x.\n",
 			  addr, adapter->name, dvo->slave_addr);
 	}
+	*ch = 0;
 	return false;
 }
 
diff --git a/drivers/gpu/drm/i915/dvo_tfp410.c b/drivers/gpu/drm/i915/dvo_tfp410.c
index 7853719..2b66881 100644
--- a/drivers/gpu/drm/i915/dvo_tfp410.c
+++ b/drivers/gpu/drm/i915/dvo_tfp410.c
@@ -124,6 +124,7 @@ static bool tfp410_readb(struct intel_dvo_device *dvo, int addr, uint8_t *ch)
 		DRM_DEBUG_KMS("Unable to read register 0x%02x from %s:%02x.\n",
 			  addr, adapter->name, dvo->slave_addr);
 	}
+	*ch = 0;
 	return false;
 }
 
-- 
2.0.5

Re: [PATCH] i915: stack address leak when failing to read registers.

[Greg KH]

On Mon, Feb 02, 2015 at 02:58:36PM +0100, Quentin Casasnovas wrote:
> It is possible for the *_read*() functions to fail, in which case it'll
> leave its third argument untouched.  Most of the code do not check the
> return value of *_read*() functions, and will happily use garbage from the
> stack to test various things.  For example, ch7xxx_dump_regs() will leak 1
> byte from the stack in case of failure, ivch_dump_regs() 2 bytes, etc.
> 
> Instead of fixing every caller to check the return value, just clear the
> output variable so this patch can easily go to -stable.  A proper fix would
> probably to check the return value everywhere though, since it does not
> make much sense to carry on talking to the ship after some error.
> 
> This issue was found by code review while preparing Ksplice updates.
> 
> Signed-off-by: Quentin Casasnovas <quentin.casasnovas@oracle.com>
> ---
>  drivers/gpu/drm/i915/dvo_ch7017.c | 1 +
>  drivers/gpu/drm/i915/dvo_ch7xxx.c | 1 +
>  drivers/gpu/drm/i915/dvo_ivch.c   | 1 +
>  drivers/gpu/drm/i915/dvo_ns2501.c | 2 +-
>  drivers/gpu/drm/i915/dvo_sil164.c | 1 +
>  drivers/gpu/drm/i915/dvo_tfp410.c | 1 +
>  6 files changed, 6 insertions(+), 1 deletion(-)

<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read Documentation/stable_kernel_rules.txt
for how to do this properly.

</formletter>