Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Sender: Corey Minyard <tcminyard@gmail.com>
Date:   Fri, 1 Apr 2022 07:48:09 -0500
From:   Corey Minyard <minyard@acm.org>
To:     Haowen Bai <baihaowen@meizu.com>
Cc:     openipmi-developer@lists.sourceforge.net,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] ipmi: ssif: potential NULL dereference in
 msg_done_handler()
Message-ID: <20220401124809.GF29333@minyard.net>
Reply-To: minyard@acm.org
References: <1648783665-19237-1-git-send-email-baihaowen@meizu.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1648783665-19237-1-git-send-email-baihaowen@meizu.com>
Precedence: bulk

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
>