sh: clkfwk: potential null deref in debug code

PATCH In review Dan Carpenter 3 participants 4 messages Mar 20, 2011 – Mar 23, 2011

Subsystems (2)

Kernel janitors SuperH

lore

Share & tools

lore

Dan Carpenter

author

2011-03-20 11:12 UTC #0 lore raw

PATCH 1/1 sh: clkfwk: potential null deref in debug code

This function is designed to accept a NULL for "best_freq" but the debug code dereferences it unconditionally.

Signed-off-by Dan Carpenter <[email protected]>

---
Btw. Smatch complains that "best" could NULL as well, but I don't know if actually that's possible so I left it as is.

drivers/sh/clk/core.c +1 -1

index 5f63c3b..dee971c 100644
@@ -616,7 +616,7 @@ long clk_round_parent(struct clk *clk, unsigned long target,
 
 		pr_debug("%u / %lu = %lu, / %lu = %lu, best %lu, parent %u\n",
 			 freq->frequency, div, freq_high, div + 1, freq_low,
-			 *best_freq, best->frequency);
+			 best_freq ? *best_freq : -1, best->frequency);
 
 		if (!error)
 			break;

Guennadi Liakhovetski

2011-03-20 15:57 UTC #1 lore raw

Hi Dan

Quoted 13 lines · Dan Carpenter

This function is designed to accept a NULL for "best_freq" but the debug code dereferences it unconditionally.
Signed-off-by Dan Carpenter <[email protected]>
---
Btw. Smatch complains that "best" could NULL as well, but I don't know if actually that's possible so I left it as is.
drivers/sh/clk/core.c +0 -0
index 5f63c3b..dee971c 100644

drivers/sh/clk/core.c long clk_round_parent(struct clk *clk, unsigned long target, · line 619

Quoted 5 lines

drivers/sh/clk/core.c +1 -1

@@ -616,7 +616,7 @@ long clk_round_parent(struct clk *clk, unsigned long target,
 
 		pr_debug("%u / %lu = %lu, / %lu = %lu, best %lu, parent %u\n",
 			 freq->frequency, div, freq_high, div + 1, freq_low,
-			 *best_freq, best->frequency);
+			 best_freq ? *best_freq : -1, best->frequency);

What about another occasion of dereferencing best_freq at the top of the clk_round_parent() function:

	if (!parent) {
		*parent_freq = 0;
		*best_freq = clk_round_rate(clk, target);
		return abs(target - *best_freq);
	}

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Paul Mundt

2011-03-23 13:32 UTC #2 lore raw

In reply to Dan Carpenter

Quoted 26 lines · Guennadi Liakhovetski

Hi Dan
Quoted 13 lines · Dan Carpenter
This function is designed to accept a NULL for "best_freq" but the debug code dereferences it unconditionally.
Signed-off-by Dan Carpenter <[email protected]>
---
Btw. Smatch complains that "best" could NULL as well, but I don't know if actually that's possible so I left it as is.
drivers/sh/clk/core.c +0 -0
index 5f63c3b..dee971c 100644
drivers/sh/clk/core.c long clk_round_parent(struct clk *clk, unsigned long target, · line 619
Quoted 5 lines
drivers/sh/clk/core.c +1 -1
@@ -616,7 +616,7 @@ long clk_round_parent(struct clk *clk, unsigned long target,
 
 		pr_debug("%u / %lu = %lu, / %lu = %lu, best %lu, parent %u\n",
 			 freq->frequency, div, freq_high, div + 1, freq_low,
-			 *best_freq, best->frequency);
+			 best_freq ? *best_freq : -1, best->frequency);
What about another occasion of dereferencing best_freq at the top of the clk_round_parent() function:

Quoted 8 lines

	if (!parent) {
		*parent_freq = 0;
		*best_freq = clk_round_rate(clk, target);
		return abs(target - *best_freq);
	}

Do we actually have users that are passing in NULL for best_freq? I don't recall writing this code, so I assume you did :-)

Guennadi Liakhovetski

2011-03-23 14:01 UTC #3 lore raw

In reply to Dan Carpenter

Quoted 33 lines · Paul Mundt

Quoted 32 lines · Guennadi Liakhovetski
Hi Dan
Quoted 13 lines · Dan Carpenter
This function is designed to accept a NULL for "best_freq" but the debug code dereferences it unconditionally.
Signed-off-by Dan Carpenter <[email protected]>
---
Btw. Smatch complains that "best" could NULL as well, but I don't know if actually that's possible so I left it as is.
drivers/sh/clk/core.c +0 -0
index 5f63c3b..dee971c 100644
drivers/sh/clk/core.c long clk_round_parent(struct clk *clk, unsigned long target, · line 619
Quoted 5 lines
drivers/sh/clk/core.c +1 -1
@@ -616,7 +616,7 @@ long clk_round_parent(struct clk *clk, unsigned long target,
 
 		pr_debug("%u / %lu = %lu, / %lu = %lu, best %lu, parent %u\n",
 			 freq->frequency, div, freq_high, div + 1, freq_low,
-			 *best_freq, best->frequency);
+			 best_freq ? *best_freq : -1, best->frequency);
What about another occasion of dereferencing best_freq at the top of the clk_round_parent() function:
	if (!parent) {
		*parent_freq = 0;
		*best_freq = clk_round_rate(clk, target);
		return abs(target - *best_freq);
	}

Quoted 4 lines

Quoted 2 lines
?
Do we actually have users that are passing in NULL for best_freq? I don't recall writing this code, so I assume you did :-)

That seems very likely;) Yes, that's from my patch, and so far we have only one user - ap4evb_clk_optimize() and it doesn't pass NULL to the fn. But perhaps it is better to be consistent and if the variable is checked for NULL on some occasions, it should be checked everywhere.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/