linux-kernel - Re: [PATCH v3 2/4] kstrtox: introduce a safer version of memparse()

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: <d321160b-b895-4049-8ac6-4ff6ce5df7d4@gmx.com>
Date: Thu, 4 Jan 2024 17:12:25 +1030
From: Qu Wenruo <quwenruo.btrfs@....com>
To: Randy Dunlap <rdunlap@...radead.org>, Qu Wenruo <wqu@...e.com>,
 linux-btrfs@...r.kernel.org, linux-kernel@...r.kernel.org,
 akpm@...ux-foundation.org, christophe.jaillet@...adoo.fr,
 andriy.shevchenko@...ux.intel.com, David.Laight@...LAB.COM, ddiss@...e.de,
 geert@...ux-m68k.org
Subject: Re: [PATCH v3 2/4] kstrtox: introduce a safer version of memparse()



On 2024/1/4 17:00, Randy Dunlap wrote:
> Hi,
>
[...]
>> + *		parameter.
>> + *
>> + * @suffixes:	The suffixes which should be parsed. Use logical ORed
>> + *		memparse_suffix enum to indicate the supported suffixes.
>> + *		The suffixes are case-insensive, all 2 ^ 10 based.
>
> 		                 case-insensitive
>
>> + *		Supported ones are "KMGPTE".
>> + *		NOTE: If one suffix out of the supported one is hit, it would
>
> 		                                         ones
>
>> + *		end the parse normally, with @retptr pointed to the unsupported
>> + *		suffix.
>
> Could you explain (or give an example) of "to the unsupported suffix"?
> This isn't clear IMO.

Oh, my bad, that sentence itself is not correct.

What I really want to say is:

  If one suffix (one of "KMGPTE") is hit but that suffix is not
  specified in the @suffxies parameter, it would end the parse normally,
  with @retptr pointed to the (unsupported) suffix.

The example would be the "68k " case in the ok cases in the next patch.
We have two different cases for the same "68k" string, with different
@suffixes and different results:

	"68k ", KMGPTE -> 68 * 1024, @retptr at " ".
	"68k ", M -> 68, @retptr at 'k'.

I don't have a better expression here unfortunately, maybe the special
case is not even worthy explaining?

>
>> + *
>> + * @res:	Where to write the result.
>> + *
>> + * @retptr:	(output) Optional pointer to the next char after parse completes.
>> + *
>> + * Return 0 if any valid numberic string can be parsed, and @retptr updated.
>> + * Return -INVALID if no valid number string can be found.
>> + * Return -ERANGE if the number overflows.
>> + * For minus return values, @retptr would not be updated.
>
>   * Returns:
>   * * %0 if any valid numeric string can be parsed, and @retptr is updated.
>   * * %-EINVAL if no valid number string can be found.
>   * * %-ERANGE if the number overflows.
>   * * For negative return values, @retptr is not updated.
>
>
> For *ALL* of the comments, I request/suggest that you change the "would be" or
> "would not be" to "is" or "is not" or whatever present tense words make the
> most sense.

No problem.

Thanks,
Qu

>
>
>> + */
>> +noinline int memparse_safe(const char *s, enum memparse_suffix suffixes,
>> +			   unsigned long long *res, char **retptr)
>> +{
>> +	unsigned long long value;
>> +	unsigned int rv;
>> +	int shift = 0;
>> +	int base = 0;
>> +
>> +	s = _parse_integer_fixup_radix(s, &base);
>> +	rv = _parse_integer(s, base, &value);
>> +	if (rv & KSTRTOX_OVERFLOW)
>> +		return -ERANGE;
>> +	if (rv == 0)
>> +		return -EINVAL;
>> +
>> +	s += rv;
>> +	switch (*s) {
>> +	case 'K':
>> +	case 'k':
>> +		if (!(suffixes & MEMPARSE_SUFFIX_K))
>> +			break;
>> +		shift = 10;
>> +		break;
>> +	case 'M':
>> +	case 'm':
>> +		if (!(suffixes & MEMPARSE_SUFFIX_M))
>> +			break;
>> +		shift = 20;
>> +		break;
>> +	case 'G':
>> +	case 'g':
>> +		if (!(suffixes & MEMPARSE_SUFFIX_G))
>> +			break;
>> +		shift = 30;
>> +		break;
>> +	case 'T':
>> +	case 't':
>> +		if (!(suffixes & MEMPARSE_SUFFIX_T))
>> +			break;
>> +		shift = 40;
>> +		break;
>> +	case 'P':
>> +	case 'p':
>> +		if (!(suffixes & MEMPARSE_SUFFIX_P))
>> +			break;
>> +		shift = 50;
>> +		break;
>> +	case 'E':
>> +	case 'e':
>> +		if (!(suffixes & MEMPARSE_SUFFIX_E))
>> +			break;
>> +		shift = 60;
>> +		break;
>> +	}
>> +	if (shift) {
>> +		s++;
>> +		if (value >> (64 - shift))
>> +			return -ERANGE;
>> +		value <<= shift;
>> +	}
>> +	*res = value;
>> +	if (retptr)
>> +		*retptr = (char *)s;
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(memparse_safe);
>> +
>>   /**
>>    * kstrtoull - convert a string to an unsigned long long
>>    * @s: The start of the string. The string must be null-terminated, and may also
>
> Thanks.