[PATCH 1/7] xyz-modem: Fix crash after cancelling transfer

[Heinrich Schuchardt]

On 03.08.21 16:28, Pali Rohár wrote:
> Variable xyz.len is set to -1 on error. At the end xyzModem_stream_read()
> function calls memcpy() with length from variable xyz.len. If this variable
> is set to -1 then value passed to memcpy is casted to unsigned value, which
> means to copy whole address space. Which then cause U-Boot crash. E.g. on
> arm64 it cause CPU crash: "Synchronous Abort" handler, esr 0x96000006
> 
> Fix this issue by checking that value stored in xyz.len is valid prior
> trying to use it.
> 
> Signed-off-by: Pali Rohár <pali at kernel.org>

The changes below seem reasonable.

I would prefer if you could document the elements of struct xyz first. 
Just add Sphinx style comments to the structure and indicate for element 
len that a negative value signals an error.

Cf.
https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#structure-union-and-enumeration-documentation

Acked-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>

> ---
>   common/xyzModem.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/common/xyzModem.c b/common/xyzModem.c
> index fc3459ebbafe..b1b72aae0baf 100644
> --- a/common/xyzModem.c
> +++ b/common/xyzModem.c
> @@ -494,7 +494,7 @@ xyzModem_stream_read (char *buf, int size, int *err)
>     total = 0;
>     stat = xyzModem_cancel;
>     /* Try and get 'size' bytes into the buffer */
> -  while (!xyz.at_eof && (size > 0))
> +  while (!xyz.at_eof && xyz.len >= 0 && (size > 0))
>       {
>         if (xyz.len == 0)
>   	{
> @@ -587,7 +587,7 @@ xyzModem_stream_read (char *buf, int size, int *err)
>   	    }
>   	}
>         /* Don't "read" data from the EOF protocol package */
> -      if (!xyz.at_eof)
> +      if (!xyz.at_eof && xyz.len > 0)
>   	{
>   	  len = xyz.len;
>   	  if (size < len)
>