Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 24 Jul 2019 08:03:49 -0500
From:   Corey Minyard <minyard@acm.org>
To:     Jia-Ju Bai <baijiaju1990@gmail.com>
Cc:     arnd@arndb.de, gregkh@linuxfoundation.org,
        openipmi-developer@lists.sourceforge.net,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] char: ipmi: Fix possible pointer dereferences in
 msg_done_handler()
Message-ID: <20190724130349.GI3066@minyard.net>
Reply-To: minyard@acm.org
References: <20190724102528.2165-1-baijiaju1990@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190724102528.2165-1-baijiaju1990@gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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
>