crypto: using size_add() for kmalloc()

[PATCH] crypto: using size_add() for kmalloc()

Posted by Su Hui 7 months, 3 weeks ago

It's safer to use size_add() to replace open-coded aithmetic in allocator
arguments, because size_add() can prevent possible overflow problem.

Signed-off-by: Su Hui <suhui@nfschina.com>
---
 include/crypto/aead.h     | 3 ++-
 include/crypto/akcipher.h | 4 +++-
 include/crypto/kpp.h      | 3 ++-
 3 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/include/crypto/aead.h b/include/crypto/aead.h
index 0e8a41638678..cf212d28fe18 100644
--- a/include/crypto/aead.h
+++ b/include/crypto/aead.h
@@ -10,6 +10,7 @@
 
 #include <linux/atomic.h>
 #include <linux/container_of.h>
+#include <linux/overflow.h>
 #include <linux/crypto.h>
 #include <linux/slab.h>
 #include <linux/types.h>
@@ -433,7 +434,7 @@ static inline struct aead_request *aead_request_alloc(struct crypto_aead *tfm,
 {
 	struct aead_request *req;
 
-	req = kmalloc(sizeof(*req) + crypto_aead_reqsize(tfm), gfp);
+	req = kmalloc(size_add(sizeof(*req), crypto_aead_reqsize(tfm)), gfp);
 
 	if (likely(req))
 		aead_request_set_tfm(req, tfm);
diff --git a/include/crypto/akcipher.h b/include/crypto/akcipher.h
index cdf7da74bf2f..4c37a602cce5 100644
--- a/include/crypto/akcipher.h
+++ b/include/crypto/akcipher.h
@@ -10,6 +10,7 @@
 
 #include <linux/atomic.h>
 #include <linux/crypto.h>
+#include <linux/overflow.h>
 
 /**
  * struct akcipher_request - public key cipher request
@@ -184,7 +185,8 @@ static inline struct akcipher_request *akcipher_request_alloc(
 {
 	struct akcipher_request *req;
 
-	req = kmalloc(sizeof(*req) + crypto_akcipher_reqsize(tfm), gfp);
+	req = kmalloc(size_add(sizeof(*req),
+			       crypto_akcipher_reqsize(tfm)), gfp);
 	if (likely(req))
 		akcipher_request_set_tfm(req, tfm);
 
diff --git a/include/crypto/kpp.h b/include/crypto/kpp.h
index 2d9c4de57b69..11ae1ad41d2a 100644
--- a/include/crypto/kpp.h
+++ b/include/crypto/kpp.h
@@ -11,6 +11,7 @@
 
 #include <linux/atomic.h>
 #include <linux/container_of.h>
+#include <linux/overflow.h>
 #include <linux/crypto.h>
 #include <linux/slab.h>
 
@@ -182,7 +183,7 @@ static inline struct kpp_request *kpp_request_alloc(struct crypto_kpp *tfm,
 {
 	struct kpp_request *req;
 
-	req = kmalloc(sizeof(*req) + crypto_kpp_reqsize(tfm), gfp);
+	req = kmalloc(size_add(sizeof(*req), crypto_kpp_reqsize(tfm)), gfp);
 	if (likely(req))
 		kpp_request_set_tfm(req, tfm);
 
-- 
2.30.2

Re: [PATCH] crypto: using size_add() for kmalloc()

Posted by Herbert Xu 7 months, 3 weeks ago

On Mon, Apr 21, 2025 at 01:51:06PM +0800, Su Hui wrote:
>
> @@ -433,7 +434,7 @@ static inline struct aead_request *aead_request_alloc(struct crypto_aead *tfm,
>  {
>  	struct aead_request *req;
>  
> -	req = kmalloc(sizeof(*req) + crypto_aead_reqsize(tfm), gfp);
> +	req = kmalloc(size_add(sizeof(*req), crypto_aead_reqsize(tfm)), gfp);

This is just wrong.  You should fail the allocation altogether
rather than proceeding with a length that is insufficient.

However, reqsize shouldn't be anywhere near overflowing in the
first place.  If you're truly worried about this, you should
change the algorithm registration code to check whether reqsize
is sane.

And that needs to wait until the algorithms are fixed to not use
dynamic reqsizes.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: using size_add() for kmalloc()

Posted by Dan Carpenter 7 months, 3 weeks ago

On Mon, Apr 21, 2025 at 08:05:43PM +0800, Herbert Xu wrote:
> On Mon, Apr 21, 2025 at 01:51:06PM +0800, Su Hui wrote:
> >
> > @@ -433,7 +434,7 @@ static inline struct aead_request *aead_request_alloc(struct crypto_aead *tfm,
> >  {
> >  	struct aead_request *req;
> >  
> > -	req = kmalloc(sizeof(*req) + crypto_aead_reqsize(tfm), gfp);
> > +	req = kmalloc(size_add(sizeof(*req), crypto_aead_reqsize(tfm)), gfp);
> 
> This is just wrong.  You should fail the allocation altogether
> rather than proceeding with a length that is insufficient.

When size_add() overflows then it returns SIZE_MAX.  None of the
allocation functions can allocate SIZE_MAX bytes so kmalloc() will
fail and that's already handled correctly.  Meanwhile if
"sizeof(*req) + crypto_aead_reqsize(tfm)" overflows then the
allocation will succeed and it results in memory corruption.

This is exactly what Kees did with the mass conversion to
struct_size().  I occasionally run across places where Kees's mass
conversion patches did fix real integer overflow bugs.

regards,
dan carpenter

Re: [PATCH] crypto: using size_add() for kmalloc()

Posted by Herbert Xu 7 months, 3 weeks ago

On Tue, Apr 22, 2025 at 01:24:22PM +0300, Dan Carpenter wrote:
>
> This is exactly what Kees did with the mass conversion to
> struct_size().  I occasionally run across places where Kees's mass
> conversion patches did fix real integer overflow bugs.

The point is that the reqsize shouldn't even exceed a page in size,
let alone be anywhere near 2^32.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: using size_add() for kmalloc()

Posted by Su Hui 7 months, 3 weeks ago

On 2025/4/21 20:05, Herbert Xu wrote:
> On Mon, Apr 21, 2025 at 01:51:06PM +0800, Su Hui wrote:
>> @@ -433,7 +434,7 @@ static inline struct aead_request *aead_request_alloc(struct crypto_aead *tfm,
>>   {
>>   	struct aead_request *req;
>>   
>> -	req = kmalloc(sizeof(*req) + crypto_aead_reqsize(tfm), gfp);
>> +	req = kmalloc(size_add(sizeof(*req), crypto_aead_reqsize(tfm)), gfp);
> This is just wrong.  You should fail the allocation altogether
> rather than proceeding with a length that is insufficient.
>
> However, reqsize shouldn't be anywhere near overflowing in the
> first place.  If you're truly worried about this, you should
> change the algorithm registration code to check whether reqsize
> is sane.
>
> And that needs to wait until the algorithms are fixed to not use
> dynamic reqsizes.
Got it, thanks for your explanation.
This patch (v1 and v2) is wrong.  Sorry for the noise again.

Su Hui

Re: [PATCH] crypto: using size_add() for kmalloc()

Posted by Christophe JAILLET 7 months, 3 weeks ago

Le 21/04/2025 à 07:51, Su Hui a écrit :
> It's safer to use size_add() to replace open-coded aithmetic in allocator
> arguments, because size_add() can prevent possible overflow problem.
> 
> Signed-off-by: Su Hui <suhui@nfschina.com>
> ---
>   include/crypto/aead.h     | 3 ++-
>   include/crypto/akcipher.h | 4 +++-
>   include/crypto/kpp.h      | 3 ++-
>   3 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/include/crypto/aead.h b/include/crypto/aead.h
> index 0e8a41638678..cf212d28fe18 100644
> --- a/include/crypto/aead.h
> +++ b/include/crypto/aead.h
> @@ -10,6 +10,7 @@
>   
>   #include <linux/atomic.h>
>   #include <linux/container_of.h>
> +#include <linux/overflow.h>

You could move this 1 line below, to keep alphabetical order.
And why do you say that it is redundant in your follow-up mail?

>   #include <linux/crypto.h>
>   #include <linux/slab.h>
>   #include <linux/types.h>
> @@ -433,7 +434,7 @@ static inline struct aead_request *aead_request_alloc(struct crypto_aead *tfm,
>   {
>   	struct aead_request *req;
>   
> -	req = kmalloc(sizeof(*req) + crypto_aead_reqsize(tfm), gfp);
> +	req = kmalloc(size_add(sizeof(*req), crypto_aead_reqsize(tfm)), gfp);
>   
>   	if (likely(req))
>   		aead_request_set_tfm(req, tfm);
> diff --git a/include/crypto/akcipher.h b/include/crypto/akcipher.h
> index cdf7da74bf2f..4c37a602cce5 100644
> --- a/include/crypto/akcipher.h
> +++ b/include/crypto/akcipher.h
> @@ -10,6 +10,7 @@
>   
>   #include <linux/atomic.h>
>   #include <linux/crypto.h>
> +#include <linux/overflow.h>
>   
>   /**
>    * struct akcipher_request - public key cipher request
> @@ -184,7 +185,8 @@ static inline struct akcipher_request *akcipher_request_alloc(
>   {
>   	struct akcipher_request *req;
>   
> -	req = kmalloc(sizeof(*req) + crypto_akcipher_reqsize(tfm), gfp);
> +	req = kmalloc(size_add(sizeof(*req),
> +			       crypto_akcipher_reqsize(tfm)), gfp);
>   	if (likely(req))
>   		akcipher_request_set_tfm(req, tfm);
>   
> diff --git a/include/crypto/kpp.h b/include/crypto/kpp.h
> index 2d9c4de57b69..11ae1ad41d2a 100644
> --- a/include/crypto/kpp.h
> +++ b/include/crypto/kpp.h
> @@ -11,6 +11,7 @@
>   
>   #include <linux/atomic.h>
>   #include <linux/container_of.h>
> +#include <linux/overflow.h>

You could move this 1 line below, to keep alphabetical order.

>   #include <linux/crypto.h>
>   #include <linux/slab.h>
>   
> @@ -182,7 +183,7 @@ static inline struct kpp_request *kpp_request_alloc(struct crypto_kpp *tfm,
>   {
>   	struct kpp_request *req;
>   
> -	req = kmalloc(sizeof(*req) + crypto_kpp_reqsize(tfm), gfp);
> +	req = kmalloc(size_add(sizeof(*req), crypto_kpp_reqsize(tfm)), gfp);
>   	if (likely(req))
>   		kpp_request_set_tfm(req, tfm);
>

Re: [PATCH] crypto: using size_add() for kmalloc()

Posted by Su Hui 7 months, 3 weeks ago

On 2025/4/21 15:10, Christophe JAILLET wrote:
> Le 21/04/2025 à 07:51, Su Hui a écrit :
>> It's safer to use size_add() to replace open-coded aithmetic in 
>> allocator
>> arguments, because size_add() can prevent possible overflow problem.
>>
>> Signed-off-by: Su Hui <suhui@nfschina.com>
>> ---
>>   include/crypto/aead.h     | 3 ++-
>>   include/crypto/akcipher.h | 4 +++-
>>   include/crypto/kpp.h      | 3 ++-
>>   3 files changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/crypto/aead.h b/include/crypto/aead.h
>> index 0e8a41638678..cf212d28fe18 100644
>> --- a/include/crypto/aead.h
>> +++ b/include/crypto/aead.h
>> @@ -10,6 +10,7 @@
>>     #include <linux/atomic.h>
>>   #include <linux/container_of.h>
>> +#include <linux/overflow.h>
>
> You could move this 1 line below, to keep alphabetical order.
> And why do you say that it is redundant in your follow-up mail?
Thanks for your suggestion, I didn't notice this alphabetical order at 
first :( .
Because I found that  <linux/crypto.h> includes <linux/slab.h>, and
<linux/slab.h> includes <linux/overflow.h>, so this overflow.h is 
redundant.
>
>>   #include <linux/crypto.h>
>>   #include <linux/slab.h>
And I also found these <linux/{atomic,container_of,slab,types,list}.h> 
is included by
<linux/crypto.h>, I am trying to remove these redundant headers in v2 patch.

It's sad that remving these duplicate header files didn't save any 
compilation time,
only save some code space.
>>   #include <linux/types.h>
>> @@ -433,7 +434,7 @@ static inline struct aead_request 
>> *aead_request_alloc(struct crypto_aead *tfm,
>> --- a/include/crypto/kpp.h
>> +++ b/include/crypto/kpp.h
>> @@ -11,6 +11,7 @@
>>     #include <linux/atomic.h>
>>   #include <linux/container_of.h>
>> +#include <linux/overflow.h>
>
> You could move this 1 line below, to keep alphabetical order.
This overflow.h is redundant too.
>
>>   #include <linux/crypto.h>
>>
>

Re: [PATCH] crypto: using size_add() for kmalloc()

Posted by Christophe JAILLET 7 months, 3 weeks ago

Le 21/04/2025 à 09:43, Su Hui a écrit :
> On 2025/4/21 15:10, Christophe JAILLET wrote:
>> Le 21/04/2025 à 07:51, Su Hui a écrit :
>>> It's safer to use size_add() to replace open-coded aithmetic in 
>>> allocator
>>> arguments, because size_add() can prevent possible overflow problem.
>>>
>>> Signed-off-by: Su Hui <suhui@nfschina.com>
>>> ---
>>>   include/crypto/aead.h     | 3 ++-
>>>   include/crypto/akcipher.h | 4 +++-
>>>   include/crypto/kpp.h      | 3 ++-
>>>   3 files changed, 7 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/include/crypto/aead.h b/include/crypto/aead.h
>>> index 0e8a41638678..cf212d28fe18 100644
>>> --- a/include/crypto/aead.h
>>> +++ b/include/crypto/aead.h
>>> @@ -10,6 +10,7 @@
>>>     #include <linux/atomic.h>
>>>   #include <linux/container_of.h>
>>> +#include <linux/overflow.h>
>>
>> You could move this 1 line below, to keep alphabetical order.
>> And why do you say that it is redundant in your follow-up mail?
> Thanks for your suggestion, I didn't notice this alphabetical order at 
> first :( .
> Because I found that  <linux/crypto.h> includes <linux/slab.h>, and
> <linux/slab.h> includes <linux/overflow.h>, so this overflow.h is 
> redundant.

It is usually considered best practice to include what is used, and not 
relying on indirect includes.

Should these others includes change one day, then some apparently 
unrelated files will fails to built.

CJ

Re: [PATCH] crypto: using size_add() for kmalloc()

Posted by Su Hui 7 months, 3 weeks ago

On 2025/4/21 16:32, Christophe JAILLET wrote:
> Le 21/04/2025 à 09:43, Su Hui a écrit :
>> On 2025/4/21 15:10, Christophe JAILLET wrote:
>>> Le 21/04/2025 à 07:51, Su Hui a écrit :
>>>> It's safer to use size_add() to replace open-coded aithmetic in 
>>>> allocator
>>>> arguments, because size_add() can prevent possible overflow problem.
>>>>
>>>> Signed-off-by: Su Hui <suhui@nfschina.com>
>>>> ---
>>>>   include/crypto/aead.h     | 3 ++-
>>>>   include/crypto/akcipher.h | 4 +++-
>>>>   include/crypto/kpp.h      | 3 ++-
>>>>   3 files changed, 7 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/include/crypto/aead.h b/include/crypto/aead.h
>>>> index 0e8a41638678..cf212d28fe18 100644
>>>> --- a/include/crypto/aead.h
>>>> +++ b/include/crypto/aead.h
>>>> @@ -10,6 +10,7 @@
>>>>     #include <linux/atomic.h>
>>>>   #include <linux/container_of.h>
>>>> +#include <linux/overflow.h>
>>>
>>> You could move this 1 line below, to keep alphabetical order.
>>> And why do you say that it is redundant in your follow-up mail?
>> Thanks for your suggestion, I didn't notice this alphabetical order 
>> at first :( .
>> Because I found that  <linux/crypto.h> includes <linux/slab.h>, and
>> <linux/slab.h> includes <linux/overflow.h>, so this overflow.h is 
>> redundant.
>
> It is usually considered best practice to include what is used, and 
> not relying on indirect includes.
>
> Should these others includes change one day, then some apparently 
> unrelated files will fails to built.
>
I already send a v2 patch, too fast for this v2 sending :(.
v2: https://lore.kernel.org/all/20250421083116.1161805-1-suhui@nfschina.com/

I agreed with 'include what is used'.  So I guess v1 is enough and v2 
maybe a wrong patchset.
Sorry for the noise.

Su Hui

Re: [PATCH] crypto: using size_add() for kmalloc()

Posted by Su Hui 7 months, 3 weeks ago

On 2025/4/21 16:46, Su Hui wrote:
> On 2025/4/21 16:32, Christophe JAILLET wrote:
>> Le 21/04/2025 à 09:43, Su Hui a écrit :
>>> On 2025/4/21 15:10, Christophe JAILLET wrote:
>>>> Le 21/04/2025 à 07:51, Su Hui a écrit :
>>>>> It's safer to use size_add() to replace open-coded aithmetic in 
>>>>> allocator
>>>>> arguments, because size_add() can prevent possible overflow problem.
>>>>>
>>>>> Signed-off-by: Su Hui <suhui@nfschina.com>
>>>>> ---
>>>>>   include/crypto/aead.h     | 3 ++-
>>>>>   include/crypto/akcipher.h | 4 +++-
>>>>>   include/crypto/kpp.h      | 3 ++-
>>>>>   3 files changed, 7 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/include/crypto/aead.h b/include/crypto/aead.h
>>>>> index 0e8a41638678..cf212d28fe18 100644
>>>>> --- a/include/crypto/aead.h
>>>>> +++ b/include/crypto/aead.h
>>>>> @@ -10,6 +10,7 @@
>>>>>     #include <linux/atomic.h>
>>>>>   #include <linux/container_of.h>
>>>>> +#include <linux/overflow.h>
>>>>
>>>> You could move this 1 line below, to keep alphabetical order.
>>>> And why do you say that it is redundant in your follow-up mail?
>>> Thanks for your suggestion, I didn't notice this alphabetical order 
>>> at first :( .
>>> Because I found that  <linux/crypto.h> includes <linux/slab.h>, and
>>> <linux/slab.h> includes <linux/overflow.h>, so this overflow.h is 
>>> redundant.
>>
>> It is usually considered best practice to include what is used, and 
>> not relying on indirect includes.
>>
>> Should these others includes change one day, then some apparently 
>> unrelated files will fails to built.
>>
> I already send a v2 patch, too fast for this v2 sending :(.
> v2: 
> https://lore.kernel.org/all/20250421083116.1161805-1-suhui@nfschina.com/
>
> I agreed with 'include what is used'.  So I guess v1 is enough and v2 
> maybe a wrong patchset.
> Sorry for the noise.

Oh, I forget to keep alphabetical order, so v3 is needed if there is no 
other suggestions.

Re: [PATCH] crypto: using size_add() for kmalloc()

Posted by Su Hui 7 months, 3 weeks ago

On 2025/4/21 13:51, Su Hui wrote:
> It's safer to use size_add() to replace open-coded aithmetic in allocator
> arguments, because size_add() can prevent possible overflow problem.
>
> Signed-off-by: Su Hui <suhui@nfschina.com>
> ---
>   include/crypto/aead.h     | 3 ++-
>   include/crypto/akcipher.h | 4 +++-
>   include/crypto/kpp.h      | 3 ++-
>   3 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/include/crypto/aead.h b/include/crypto/aead.h
> index 0e8a41638678..cf212d28fe18 100644
> --- a/include/crypto/aead.h
> +++ b/include/crypto/aead.h
> @@ -10,6 +10,7 @@
>   
>   #include <linux/atomic.h>
>   #include <linux/container_of.h>
> +#include <linux/overflow.h>
Sorry for this redundant <linux/overflow.h>, will remove in v2 patch.
>   #include <linux/crypto.h>
>   #include <linux/slab.h>
>   #include <linux/types.h>
> @@ -433,7 +434,7 @@ static inline struct aead_request *aead_request_alloc(struct crypto_aead *tfm,
>   {
>   	struct aead_request *req;
>   
> -	req = kmalloc(sizeof(*req) + crypto_aead_reqsize(tfm), gfp);
> +	req = kmalloc(size_add(sizeof(*req), crypto_aead_reqsize(tfm)), gfp);
>   
>   	if (likely(req))
>   		aead_request_set_tfm(req, tfm);
> diff --git a/include/crypto/akcipher.h b/include/crypto/akcipher.h
> index cdf7da74bf2f..4c37a602cce5 100644
> --- a/include/crypto/akcipher.h
> +++ b/include/crypto/akcipher.h
> @@ -10,6 +10,7 @@
>   
>   #include <linux/atomic.h>
>   #include <linux/crypto.h>
> +#include <linux/overflow.h>
>   
>   /**
>    * struct akcipher_request - public key cipher request
> @@ -184,7 +185,8 @@ static inline struct akcipher_request *akcipher_request_alloc(
>   {
>   	struct akcipher_request *req;
>   
> -	req = kmalloc(sizeof(*req) + crypto_akcipher_reqsize(tfm), gfp);
> +	req = kmalloc(size_add(sizeof(*req),
> +			       crypto_akcipher_reqsize(tfm)), gfp);
>   	if (likely(req))
>   		akcipher_request_set_tfm(req, tfm);
>   
> diff --git a/include/crypto/kpp.h b/include/crypto/kpp.h
> index 2d9c4de57b69..11ae1ad41d2a 100644
> --- a/include/crypto/kpp.h
> +++ b/include/crypto/kpp.h
> @@ -11,6 +11,7 @@
>   
>   #include <linux/atomic.h>
>   #include <linux/container_of.h>
> +#include <linux/overflow.h>
>   #include <linux/crypto.h>
>   #include <linux/slab.h>
>   
> @@ -182,7 +183,7 @@ static inline struct kpp_request *kpp_request_alloc(struct crypto_kpp *tfm,
>   {
>   	struct kpp_request *req;
>   
> -	req = kmalloc(sizeof(*req) + crypto_kpp_reqsize(tfm), gfp);
> +	req = kmalloc(size_add(sizeof(*req), crypto_kpp_reqsize(tfm)), gfp);
>   	if (likely(req))
>   		kpp_request_set_tfm(req, tfm);
>