Message-ID: <83391944-e0bf-288f-f50e-cc1a28b89a34@linux.alibaba.com>
Date:   Tue, 7 Feb 2023 10:41:35 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.6.1
Subject: Re: [PATCH] lib/mpi: Fix poential NULL pointer dereference in
 mpi_fdiv_q
To:     Zheng Wang <zyytlz.wz@163.com>
Cc:     linux-kernel@vger.kernel.org, hackerzheng666@gmail.com,
        alex000young@gmail.com
References: <20230203084302.589017-1-zyytlz.wz@163.com>
Content-Language: en-US
From:   Tianjia Zhang <tianjia.zhang@linux.alibaba.com>
In-Reply-To: <20230203084302.589017-1-zyytlz.wz@163.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Zheng Wang,

On 2/3/23 4:43 PM, Zheng Wang wrote:
> in lib/mpi, there is multiple function that not check the return
> value of mpi_alloc. One case is mpi_fdiv_q, if tmp == NULL,
> tmp->nlimbs in mpi_fdiv_qr will cause NULL pointer dereference.
> As the code is too much, here I only fix one of them. Other
> function like mpi_barrett_init mpi_copy mpi_alloc_like mpi_set
> mpi_set_ui mpi_alloc_set_ui has the same problem.
> 
> Please let me know if there is a better way to fix the
> problem.
> 
> Note that, as a bug found by static analysis, it can be a false
> positive or hard to trigger.

Thanks for your patch, lib/mpi is ported from libgcrypt, as an
application layer library, such error handling is no problem, but for
the kernel, these return values should be checked, even if these errors
are difficult to trigger.

As you said, there are many places where this check is ignored, and even
the error return value needs to be passed upwards, which should be
upgraded and fixed uniformly, even some callers need such a check, such
as the SM2 algorithm. These checks were initially ignored in the
interest of code brevity and rapid development, and it looks like it's
time to fix them.

> 
> Fixes: a8ea8bdd9df9 ("lib/mpi: Extend the MPI library")
> Signed-off-by: Zheng Wang <zyytlz.wz@163.com>
> ---
>   lib/mpi/mpi-div.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/mpi/mpi-div.c b/lib/mpi/mpi-div.c
> index 45beab8b9e9e..e8642265d7d4 100644
> --- a/lib/mpi/mpi-div.c
> +++ b/lib/mpi/mpi-div.c
> @@ -43,7 +43,8 @@ void mpi_fdiv_r(MPI rem, MPI dividend, MPI divisor)
>   void mpi_fdiv_q(MPI quot, MPI dividend, MPI divisor)
>   {
>   	MPI tmp = mpi_alloc(mpi_get_nlimbs(quot));
> -	mpi_fdiv_qr(quot, tmp, dividend, divisor);
> +	if (tmp)
> +		mpi_fdiv_qr(quot, tmp, dividend, divisor);
>   	mpi_free(tmp);
>   }
>   

As far as this is concerned, the allocation failure should pass the
ENOMEM error upwards, not the void type.

Best regards,
Tianjia