linux-kernel - Re: [PATCH] ipmi: ssif: potential NULL dereference in msg_done

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220401124809.GF29333@minyard.net>
Date:   Fri, 1 Apr 2022 07:48:09 -0500
From:   Corey Minyard <minyard@....org>
To:     Haowen Bai <baihaowen@...zu.com>
Cc:     openipmi-developer@...ts.sourceforge.net,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ipmi: ssif: potential NULL dereference in
 msg_done_handler()

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@...zu.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
>