* [PATCH] isdn: hisax: l3dss1.c: Fix for possible null pointer dereference
@ 2014-05-19 21:24 Rickard Strandqvist
2014-05-19 21:26 ` Sergei Shtylyov
0 siblings, 1 reply; 8+ messages in thread
From: Rickard Strandqvist @ 2014-05-19 21:24 UTC (permalink / raw)
To: Karsten Keil, Rickard Strandqvist; +Cc: netdev, linux-kernel
There is otherwise a risk of a possible null pointer dereference.
Was largely found by using a static code analysis program called cppcheck.
Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
---
drivers/isdn/hisax/l3dss1.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/isdn/hisax/l3dss1.c b/drivers/isdn/hisax/l3dss1.c
index cda7006..9ea7377 100644
--- a/drivers/isdn/hisax/l3dss1.c
+++ b/drivers/isdn/hisax/l3dss1.c
@@ -2203,13 +2203,14 @@ static int l3dss1_cmd_global(struct PStack *st, isdn_ctrl *ic)
memcpy(p, ic->parm.dss1_io.data, ic->parm.dss1_io.datalen); /* copy data */
l = (p - temp) + ic->parm.dss1_io.datalen; /* total length */
- if (ic->parm.dss1_io.timeout > 0)
- if (!(pc = dss1_new_l3_process(st, -1)))
- { free_invoke_id(st, id);
+ if (ic->parm.dss1_io.timeout > 0) {
+ if (!(pc = dss1_new_l3_process(st, -1))) {
+ free_invoke_id(st, id);
return (-2);
}
- pc->prot.dss1.ll_id = ic->parm.dss1_io.ll_id; /* remember id */
- pc->prot.dss1.proc = ic->parm.dss1_io.proc; /* and procedure */
+ pc->prot.dss1.ll_id = ic->parm.dss1_io.ll_id; /* remember id */
+ pc->prot.dss1.proc = ic->parm.dss1_io.proc; /* and procedure */
+ }
if (!(skb = l3_alloc_skb(l)))
{ free_invoke_id(st, id);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] isdn: hisax: l3dss1.c: Fix for possible null pointer dereference
2014-05-19 21:24 [PATCH] isdn: hisax: l3dss1.c: Fix for possible null pointer dereference Rickard Strandqvist
@ 2014-05-19 21:26 ` Sergei Shtylyov
2014-05-19 21:56 ` Rickard Strandqvist
0 siblings, 1 reply; 8+ messages in thread
From: Sergei Shtylyov @ 2014-05-19 21:26 UTC (permalink / raw)
To: Rickard Strandqvist, Karsten Keil; +Cc: netdev, linux-kernel
On 05/20/2014 01:24 AM, Rickard Strandqvist wrote:
> There is otherwise a risk of a possible null pointer dereference.
> Was largely found by using a static code analysis program called cppcheck.
> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
> ---
> drivers/isdn/hisax/l3dss1.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
> diff --git a/drivers/isdn/hisax/l3dss1.c b/drivers/isdn/hisax/l3dss1.c
> index cda7006..9ea7377 100644
> --- a/drivers/isdn/hisax/l3dss1.c
> +++ b/drivers/isdn/hisax/l3dss1.c
> @@ -2203,13 +2203,14 @@ static int l3dss1_cmd_global(struct PStack *st, isdn_ctrl *ic)
> memcpy(p, ic->parm.dss1_io.data, ic->parm.dss1_io.datalen); /* copy data */
> l = (p - temp) + ic->parm.dss1_io.datalen; /* total length */
>
> - if (ic->parm.dss1_io.timeout > 0)
> - if (!(pc = dss1_new_l3_process(st, -1)))
> - { free_invoke_id(st, id);
> + if (ic->parm.dss1_io.timeout > 0) {
> + if (!(pc = dss1_new_l3_process(st, -1))) {
Assignments shouldn't be put into *if* statement. If you did run the patch
thru scripts/checkpatch.pl, it would have told you.
> + free_invoke_id(st, id);
> return (-2);
Parens not needed. Could as well fix it here...
> }
> - pc->prot.dss1.ll_id = ic->parm.dss1_io.ll_id; /* remember id */
> - pc->prot.dss1.proc = ic->parm.dss1_io.proc; /* and procedure */
> + pc->prot.dss1.ll_id = ic->parm.dss1_io.ll_id; /* remember id */
> + pc->prot.dss1.proc = ic->parm.dss1_io.proc; /* and procedure */
> + }
WBR, Sergei
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] isdn: hisax: l3dss1.c: Fix for possible null pointer dereference
2014-05-19 21:26 ` Sergei Shtylyov
@ 2014-05-19 21:56 ` Rickard Strandqvist
2014-05-20 12:23 ` Sergei Shtylyov
0 siblings, 1 reply; 8+ messages in thread
From: Rickard Strandqvist @ 2014-05-19 21:56 UTC (permalink / raw)
To: Sergei Shtylyov; +Cc: Karsten Keil, netdev, linux-kernel
Hi Sergei
I did not put the assignment in the if statement. Is it meant for me
to change all the code around the parts I fix?
There are many assignments in if in the l3dss1_cmd_global() funktion.
It's late and I'm tired, but I still can not see which of these pairs
of features that I can remove ...?
if (ic->parm.dss1_io.timeout > 0) {
if (!(pc = dss1_new_l3_process(st, -1))) {
free_invoke_id(st, id);
return (-2);
}
pc->prot.dss1.ll_id = ic->parm.dss1_io.ll_id; /* remember id */
pc->prot.dss1.proc = ic->parm.dss1_io.proc; /* and procedure */
}
Best regards
Rickard Strandqvist
2014-05-19 23:26 GMT+02:00 Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>:
> On 05/20/2014 01:24 AM, Rickard Strandqvist wrote:
>
>> There is otherwise a risk of a possible null pointer dereference.
>
>
>> Was largely found by using a static code analysis program called cppcheck.
>
>
>> Signed-off-by: Rickard Strandqvist
>> <rickard_strandqvist@spectrumdigital.se>
>> ---
>> drivers/isdn/hisax/l3dss1.c | 11 ++++++-----
>> 1 file changed, 6 insertions(+), 5 deletions(-)
>
>
>> diff --git a/drivers/isdn/hisax/l3dss1.c b/drivers/isdn/hisax/l3dss1.c
>> index cda7006..9ea7377 100644
>> --- a/drivers/isdn/hisax/l3dss1.c
>> +++ b/drivers/isdn/hisax/l3dss1.c
>> @@ -2203,13 +2203,14 @@ static int l3dss1_cmd_global(struct PStack *st,
>> isdn_ctrl *ic)
>> memcpy(p, ic->parm.dss1_io.data,
>> ic->parm.dss1_io.datalen); /* copy data */
>> l = (p - temp) + ic->parm.dss1_io.datalen; /*
>> total length */
>>
>> - if (ic->parm.dss1_io.timeout > 0)
>> - if (!(pc = dss1_new_l3_process(st, -1)))
>> - { free_invoke_id(st, id);
>> + if (ic->parm.dss1_io.timeout > 0) {
>> + if (!(pc = dss1_new_l3_process(st, -1))) {
>
>
> Assignments shouldn't be put into *if* statement. If you did run the
> patch thru scripts/checkpatch.pl, it would have told you.
>
>
>> + free_invoke_id(st, id);
>> return (-2);
>
>
> Parens not needed. Could as well fix it here...
>
>
>> }
>> - pc->prot.dss1.ll_id = ic->parm.dss1_io.ll_id; /*
>> remember id */
>> - pc->prot.dss1.proc = ic->parm.dss1_io.proc; /* and
>> procedure */
>> + pc->prot.dss1.ll_id =
>> ic->parm.dss1_io.ll_id; /* remember id */
>> + pc->prot.dss1.proc =
>> ic->parm.dss1_io.proc; /* and procedure */
>> + }
>
>
> WBR, Sergei
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] isdn: hisax: l3dss1.c: Fix for possible null pointer dereference
2014-05-19 21:56 ` Rickard Strandqvist
@ 2014-05-20 12:23 ` Sergei Shtylyov
0 siblings, 0 replies; 8+ messages in thread
From: Sergei Shtylyov @ 2014-05-20 12:23 UTC (permalink / raw)
To: Rickard Strandqvist; +Cc: Karsten Keil, netdev, linux-kernel
Hello.
On 20-05-2014 1:56, Rickard Strandqvist wrote:
> Hi Sergei
> I did not put the assignment in the if statement.
You didn't but you changed the line it's on (which wasn't required BTW).
> Is it meant for me
> to change all the code around the parts I fix?
No, the patch just needs to be checkpatch.pl-clean.
> There are many assignments in if in the l3dss1_cmd_global() funktion.
You can leave them alone for now. You can also leave alone the line I was
referring to.
> It's late and I'm tired, but I still can not see which of these pairs
> of features that I can remove ...?
> if (ic->parm.dss1_io.timeout > 0) {
> if (!(pc = dss1_new_l3_process(st, -1))) {
> free_invoke_id(st, id);
> return (-2);
> }
> pc->prot.dss1.ll_id = ic->parm.dss1_io.ll_id; /* remember id */
> pc->prot.dss1.proc = ic->parm.dss1_io.proc; /* and procedure */
> }
I didn't quite get the question, if you were asking me.
> Best regards
> Rickard Strandqvist
WBR, Sergei
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] isdn: hisax: l3dss1.c: Fix for possible null pointer dereference
@ 2014-05-17 17:18 Rickard Strandqvist
2014-05-19 1:12 ` David Miller
2014-05-19 21:24 ` Sergei Shtylyov
0 siblings, 2 replies; 8+ messages in thread
From: Rickard Strandqvist @ 2014-05-17 17:18 UTC (permalink / raw)
To: Karsten Keil, Rickard Strandqvist; +Cc: netdev, linux-kernel
There is otherwise a risk of a possible null pointer dereference.
Was largely found by using a static code analysis program called cppcheck.
Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
---
drivers/isdn/hisax/l3dss1.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/isdn/hisax/l3dss1.c b/drivers/isdn/hisax/l3dss1.c
index cda7006..3ea6257 100644
--- a/drivers/isdn/hisax/l3dss1.c
+++ b/drivers/isdn/hisax/l3dss1.c
@@ -2208,8 +2208,12 @@ static int l3dss1_cmd_global(struct PStack *st, isdn_ctrl *ic)
{ free_invoke_id(st, id);
return (-2);
}
- pc->prot.dss1.ll_id = ic->parm.dss1_io.ll_id; /* remember id */
- pc->prot.dss1.proc = ic->parm.dss1_io.proc; /* and procedure */
+
+ if (pc)
+ {
+ pc->prot.dss1.ll_id = ic->parm.dss1_io.ll_id; /* remember id */
+ pc->prot.dss1.proc = ic->parm.dss1_io.proc; /* and procedure */
+ }
if (!(skb = l3_alloc_skb(l)))
{ free_invoke_id(st, id);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] isdn: hisax: l3dss1.c: Fix for possible null pointer dereference
2014-05-17 17:18 Rickard Strandqvist
@ 2014-05-19 1:12 ` David Miller
2014-05-19 21:18 ` Rickard Strandqvist
2014-05-19 21:24 ` Sergei Shtylyov
1 sibling, 1 reply; 8+ messages in thread
From: David Miller @ 2014-05-19 1:12 UTC (permalink / raw)
To: rickard_strandqvist; +Cc: isdn, netdev, linux-kernel
From: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
Date: Sat, 17 May 2014 19:18:21 +0200
> There is otherwise a risk of a possible null pointer dereference.
>
> Was largely found by using a static code analysis program called cppcheck.
>
> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
...
> @@ -2208,8 +2208,12 @@ static int l3dss1_cmd_global(struct PStack *st, isdn_ctrl *ic)
> { free_invoke_id(st, id);
> return (-2);
> }
> - pc->prot.dss1.ll_id = ic->parm.dss1_io.ll_id; /* remember id */
> - pc->prot.dss1.proc = ic->parm.dss1_io.proc; /* and procedure */
> +
> + if (pc)
> + {
> + pc->prot.dss1.ll_id = ic->parm.dss1_io.ll_id; /* remember id */
> + pc->prot.dss1.proc = ic->parm.dss1_io.proc; /* and procedure */
> + }
I think rather the intention was to put these pc-> assignments in the
timeout > 0 code block.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] isdn: hisax: l3dss1.c: Fix for possible null pointer dereference
2014-05-19 1:12 ` David Miller
@ 2014-05-19 21:18 ` Rickard Strandqvist
0 siblings, 0 replies; 8+ messages in thread
From: Rickard Strandqvist @ 2014-05-19 21:18 UTC (permalink / raw)
To: David Miller; +Cc: Karsten Keil, netdev, linux-kernel
Hi
Ahh, yes. That's niecer..
I am sending a new patch. If it is as you intended, I do the same with
the l3ni1.c file.
Best regards
Rickard Strandqvist
2014-05-19 3:12 GMT+02:00 David Miller <davem@davemloft.net>:
> From: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
> Date: Sat, 17 May 2014 19:18:21 +0200
>
>> There is otherwise a risk of a possible null pointer dereference.
>>
>> Was largely found by using a static code analysis program called cppcheck.
>>
>> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
> ...
>
>> @@ -2208,8 +2208,12 @@ static int l3dss1_cmd_global(struct PStack *st, isdn_ctrl *ic)
>> { free_invoke_id(st, id);
>> return (-2);
>> }
>> - pc->prot.dss1.ll_id = ic->parm.dss1_io.ll_id; /* remember id */
>> - pc->prot.dss1.proc = ic->parm.dss1_io.proc; /* and procedure */
>> +
>> + if (pc)
>> + {
>> + pc->prot.dss1.ll_id = ic->parm.dss1_io.ll_id; /* remember id */
>> + pc->prot.dss1.proc = ic->parm.dss1_io.proc; /* and procedure */
>> + }
>
> I think rather the intention was to put these pc-> assignments in the
> timeout > 0 code block.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] isdn: hisax: l3dss1.c: Fix for possible null pointer dereference
2014-05-17 17:18 Rickard Strandqvist
2014-05-19 1:12 ` David Miller
@ 2014-05-19 21:24 ` Sergei Shtylyov
1 sibling, 0 replies; 8+ messages in thread
From: Sergei Shtylyov @ 2014-05-19 21:24 UTC (permalink / raw)
To: Rickard Strandqvist, Karsten Keil; +Cc: netdev, linux-kernel
Hello.
On 05/17/2014 09:18 PM, Rickard Strandqvist wrote:
> There is otherwise a risk of a possible null pointer dereference.
> Was largely found by using a static code analysis program called cppcheck.
> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
> ---
> drivers/isdn/hisax/l3dss1.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
> diff --git a/drivers/isdn/hisax/l3dss1.c b/drivers/isdn/hisax/l3dss1.c
> index cda7006..3ea6257 100644
> --- a/drivers/isdn/hisax/l3dss1.c
> +++ b/drivers/isdn/hisax/l3dss1.c
> @@ -2208,8 +2208,12 @@ static int l3dss1_cmd_global(struct PStack *st, isdn_ctrl *ic)
> { free_invoke_id(st, id);
> return (-2);
> }
> - pc->prot.dss1.ll_id = ic->parm.dss1_io.ll_id; /* remember id */
> - pc->prot.dss1.proc = ic->parm.dss1_io.proc; /* and procedure */
> +
> + if (pc)
> + {
{ sould be on the same line as *if*.
> + pc->prot.dss1.ll_id = ic->parm.dss1_io.ll_id; /* remember id */
> + pc->prot.dss1.proc = ic->parm.dss1_io.proc; /* and procedure */
> + }
WBR, Sergei
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-05-20 12:23 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-05-19 21:24 [PATCH] isdn: hisax: l3dss1.c: Fix for possible null pointer dereference Rickard Strandqvist
2014-05-19 21:26 ` Sergei Shtylyov
2014-05-19 21:56 ` Rickard Strandqvist
2014-05-20 12:23 ` Sergei Shtylyov
-- strict thread matches above, loose matches on Subject: below --
2014-05-17 17:18 Rickard Strandqvist
2014-05-19 1:12 ` David Miller
2014-05-19 21:18 ` Rickard Strandqvist
2014-05-19 21:24 ` Sergei Shtylyov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).