* [PATCH] ipmi: ssif: potential NULL dereference in msg_done_handler()
@ 2022-04-01 3:27 Haowen Bai
2022-04-01 12:48 ` Corey Minyard
0 siblings, 1 reply; 4+ messages in thread
From: Haowen Bai @ 2022-04-01 3:27 UTC (permalink / raw)
To: Corey Minyard; +Cc: Haowen Bai, openipmi-developer, linux-kernel
msg could be null without checking null and return, but still dereference
msg->rsp[2] and will lead to a null pointer trigger.
Signed-off-by: Haowen Bai <baihaowen@meizu.com>
---
drivers/char/ipmi/ipmi_ssif.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
index f199cc1..9383de3 100644
--- a/drivers/char/ipmi/ipmi_ssif.c
+++ b/drivers/char/ipmi/ipmi_ssif.c
@@ -814,7 +814,7 @@ static void msg_done_handler(struct ssif_info *ssif_info, int result,
break;
case SSIF_GETTING_EVENTS:
- if ((result < 0) || (len < 3) || (msg->rsp[2] != 0)) {
+ if ((result < 0) || (len < 3) || (msg && (msg->rsp[2] != 0))) {
/* Error getting event, probably done. */
msg->done(msg);
--
2.7.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] ipmi: ssif: potential NULL dereference in msg_done_handler()
2022-04-01 3:27 [PATCH] ipmi: ssif: potential NULL dereference in msg_done_handler() Haowen Bai
@ 2022-04-01 12:48 ` Corey Minyard
2022-04-02 3:52 ` baihaowen
0 siblings, 1 reply; 4+ messages in thread
From: Corey Minyard @ 2022-04-01 12:48 UTC (permalink / raw)
To: Haowen Bai; +Cc: openipmi-developer, linux-kernel
On Fri, Apr 01, 2022 at 11:27:45AM +0800, Haowen Bai wrote:
> msg could be null without checking null and return, but still dereference
> msg->rsp[2] and will lead to a null pointer trigger.
Actually:
If you look at the big picture (how the rest of the code works), it's
not possible for msg to be NULL in these cases. However, being
defensive here is probably a good idea.
There are two of these cases, why didn't you fix both of them?
This still doesn't fix the problem. There is an "else if" in both
cases that also uses msg.
You can't just look at the output of some code analysis tool and make a
blind decision like this. You have to look at the big picture. And you
have to analyze the code carefully.
The right way to be defensive here is to add:
if (!msg) {
ipmi_ssif_unlock_cond(ssif_info, flags);
break;
}
in both cases. And probably add a log, since this means something else
went wrong.
Anyway, I'll add a patch for defensive measure and give you credit for
pointing it out.
-corey
>
> Signed-off-by: Haowen Bai <baihaowen@meizu.com>
> ---
> drivers/char/ipmi/ipmi_ssif.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
> index f199cc1..9383de3 100644
> --- a/drivers/char/ipmi/ipmi_ssif.c
> +++ b/drivers/char/ipmi/ipmi_ssif.c
> @@ -814,7 +814,7 @@ static void msg_done_handler(struct ssif_info *ssif_info, int result,
> break;
>
> case SSIF_GETTING_EVENTS:
> - if ((result < 0) || (len < 3) || (msg->rsp[2] != 0)) {
> + if ((result < 0) || (len < 3) || (msg && (msg->rsp[2] != 0))) {
> /* Error getting event, probably done. */
> msg->done(msg);
>
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ipmi: ssif: potential NULL dereference in msg_done_handler()
2022-04-01 12:48 ` Corey Minyard
@ 2022-04-02 3:52 ` baihaowen
2022-04-02 4:01 ` [PATCH V2] " Haowen Bai
0 siblings, 1 reply; 4+ messages in thread
From: baihaowen @ 2022-04-02 3:52 UTC (permalink / raw)
To: minyard; +Cc: openipmi-developer, linux-kernel
Dear Corey
Thank you for your kindly reply and suggestion. :)
在 4/1/22 8:48 PM, Corey Minyard 写道:
> On Fri, Apr 01, 2022 at 11:27:45AM +0800, Haowen Bai wrote:
>> msg could be null without checking null and return, but still dereference
>> msg->rsp[2] and will lead to a null pointer trigger.
> Actually:
>
> If you look at the big picture (how the rest of the code works), it's
> not possible for msg to be NULL in these cases. However, being
> defensive here is probably a good idea.
>
> There are two of these cases, why didn't you fix both of them?
>
> This still doesn't fix the problem. There is an "else if" in both
> cases that also uses msg.
>
> You can't just look at the output of some code analysis tool and make a
> blind decision like this. You have to look at the big picture. And you
> have to analyze the code carefully.
>
> The right way to be defensive here is to add:
> if (!msg) {
> ipmi_ssif_unlock_cond(ssif_info, flags);
> break;
> }
> in both cases. And probably add a log, since this means something else
> went wrong.
>
> Anyway, I'll add a patch for defensive measure and give you credit for
> pointing it out.
>
> -corey
>
>> Signed-off-by: Haowen Bai <baihaowen@meizu.com>
>> ---
>> drivers/char/ipmi/ipmi_ssif.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
>> index f199cc1..9383de3 100644
>> --- a/drivers/char/ipmi/ipmi_ssif.c
>> +++ b/drivers/char/ipmi/ipmi_ssif.c
>> @@ -814,7 +814,7 @@ static void msg_done_handler(struct ssif_info *ssif_info, int result,
>> break;
>>
>> case SSIF_GETTING_EVENTS:
>> - if ((result < 0) || (len < 3) || (msg->rsp[2] != 0)) {
>> + if ((result < 0) || (len < 3) || (msg && (msg->rsp[2] != 0))) {
>> /* Error getting event, probably done. */
>> msg->done(msg);
>>
>> --
>> 2.7.4
>>
--
Haowen Bai
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH V2] ipmi: ssif: potential NULL dereference in msg_done_handler()
2022-04-02 3:52 ` baihaowen
@ 2022-04-02 4:01 ` Haowen Bai
0 siblings, 0 replies; 4+ messages in thread
From: Haowen Bai @ 2022-04-02 4:01 UTC (permalink / raw)
To: Corey Minyard; +Cc: Haowen Bai, openipmi-developer, linux-kernel
msg could be null without checking null and return, but still dereference
msg->rsp[2] and will lead to a null pointer trigger.
Signed-off-by: Haowen Bai <baihaowen@meizu.com>
---
V1->V2: check msg both cases that also uses msg; add logs;
drivers/char/ipmi/ipmi_ssif.c | 19 ++++++++++++++++++-
1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
index f199cc194844..eb6bdd44b69e 100644
--- a/drivers/char/ipmi/ipmi_ssif.c
+++ b/drivers/char/ipmi/ipmi_ssif.c
@@ -758,8 +758,11 @@ static void msg_done_handler(struct ssif_info *ssif_info, int result,
switch (ssif_info->ssif_state) {
case SSIF_NORMAL:
ipmi_ssif_unlock_cond(ssif_info, flags);
- if (!msg)
+ if (!msg) {
+ dev_dbg(&ssif_info->client->dev,
+ "msg was null, case SSIF_NORMAL\n");
break;
+ }
if (result < 0)
return_hosed_msg(ssif_info, msg);
@@ -814,6 +817,13 @@ static void msg_done_handler(struct ssif_info *ssif_info, int result,
break;
case SSIF_GETTING_EVENTS:
+ if (!msg) {
+ ipmi_ssif_unlock_cond(ssif_info, flags);
+ dev_dbg(&ssif_info->client->dev,
+ "msg was null, case SSIF_GETTING_EVENTS\n");
+ break;
+ }
+
if ((result < 0) || (len < 3) || (msg->rsp[2] != 0)) {
/* Error getting event, probably done. */
msg->done(msg);
@@ -838,6 +848,13 @@ static void msg_done_handler(struct ssif_info *ssif_info, int result,
break;
case SSIF_GETTING_MESSAGES:
+ if (!msg) {
+ ipmi_ssif_unlock_cond(ssif_info, flags);
+ dev_dbg(&ssif_info->client->dev,
+ "msg was null, case SSIF_GETTING_MESSAGES\n");
+ break;
+ }
+
if ((result < 0) || (len < 3) || (msg->rsp[2] != 0)) {
/* Error getting event, probably done. */
msg->done(msg);
--
2.7.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-04-02 4:01 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-01 3:27 [PATCH] ipmi: ssif: potential NULL dereference in msg_done_handler() Haowen Bai
2022-04-01 12:48 ` Corey Minyard
2022-04-02 3:52 ` baihaowen
2022-04-02 4:01 ` [PATCH V2] " Haowen Bai
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).