[PATCH] afs: Replace simple_strtoul with kstrtoul in afs_parse_address

[PATCH] afs: Replace simple_strtoul with kstrtoul in afs_parse_address

[Su Hui]

kstrtoul() is better because simple_strtoul() ignores overflow which
may lead to unexpected results.

Signed-off-by: Su Hui <suhui@nfschina.com>
---
 fs/afs/addr_prefs.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/fs/afs/addr_prefs.c b/fs/afs/addr_prefs.c
index c0384201b8fe..ae4f4b371882 100644
--- a/fs/afs/addr_prefs.c
+++ b/fs/afs/addr_prefs.c
@@ -118,7 +118,10 @@ static int afs_parse_address(char *p, struct afs_addr_preference *pref)
 
 	if (*p == '/') {
 		p++;
-		tmp = simple_strtoul(p, &p, 10);
+		if (kstrtoul(p, 10, &tmp)) {
+			pr_warn("Invalid address\n");
+			return -EINVAL;
+		}
 		if (tmp > mask) {
 			pr_warn("Subnet mask too large\n");
 			return -EINVAL;
@@ -130,11 +133,6 @@ static int afs_parse_address(char *p, struct afs_addr_preference *pref)
 		mask = tmp;
 	}
 
-	if (*p) {
-		pr_warn("Invalid address\n");
-		return -EINVAL;
-	}
-
 	pref->subnet_mask = mask;
 	return 0;
 }
-- 
2.30.2

Re: [PATCH] afs: Replace simple_strtoul with kstrtoul in afs_parse_address

[Jeffrey E Altman]

[-- Attachment #1: Type: text/plain, Size: 1748 bytes --]

On 5/27/2025 4:49 AM, Su Hui wrote:
> kstrtoul() is better because simple_strtoul() ignores overflow which
> may lead to unexpected results.
>
> Signed-off-by: Su Hui<suhui@nfschina.com>
> ---
>   fs/afs/addr_prefs.c | 10 ++++------
>   1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/fs/afs/addr_prefs.c b/fs/afs/addr_prefs.c
> index c0384201b8fe..ae4f4b371882 100644
> --- a/fs/afs/addr_prefs.c
> +++ b/fs/afs/addr_prefs.c
> @@ -118,7 +118,10 @@ static int afs_parse_address(char *p, struct afs_addr_preference *pref)
>   
>   	if (*p == '/') {
>   		p++;
> -		tmp = simple_strtoul(p, &p, 10);
> +		if (kstrtoul(p, 10, &tmp)) {
> +			pr_warn("Invalid address\n");
> +			return -EINVAL;
> +		}
>   		if (tmp > mask) {
>   			pr_warn("Subnet mask too large\n");
>   			return -EINVAL;
> @@ -130,11 +133,6 @@ static int afs_parse_address(char *p, struct afs_addr_preference *pref)
>   		mask = tmp;
>   	}
>   
> -	if (*p) {
> -		pr_warn("Invalid address\n");
> -		return -EINVAL;
> -	}
> -
>   	pref->subnet_mask = mask;
>   	return 0;
>   }

Su Hui,

Thank you for the contribution but I do not believe this patch is correct.

The second block is required even if the simple_stroul() is replaced by 
kstrtoul() as it protects against an input string which does not contain 
the optional subnet mask but has some other characters after the address.

afs_parse_address() already has its own overflow checks following the 
simple_strtoul() call which is specific to the interpretation of the 
allowed subnet mask values.

Do you see an overflow condition which would not be caught by those 
checks which would be caught by use of kstrtoul()?

Thanks again.

Jeffrey Altman



[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4276 bytes --]

Re: [PATCH] afs: Replace simple_strtoul with kstrtoul in afs_parse_address

[David Howells]

Su Hui <suhui@nfschina.com> wrote:

> kstrtoul() is better because simple_strtoul() ignores overflow which
> may lead to unexpected results.

Overflow in what sense?  Are we talking about a mathematical overflow or not
checking the text beyond the end of the number?

David

Re: [PATCH] afs: Replace simple_strtoul with kstrtoul in afs_parse_address

[Su Hui]

On 5/30/25 7:35 AM, Jeffrey E Altman wrote:
> On 5/27/2025 4:49 AM, Su Hui wrote:
>> kstrtoul() is better because simple_strtoul() ignores overflow which
>> may lead to unexpected results.
>>
>> Signed-off-by: Su Hui<suhui@nfschina.com>
>> ---
>>
> Su Hui,
>
> Thank you for the contribution but I do not believe this patch is 
> correct.
>
Oh, really sorry for my stupid mistake. Thanks for your review too :) .
> The second block is required even if the simple_stroul() is replaced 
> by kstrtoul() as it protects against an input string which does not 
> contain the optional subnet mask but has some other characters after 
> the address.
>
> afs_parse_address() already has its own overflow checks following the 
> simple_strtoul() call which is specific to the interpretation of the 
> allowed subnet mask values.
Agreed, it's my fault that I only see the pattern about 
'simple_strtoxx(....)' and 'if (*p)'.....
>
> Do you see an overflow condition which would not be caught by those 
> checks which would be caught by use of kstrtoul()?
Actually, no example in reality.
If p can equal to '0xffffffffffffffff0000000000000001', simple_strtoul() 
and kstroul() all transform 'p' to unsigned long value '0x1'.
But kstrtoul() return an error and we can know overflow happens.  If 'p' 
can be a very long string, kstroul() make sense.

Su Hui

Re: [PATCH] afs: Replace simple_strtoul with kstrtoul in afs_parse_address

[Su Hui]

On 5/30/25 5:32 PM, David Howells wrote:
> Su Hui <suhui@nfschina.com> wrote:
>
>> kstrtoul() is better because simple_strtoul() ignores overflow which
>> may lead to unexpected results.
> Overflow in what sense?  Are we talking about a mathematical overflow or not
> checking the text beyond the end of the number?

IMO, It's meaning that  the number represented by the string exceeds the 
type range. Like this code:

const char str[] = "0xffffffffffffffff0000000000000001";
unsigned long res;
res = simple_strtoul(str, &p, 0); //overflow happends and  res = 0x1
err = kstrtoul(str, 0, &res); // overflow happends and res = 0x1,  err = 
-ERANGE

Su Hui

Re: [PATCH] afs: Replace simple_strtoul with kstrtoul in afs_parse_address

[Jeffrey E Altman]

[-- Attachment #1: Type: text/plain, Size: 749 bytes --]

On 5/30/2025 6:29 AM, Su Hui wrote:
> On 5/30/25 7:35 AM, Jeffrey E Altman wrote:
>>
>> Do you see an overflow condition which would not be caught by those 
>> checks which would be caught by use of kstrtoul()?
> Actually, no example in reality.
> If p can equal to '0xffffffffffffffff0000000000000001', 
> simple_strtoul() and kstroul() all transform 'p' to unsigned long 
> value '0x1'.
> But kstrtoul() return an error and we can know overflow happens.  If 
> 'p' can be a very long string, kstroul() make sense.
>
The expected use case is for the input string not to exceed 3 
characters.  The valid range is decimal 0 to 128.  That could be 
enforced by switching to simple_strntoul() and relying upon the existing 
checks.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4276 bytes --]

Re: [PATCH] afs: Replace simple_strtoul with kstrtoul in afs_parse_address

[Su Hui]

On 5/30/25 8:43 PM, Jeffrey E Altman wrote:
> On 5/30/2025 6:29 AM, Su Hui wrote:
>> On 5/30/25 7:35 AM, Jeffrey E Altman wrote:
>>>
>>> Do you see an overflow condition which would not be caught by those 
>>> checks which would be caught by use of kstrtoul()?
>> Actually, no example in reality.
>> If p can equal to '0xffffffffffffffff0000000000000001', 
>> simple_strtoul() and kstroul() all transform 'p' to unsigned long 
>> value '0x1'.
>> But kstrtoul() return an error and we can know overflow happens.  If 
>> 'p' can be a very long string, kstroul() make sense.
>>
> The expected use case is for the input string not to exceed 3 
> characters.  The valid range is decimal 0 to 128.  That could be 
> enforced by switching to simple_strntoul() and relying upon the 
> existing checks.

Got it, thanks for your reply and sorry for the noise.

Su Hui