Re: [PATCH XENSTORE v1 06/10] xenstored: handle port reads correctly

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 26/02/2021 14:41, Norbert Manthey wrote:
> The read value could be larger than a signed 32bit integer. As -1 is
> used as error value, we should not rely on using the full 32 bits.
> Hence, when reading the port number, we should make sure we only return
> valid values.
>
> This change sanity checks the input.
> The issue is that the value for the port is
>  1. transmitted as a string, with a fixed amount of digits.
>  2. Next, this string is parsed by a function that can deal with strings
>     representing 64bit integers
>  3. A 64bit integer is returned, and will be truncated to it's lower
>     32bits, resulting in a wrong port number (in case the sender of the
>     string decides to craft a suitable 64bit value).
>
> The value is typically provided by the kernel, which has this value hard
> coded in the proper range. As we use the function strtoul, non-digit
> character are considered as end of the input, and hence do not require
> checking. Therefore, this change only covers the corner case to make
> sure we stay in the 32 bit range.
>
> This bug was discovered and resolved using Coverity Static Analysis
> Security Testing (SAST) by Synopsys, Inc.
>
> Signed-off-by: Norbert Manthey <nmanthey@xxxxxxxxx>
> Reviewed-by: Thomas Friebel <friebelt@xxxxxxxxx>
> Reviewed-by: Julien Grall <jgrall@xxxxxxxxxxxx>

Port numbers are currently limited at 2^17, with easy extension to 2^29
(iirc), but the entire event channel infrastructure would have to
undergo another redesign to get beyond that.

I think we can reasonably make an ABI statement saying that a port
number will never exceed 2^31.  This is already pseudo-encoded in the
evtchn_port_or_error_t mouthful.

~Andrew