linux-kernel - Re: [PATCH] afs: Replace simple_strtoul with kstrtoul in afs_parse

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ea42a3fd-7ce8-43e0-a2d5-c5353070bfe2@auristor.com>
Date: Thu, 29 May 2025 19:35:39 -0400
From: Jeffrey E Altman <jaltman@...istor.com>
To: Su Hui <suhui@...china.com>, dhowells@...hat.com, marc.dionne@...istor.com
Cc: linux-afs@...ts.infradead.org, linux-kernel@...r.kernel.org,
 kernel-janitors@...r.kernel.org
Subject: Re: [PATCH] afs: Replace simple_strtoul with kstrtoul in
 afs_parse_address

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



Download attachment "smime.p7s" of type "application/pkcs7-signature" (4276 bytes)