diff mbox series

[15/16] of: unittest: initialize args before calling of_irq_parse_one()

Message ID 1538712767-30394-16-git-send-email-frowand.list@gmail.com (mailing list archive)
State Superseded, archived
Headers show
Series of: overlay: validation checks, subsequent fixes | expand

Commit Message

Frank Rowand Oct. 5, 2018, 4:12 a.m. UTC 
From: Frank Rowand <frank.rowand@sony.com>

Callers of of_irq_parse_one() blindly use the pointer args.np
without checking whether of_irq_parse_one() had an error and
thus did not set the value of args.np.  Initialize args to
zero so that using the format "%pOF" to show the value of
args.np will show "(null)" when of_irq_parse_one() has an
error and does not set args.np instead of trying to
dereference a random value.

Reported-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---
 drivers/of/unittest.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Guenter Roeck Oct. 5, 2018, 1:26 p.m. UTC | #1 
On 10/04/2018 09:12 PM, frowand.list@gmail.com wrote:
> From: Frank Rowand <frank.rowand@sony.com>
> 
> Callers of of_irq_parse_one() blindly use the pointer args.np
> without checking whether of_irq_parse_one() had an error and
> thus did not set the value of args.np.  Initialize args to
> zero so that using the format "%pOF" to show the value of
> args.np will show "(null)" when of_irq_parse_one() has an
> error and does not set args.np instead of trying to
> dereference a random value.
> 
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Frank Rowand <frank.rowand@sony.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

The same problem exists when of_parse_phandle_with_args() reports an error.

Guenter

> ---
>   drivers/of/unittest.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> index 6d80f474c8f2..b61a33f30a56 100644
> --- a/drivers/of/unittest.c
> +++ b/drivers/of/unittest.c
> @@ -780,7 +780,7 @@ static void __init of_unittest_parse_interrupts(void)
>   	for (i = 0; i < 4; i++) {
>   		bool passed = true;
>   
> -		args.args_count = 0;
> +		memset(&args, 0, sizeof(args));
>   		rc = of_irq_parse_one(np, i, &args);
>   
>   		passed &= !rc;
> @@ -801,7 +801,7 @@ static void __init of_unittest_parse_interrupts(void)
>   	for (i = 0; i < 4; i++) {
>   		bool passed = true;
>   
> -		args.args_count = 0;
> +		memset(&args, 0, sizeof(args));
>   		rc = of_irq_parse_one(np, i, &args);
>   
>   		/* Test the values from tests-phandle.dtsi */
> @@ -854,6 +854,7 @@ static void __init of_unittest_parse_interrupts_extended(void)
>   	for (i = 0; i < 7; i++) {
>   		bool passed = true;
>   
> +		memset(&args, 0, sizeof(args));
>   		rc = of_irq_parse_one(np, i, &args);
>   
>   		/* Test the values from tests-phandle.dtsi */
>
Rob Herring Oct. 5, 2018, 2:53 p.m. UTC | #2 
On Thu, Oct 4, 2018 at 11:14 PM <frowand.list@gmail.com> wrote:
>
> From: Frank Rowand <frank.rowand@sony.com>
>
> Callers of of_irq_parse_one() blindly use the pointer args.np
> without checking whether of_irq_parse_one() had an error and
> thus did not set the value of args.np.  Initialize args to
> zero so that using the format "%pOF" to show the value of
> args.np will show "(null)" when of_irq_parse_one() has an
> error and does not set args.np instead of trying to
> dereference a random value.
>
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
> ---
>  drivers/of/unittest.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Does this need to be part of this series?

Rob
Frank Rowand Oct. 5, 2018, 7:04 p.m. UTC | #3 
On 10/05/18 07:53, Rob Herring wrote:
> On Thu, Oct 4, 2018 at 11:14 PM <frowand.list@gmail.com> wrote:
>>
>> From: Frank Rowand <frank.rowand@sony.com>
>>
>> Callers of of_irq_parse_one() blindly use the pointer args.np
>> without checking whether of_irq_parse_one() had an error and
>> thus did not set the value of args.np.  Initialize args to
>> zero so that using the format "%pOF" to show the value of
>> args.np will show "(null)" when of_irq_parse_one() has an
>> error and does not set args.np instead of trying to
>> dereference a random value.
>>
>> Reported-by: Guenter Roeck <linux@roeck-us.net>
>> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
>> ---
>>  drivers/of/unittest.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> Does this need to be part of this series?

I do not know if it could be independently applied before the
rest of the series (I have not tested that).  I included it
in the series because the series has so many other changes to
unittest.c.

If you want me to break this out, I will remove it from this
series and resend it after the rest of the series has been
pulled to mainline (and rebase on the new mainline).

This patch is not fixing a known failure case - the current
test data does not trigger the problem.  The recent patch
from  Guenter that you already accepted fixes the known failure
case, so this patch is not urgent.  The same is true about the
other cases Guenter pointed out that this patch should fix.

-Frank
Frank Rowand Oct. 5, 2018, 7:05 p.m. UTC | #4 
On 10/05/18 06:26, Guenter Roeck wrote:
> On 10/04/2018 09:12 PM, frowand.list@gmail.com wrote:
>> From: Frank Rowand <frank.rowand@sony.com>
>>
>> Callers of of_irq_parse_one() blindly use the pointer args.np
>> without checking whether of_irq_parse_one() had an error and
>> thus did not set the value of args.np.  Initialize args to
>> zero so that using the format "%pOF" to show the value of
>> args.np will show "(null)" when of_irq_parse_one() has an
>> error and does not set args.np instead of trying to
>> dereference a random value.
>>
>> Reported-by: Guenter Roeck <linux@roeck-us.net>
>> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
> 
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> 
> The same problem exists when of_parse_phandle_with_args() reports an error.

Thanks, I'll add a fix for that.

-Frank

> 
> Guenter
> 
>> ---
>>   drivers/of/unittest.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
>> index 6d80f474c8f2..b61a33f30a56 100644
>> --- a/drivers/of/unittest.c
>> +++ b/drivers/of/unittest.c
>> @@ -780,7 +780,7 @@ static void __init of_unittest_parse_interrupts(void)
>>       for (i = 0; i < 4; i++) {
>>           bool passed = true;
>>   -        args.args_count = 0;
>> +        memset(&args, 0, sizeof(args));
>>           rc = of_irq_parse_one(np, i, &args);
>>             passed &= !rc;
>> @@ -801,7 +801,7 @@ static void __init of_unittest_parse_interrupts(void)
>>       for (i = 0; i < 4; i++) {
>>           bool passed = true;
>>   -        args.args_count = 0;
>> +        memset(&args, 0, sizeof(args));
>>           rc = of_irq_parse_one(np, i, &args);
>>             /* Test the values from tests-phandle.dtsi */
>> @@ -854,6 +854,7 @@ static void __init of_unittest_parse_interrupts_extended(void)
>>       for (i = 0; i < 7; i++) {
>>           bool passed = true;
>>   +        memset(&args, 0, sizeof(args));
>>           rc = of_irq_parse_one(np, i, &args);
>>             /* Test the values from tests-phandle.dtsi */
>>
> 
>
diff mbox series

Patch

diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index 6d80f474c8f2..b61a33f30a56 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -780,7 +780,7 @@  static void __init of_unittest_parse_interrupts(void)
 	for (i = 0; i < 4; i++) {
 		bool passed = true;
 
-		args.args_count = 0;
+		memset(&args, 0, sizeof(args));
 		rc = of_irq_parse_one(np, i, &args);
 
 		passed &= !rc;
@@ -801,7 +801,7 @@  static void __init of_unittest_parse_interrupts(void)
 	for (i = 0; i < 4; i++) {
 		bool passed = true;
 
-		args.args_count = 0;
+		memset(&args, 0, sizeof(args));
 		rc = of_irq_parse_one(np, i, &args);
 
 		/* Test the values from tests-phandle.dtsi */
@@ -854,6 +854,7 @@  static void __init of_unittest_parse_interrupts_extended(void)
 	for (i = 0; i < 7; i++) {
 		bool passed = true;
 
+		memset(&args, 0, sizeof(args));
 		rc = of_irq_parse_one(np, i, &args);
 
 		/* Test the values from tests-phandle.dtsi */