Re: [PATCH] ipmi: ssif: potential NULL dereference in msg_done_handler()

[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@xxxxxxxxx>
> ---
>  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
>