Re:Re: [PATCH] lib/mpi: Fix poential NULL pointer dereference in mpi_fdiv_q

[王征 <zyytlz.wz@163.com>]

At 2023-02-07 09:41:35, "Tianjia Zhang" <tianjia.zhang@linux.alibaba.com> wrote:
>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.
>

Thank you for taking the time to consider my question. Hope it can be
fixed quickly.

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

Yes, I think the good way is to refactor the prototype of function,
making it available to receiver error code and pass it to the caller
function, until there is an error handler to handle it.

Best regards,
Zheng Wang