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

[Jeffrey E Altman <jaltman@auristor.com>]

[-- 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 --]