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.
Fixes: a8ea8bdd9df9 ("lib/mpi: Extend the MPI library")
Signed-off-by: Zheng Wang <[email protected]>
---
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);
}
--
2.25.1
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 <[email protected]>
> ---
> 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
At 2023-02-07 09:41:35, "Tianjia Zhang" <[email protected]> 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 <[email protected]>
>> ---
>> 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