Re: [PATCH net v2 2/2] net/ipv4: reject appends with negative fragment copy

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

tencentos_secure(TencentOS安全公共邮箱) wrote:
> __ip_append_data() caps datalen at maxfraglen - fragheaderlen when more
> fragments are needed, and later computes
> 
> 	copy = datalen - transhdrlen - fraggap - pagedlen;
> 
> If transhdrlen + fraggap exceeds datalen, copy goes negative in the
> non-paged allocation branches, where pagedlen is zero, invalidating
> the offset/length accounting. In the paged allocation branch,
> pagedlen = datalen - transhdrlen - fraggap wraps unsigned instead.
> The subsequent copy calculation cancels to zero, so checking copy
> alone does not catch this invalid fragment geometry.
> 
> This is reachable independently of the xfrm fraggap bug fixed by the
> preceding patch. icmp_timestamp() passes a 20-byte transport header.
> With a small MTU and IPv4 options, the capped datalen can be smaller
> than that header even when fraggap is zero. The negative copy then
> propagates a negative offset into icmp_glue_bits(), and a timestamp
> reply with corrupted timestamp fields can be emitted.
> 
> For example, with MTU 68 and 40 bytes of IPv4 options, fragheaderlen
> is 60, maxfraglen is 68, datalen is capped at 8, and fraggap is zero,
> so the 20-byte transport header cannot fit.
> 
> Reject this geometry before allocation with -EINVAL, covering all three
> alloclen branches and matching the existing __ip6_append_data() guard.
> A negative copy cannot produce a valid append, so this does not reject a
> send that could otherwise construct a correct packet. All callers flush
> the cork on append error, and returning -EINVAL in mid-cork state already
> happens in this function (MSG_ZEROCOPY ubuf mismatch).
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org
> Signed-off-by: Anakin Zhang <tencentos_secure@tencent.com>

Reviewed-by: Willem de Bruijn <willemb@google.com>

> ---
> v2:
> - Document the independently reachable ICMP Timestamp corruption path.
> - Distinguish negative copy in non-paged allocation branches from
>   unsigned pagedlen wrap in the paged allocation branch.
> - Update the subject and Fixes tag accordingly.
> - Drop the unrelated xfrm Reported-by tag; no code changes.
> 
>  net/ipv4/ip_output.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
> index b84d56c..5db8b06 100644
> --- a/net/ipv4/ip_output.c
> +++ b/net/ipv4/ip_output.c
> @@ -1105,6 +1105,11 @@ alloc_new_skb:
>  			fraglen = datalen + fragheaderlen;
>  			pagedlen = 0;
>  
> +			if (unlikely(transhdrlen + fraggap > datalen)) {
> +				err = -EINVAL;

nit: EMSGSIZE might be better

> +				goto error;
> +			}
> +