Re: [edk2-devel] [PATCH] CryptoPkg/SysCall: Cast variables from 4 to 8-byte size

["Laszlo Ersek" <lersek@redhat.com>]

On 12/05/19 09:46, Zhang, Shenglei wrote:
> tp, pch, digits and xdigits are both 4-byte-size, but not
> cast to 8-byte-size when operated with 8-byte-size variables.
> This is a issue reported by my local static tool.
> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
> Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
> ---
>  CryptoPkg/Library/BaseCryptLib/SysCall/inet_pton.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/CryptoPkg/Library/BaseCryptLib/SysCall/inet_pton.c b/CryptoPkg/Library/BaseCryptLib/SysCall/inet_pton.c
> index 32e1ab8690e6..ad392b18ca66 100644
> --- a/CryptoPkg/Library/BaseCryptLib/SysCall/inet_pton.c
> +++ b/CryptoPkg/Library/BaseCryptLib/SysCall/inet_pton.c
> @@ -132,7 +132,7 @@ inet_pton4(
>  		const char *pch;
>  
>  		if ((pch = strchr(digits, ch)) != NULL) {
> -			u_int new = *tp * 10 + (u_int)(pch - digits);
> 			u_int new = (u_int)(*tp) * 10 + (u_int)pch - (u_int)digits;
>  
>  			if (new > 255)
>  				return (0);
> @@ -200,7 +200,7 @@ inet_pton6(
>  			pch = strchr((xdigits = xdigits_u), ch);
>  		if (pch != NULL) {
>  			val <<= 4;
> -			val |= (pch - xdigits);
> 			val |= (u_int)pch - (u_int)xdigits;
>  			if (val > 0xffff)
>  				return (0);
>  			saw_xdigit = 1;
> 

(1) This email does not look like a real patch for edk2.

It removes some lines, yes, but the expressions that (I think?) it
proposes, as new lines, are not marked with "+". Instead, those are
displayed as existent code ("context").

But the file does not contain lines such as

u_int new = (u_int)(*tp) * 10 + (u_int)pch - (u_int)digits;

and

val |= (u_int)pch - (u_int)xdigits;

I don't understand how this patch was generated. Maybe you added the new
lines in a separate patch before, and removed the old lines in a new
patch, and posted only the last (= partial change) patch.


(2) We can spell out the current edk2 types in the definition (and
initialization) of "new" below

  u_int new = *tp * 10 + (u_int)(pch - digits);

as follows:

  UINTN new = (UINT8)*tp * (INT32)10 +
              (UINTN)((CONST CHAR8 *)pch - (CONST CHAR8 *)digits);

I don't have the slightest idea why a static analyzer whines about this.

- the subtraction of the pointers is valid ("pch" points into "digits"),
- the result of the subtraction is a ptrdiff_t,
- ptrdiff_t can be safely converted to UINTN.

Furthermore,

- In the multiplication, UINT8 is promoted to INT32, and the value is
non-negative, and does not exceed 255
- the multiplication is performed in INT32, and could never overflow
(because 255 * 10 = 2550 is representable in INT32),
- UINTN is either UINT32 or UINT64; for the addition, the INT32 product
is converted to UINTN,
- the addition is performed in UINTN,
- the result is stored to a UINTN.

I think the static analyzer warning is wrong.

I'd rather we supressed any such warning in some other way, for example
in the configuration of your static analyzer. Unless we find a critical
bug or evidently undefined behavior in this code, I'd like to keep it
intact (matching its origin from edk2-libc).


(3) In the assignment expression statement

  val |= (pch - xdigits);

the subtraction uses (CONST CHAR8 *) operands. It is a valid subtraction
("pch" points into the array pointed-to by "xdigits"). The result is of
type "ptrdiff_t" (per C spec), and has non-negative value.

"val" is UINTN.

Therefore we can spell out the above compound assignment as the below
simple assignment:

  val = (UINTN)val | (ptrdiff_t)(pch - xdigits);

whicn means, in practice:

- on 32-bit:

  val = (unsigned)val | (long)(pch - xdigits);

- on 64-bit:

  val = (unsigned long long)val | (long)(pch - xdigits);

In the 64-bit case, the (long) difference is converted to (unsigned long
long), per usual arithmetic conversions.

In the 32-bit case, both operands are converted to (unsigned long)
(because "long", on our platforms, cannot represent all values of
"unsigned int"). The result, of type "unsigned long", is assigned to
"val", of type "unsigned int". However, this cannot cause a loss of
information, because all values of the non-negative (long) difference
fit in "unsigned int".

So I don't think this code needs to be changed either. (Although I agree
it's not too easy to reason about.)


(4) Even if the patch was well-formed, and even if I agreed with
modifying these expressions to something else, replacing the pointer
subtractions with integer subtractions, as in:

  (u_int)pch - (u_int)digits

and

  (u_int)pch - (u_int)xdigits

makes things actually *harder* to understand. Semantically, we don't
want to calculate the difference between the numerical representations
of these pointers; instead, we want the offsets of the found elements
into the containing arrays.

--*--

I think I could agree to one change:

--- a/CryptoPkg/Library/BaseCryptLib/SysCall/inet_pton.c
+++ b/CryptoPkg/Library/BaseCryptLib/SysCall/inet_pton.c
@@ -200,7 +200,7 @@ inet_pton6(
                        pch = strchr((xdigits = xdigits_u), ch);
                if (pch != NULL) {
                        val <<= 4;
-                       val |= (pch - xdigits);
+                       val |= (u_int)(pch - xdigits);
                        if (val > 0xffff)
                                return (0);
                        saw_xdigit = 1;

For the following reasons:

- it would be stylistically consistent with the rest of this file;

- it would make the pointer subtraction in inet_pton6() consistent with
the pointer subtraction in inet_pton4(), where we have

  (u_int)(pch - digits)

already;

- although not technically necessary, this change would significantly
simplify the reasoning about the expression.

Thanks
Laszlo