linux-kernel - Re: [PATCH] char: ipmi: Fix possible pointer dereferences 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] [day] [month] [year] [list]

Date:   Wed, 24 Jul 2019 08:03:49 -0500
From:   Corey Minyard <minyard@....org>
To:     Jia-Ju Bai <baijiaju1990@...il.com>
Cc:     arnd@...db.de, gregkh@...uxfoundation.org,
        openipmi-developer@...ts.sourceforge.net,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] char: ipmi: Fix possible pointer dereferences in
 msg_done_handler()

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