Re: [PATCH v2 net] ptp: clockmatrix: bug fix for idtcm_strverscmp

[Jakub Kicinski]

On Wed, 18 Nov 2020 22:50:24 -0500 min.li.xe@xxxxxxxxxxx wrote:
> From: Min Li <min.li.xe@xxxxxxxxxxx>
> 
> Feed kstrtou8 with NULL terminated string.
> 
> Changes since v1:
> -Only strcpy 15 characters to leave 1 space for '\0'
> 
> Signed-off-by: Min Li <min.li.xe@xxxxxxxxxxx>

> -static int idtcm_strverscmp(const char *ver1, const char *ver2)
> +static int idtcm_strverscmp(const char *version1, const char *version2)
>  {
>  	u8 num1;
>  	u8 num2;
>  	int result = 0;
> +	char ver1[16];
> +	char ver2[16];
> +	char *cur1;
> +	char *cur2;
> +	char *next1;
> +	char *next2;
> +
> +	strncpy(ver1, version1, 15);
> +	strncpy(ver2, version2, 15);
> +	cur1 = ver1;
> +	cur2 = ver2;

Now there is no guarantee that the buffer is null terminated.

You need to use strscpy(ver... 16) or add ver[15] = '\0';

>  	/* loop through each level of the version string */
>  	while (result == 0) {
> +		next1 = strchr(cur1, '.');
> +		next2 = strchr(cur2, '.');
> +
> +		/* kstrtou8 could fail for dot */
> +		if (next1) {
> +			*next1 = '\0';
> +			next1++;
> +		}
> +
> +		if (next2) {
> +			*next2 = '\0';
> +			next2++;
> +		}
> +
>  		/* extract leading version numbers */
> -		if (kstrtou8(ver1, 10, &num1) < 0)
> +		if (kstrtou8(cur1, 10, &num1) < 0)
>  			return -1;
>  
> -		if (kstrtou8(ver2, 10, &num2) < 0)
> +		if (kstrtou8(cur2, 10, &num2) < 0)
>  			return -1;
>  
>  		/* if numbers differ, then set the result */
> -		if (num1 < num2)
> +		if (num1 < num2) {
>  			result = -1;

Why do you set the result to something instead of just returning that
value? The kstrtou8() checks above just return value directly...

If you use a return you can save yourself all the else branches.

> -		else if (num1 > num2)
> +		} else if (num1 > num2) {
>  			result = 1;
> -		else {
> +		} else {
>  			/* if numbers are the same, go to next level */
> -			ver1 = strchr(ver1, '.');
> -			ver2 = strchr(ver2, '.');
> -			if (!ver1 && !ver2)
> +			if (!next1 && !next2)
>  				break;
> -			else if (!ver1)
> +			else if (!next1) {
>  				result = -1;
> -			else if (!ver2)
> +			} else if (!next2) {
>  				result = 1;
> -			else {
> -				ver1++;
> -				ver2++;
> +			} else {
> +				cur1 = next1;
> +				cur2 = next2;
>  			}
>  		}
>  	}
> +
>  	return result;
>  }
>