[PATCH] ipw2200: prevent alloc of unspecified size on stack

[PATCH] ipw2200: prevent alloc of unspecified size on stack

[Reinette Chatre]

if log_len is larger than 4K then we are killing the stack.
allocate on heap instead and limit size to what practically can
be used (PAGE_SIZE)

Is it possible for this to get into 2.6.24?

Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
---
 drivers/net/wireless/ipw2200.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/ipw2200.c b/drivers/net/wireless/ipw2200.c
index 54f44e5..e19e83a 100644
--- a/drivers/net/wireless/ipw2200.c
+++ b/drivers/net/wireless/ipw2200.c
@@ -1233,9 +1233,19 @@ static ssize_t show_event_log(struct device *d,
 {
 	struct ipw_priv *priv = dev_get_drvdata(d);
 	u32 log_len = ipw_get_event_log_len(priv);
-	struct ipw_event log[log_len];
+	u32 log_size;
+	struct ipw_event *log;
 	u32 len = 0, i;
 
+	/* not using min() because of its strict type checking */
+	log_size = sizeof(*log) * log_len < PAGE_SIZE ?
+			sizeof(*log) * log_len : PAGE_SIZE;
+	log = kzalloc(log_size, GFP_KERNEL);
+	if (!log) {
+		IPW_ERROR("Unable to allocate memory for log\n");
+		return 0;
+	}
+	log_len = log_size / sizeof(*log);
 	ipw_capture_event_log(priv, log_len, log);
 
 	len += snprintf(buf + len, PAGE_SIZE - len, "%08X", log_len);
@@ -1244,6 +1254,7 @@ static ssize_t show_event_log(struct device *d,
 				"\n%08X%08X%08X",
 				log[i].time, log[i].event, log[i].data);
 	len += snprintf(buf + len, PAGE_SIZE - len, "\n");
+	kfree(log);
 	return len;
 }
 
-- 
1.5.3.4

Re: [PATCH] ipw2200: prevent alloc of unspecified size on stack

[Zhu Yi]

On Tue, 2007-12-18 at 22:01 -0800, Reinette Chatre wrote:
> if log_len is larger than 4K then we are killing the stack.
> allocate on heap instead and limit size to what practically can
> be used (PAGE_SIZE)
> 
> Is it possible for this to get into 2.6.24?
> 
> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>

ACK.

> ---
> +       /* not using min() because of its strict type checking */
> +       log_size = sizeof(*log) * log_len < PAGE_SIZE ?
> +                       sizeof(*log) * log_len : PAGE_SIZE;

A (u32) cast should work. But I don't think it's a big issue.

Thanks,
-yi

Re: [PATCH] ipw2200: prevent alloc of unspecified size on stack

[Al Viro]

On Tue, Dec 18, 2007 at 10:01:02PM -0800, Reinette Chatre wrote:
> +	/* not using min() because of its strict type checking */
> +	log_size = sizeof(*log) * log_len < PAGE_SIZE ?

		   PAGE_SIZE / sizeof(*log) > len ?

to be provably safe against wraparounds, if you really want to limit that
to PAGE_SIZE...

RE: [PATCH] ipw2200: prevent alloc of unspecified size on stack

[Chatre, Reinette]

On Tuesday, December 18, 2007 10:35 PM, Al Viro  wrote:

> On Tue, Dec 18, 2007 at 10:01:02PM -0800, Reinette Chatre wrote:
>> +	/* not using min() because of its strict type checking */
>> +	log_size = sizeof(*log) * log_len < PAGE_SIZE ?
> 
> 		   PAGE_SIZE / sizeof(*log) > len ?
> 
> to be provably safe against wraparounds, if you really want to limit
> that to PAGE_SIZE...

To cover this I reset log_len after allocating the memory:

+	}
+	log_len = log_size / sizeof(*log);
 	ipw_capture_event_log(priv, log_len, log);


If we use the original length then we are ok and log_len is just what it
was before. If we use PAGE_SIZE then log_len is reset to fit in the
amount of memory we allocated (PAGE_SIZE).

Is this what you meant?

Reinette

Re: [PATCH] ipw2200: prevent alloc of unspecified size on stack

[Zhu Yi]

On Wed, 2007-12-19 at 06:35 +0000, Al Viro wrote:
> to be provably safe against wraparounds, if you really want to limit
> that to PAGE_SIZE...

This is a sysfs read. We can only print PAGE_SIZE at most anyway.

Thanks,
-yi