[PATCH] char: ipmi: Fix possible pointer dereferences in msg_done_handler()

[PATCH] char: ipmi: Fix possible pointer dereferences in msg_done_handler()

[Jia-Ju Bai]

In msg_done_handler(), there is an if statement on line 778 to check
whether msg is NULL:
    if (msg)

When msg is NULL, it is used on line 845:
    if ((result < 0) || (len < 3) || (msg->rsp[2] != 0))
and line 869:
    if ((result < 0) || (len < 3) || (msg->rsp[2] != 0))

Thus, possible null-pointer dereferences may occur.

To fix these bugs, msg is checked before being used.

These bugs are found by a static analysis tool STCheck written by us.

Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
---
 drivers/char/ipmi/ipmi_ssif.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
index 305fa5054274..2e40a98d9939 100644
--- a/drivers/char/ipmi/ipmi_ssif.c
+++ b/drivers/char/ipmi/ipmi_ssif.c
@@ -842,6 +842,8 @@ static void msg_done_handler(struct ssif_info *ssif_info, int result,
 		break;
 
 	case SSIF_GETTING_EVENTS:
+		if (!msg)
+			break;
 		if ((result < 0) || (len < 3) || (msg->rsp[2] != 0)) {
 			/* Error getting event, probably done. */
 			msg->done(msg);
@@ -866,6 +868,8 @@ static void msg_done_handler(struct ssif_info *ssif_info, int result,
 		break;
 
 	case SSIF_GETTING_MESSAGES:
+		if (!msg)
+			break;
 		if ((result < 0) || (len < 3) || (msg->rsp[2] != 0)) {
 			/* Error getting event, probably done. */
 			msg->done(msg);
-- 
2.17.0

Re: [PATCH] char: ipmi: Fix possible pointer dereferences in msg_done_handler()

[Corey Minyard]

On Wed, Jul 24, 2019 at 06:25:28PM +0800, Jia-Ju Bai wrote:
> In msg_done_handler(), there is an if statement on line 778 to check
> whether msg is NULL:
>     if (msg)
> 
> When msg is NULL, it is used on line 845:
>     if ((result < 0) || (len < 3) || (msg->rsp[2] != 0))
> and line 869:
>     if ((result < 0) || (len < 3) || (msg->rsp[2] != 0))

You cannot get into those states without msg being set, so there
is no need to check for that msg is NULL there.  If you look
at those states elsewhere, you can see that curr_msg is set every
time you go into those states.

-corey

> 
> Thus, possible null-pointer dereferences may occur.
> 
> To fix these bugs, msg is checked before being used.
> 
> These bugs are found by a static analysis tool STCheck written by us.
> 
> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
> ---
>  drivers/char/ipmi/ipmi_ssif.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
> index 305fa5054274..2e40a98d9939 100644
> --- a/drivers/char/ipmi/ipmi_ssif.c
> +++ b/drivers/char/ipmi/ipmi_ssif.c
> @@ -842,6 +842,8 @@ static void msg_done_handler(struct ssif_info *ssif_info, int result,
>  		break;
>  
>  	case SSIF_GETTING_EVENTS:
> +		if (!msg)
> +			break;
>  		if ((result < 0) || (len < 3) || (msg->rsp[2] != 0)) {
>  			/* Error getting event, probably done. */
>  			msg->done(msg);
> @@ -866,6 +868,8 @@ static void msg_done_handler(struct ssif_info *ssif_info, int result,
>  		break;
>  
>  	case SSIF_GETTING_MESSAGES:
> +		if (!msg)
> +			break;
>  		if ((result < 0) || (len < 3) || (msg->rsp[2] != 0)) {
>  			/* Error getting event, probably done. */
>  			msg->done(msg);
> -- 
> 2.17.0
>