RE: [PATCH net] udp: Fix __ip_append_data()'s handling of MSG_SPLICE_PAGES

[Willem de Bruijn]

David Howells wrote:
>     
> __ip_append_data() can get into an infinite loop when asked to splice into
> a partially-built UDP message that has more than the frag-limit data and up
> to the MTU limit.  Something like:
> 
>         pipe(pfd);
>         sfd = socket(AF_INET, SOCK_DGRAM, 0);
>         connect(sfd, ...);
>         send(sfd, buffer, 8161, MSG_CONFIRM|MSG_MORE);
>         write(pfd[1], buffer, 8);
>         splice(pfd[0], 0, sfd, 0, 0x4ffe0ul, 0);
> 
> where the amount of data given to send() is dependent on the MTU size (in
> this instance an interface with an MTU of 8192).
> 
> The problem is that the calculation of the amount to copy in
> __ip_append_data() goes negative in two places, and, in the second place,
> this gets subtracted from the length remaining, thereby increasing it.
> 
> This happens when pagedlen > 0 (which happens for MSG_ZEROCOPY and
> MSG_SPLICE_PAGES), because the terms in:
> 
>         copy = datalen - transhdrlen - fraggap - pagedlen;
> 
> then mostly cancel when pagedlen is substituted for, leaving just -fraggap.
> This causes:
> 
>         length -= copy + transhdrlen;
> 
> to increase the length to more than the amount of data in msg->msg_iter,
> which causes skb_splice_from_iter() to be unable to fill the request and it
> returns less than 'copied' - which means that length never gets to 0 and we
> never exit the loop.
> 
> Fix this by:
> 
>  (1) Insert a note about the dodgy calculation of 'copy'.
> 
>  (2) If MSG_SPLICE_PAGES, clear copy if it is negative from the above
>      equation, so that 'offset' isn't regressed and 'length' isn't
>      increased, which will mean that length and thus copy should match the
>      amount left in the iterator.
> 
>  (3) When handling MSG_SPLICE_PAGES, give a warning and return -EIO if
>      we're asked to splice more than is in the iterator.  It might be
>      better to not give the warning or even just give a 'short' write.
> 
> [!] Note that this ought to also affect MSG_ZEROCOPY, but MSG_ZEROCOPY
> avoids the problem by simply assuming that everything asked for got copied,
> not just the amount that was in the iterator.  This is a potential bug for
> the future.
> 
> Fixes: 7ac7c987850c ("udp: Convert udp_sendpage() to use MSG_SPLICE_PAGES")
> Reported-by: syzbot+f527b971b4bdc8e79f9e@xxxxxxxxxxxxxxxxxxxxxxxxx
> Link: https://lore.kernel.org/r/000000000000881d0606004541d1@xxxxxxxxxx/
> Signed-off-by: David Howells <dhowells@xxxxxxxxxx>
> cc: Willem de Bruijn <willemdebruijn.kernel@xxxxxxxxx>
> cc: "David S. Miller" <davem@xxxxxxxxxxxxx>
> cc: Eric Dumazet <edumazet@xxxxxxxxxx>
> cc: Jakub Kicinski <kuba@xxxxxxxxxx>
> cc: Paolo Abeni <pabeni@xxxxxxxxxx>
> cc: David Ahern <dsahern@xxxxxxxxxx>
> cc: Jens Axboe <axboe@xxxxxxxxx>
> cc: netdev@xxxxxxxxxxxxxxx

Thanks for limiting this to MSG_SPLICE_PAGES.

__ip6_append_data probably needs the same.

I see your point that the

  if (copy > 0) {
  } else {
    copy = 0;
  }

might apply to MSG_ZEROCOPY too. I'll take a look at that. For now
this is a clear fix to a specific MSG_SPLICE_PAGES commit.

copy is recomputed on each iteration in the loop. The only fields it
directly affects below this new line are offset and length. offset is
only used in copy paths: "offset into linear skb".

So this changes length, the number of bytes still to be written.

copy -= -fraggap definitely seems off. You point out that it even can
turn length negative?

The WARN_ON_ONCE, if it can be reached, will be user triggerable.
Usually for those cases and when there is a viable return with error
path, that is preferable. But if you prefer to taunt syzbot, ok. We
can always remove this later.

> ---
>  net/ipv4/ip_output.c |    9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
> index 6e70839257f7..91715603cf6e 100644
> --- a/net/ipv4/ip_output.c
> +++ b/net/ipv4/ip_output.c
> @@ -1158,10 +1158,15 @@ static int __ip_append_data(struct sock *sk,
>  			}
>  
>  			copy = datalen - transhdrlen - fraggap - pagedlen;
> +			/* [!] NOTE: copy will be negative if pagedlen>0
> +			 * because then the equation reduces to -fraggap.
> +			 */
>  			if (copy > 0 && getfrag(from, data + transhdrlen, offset, copy, fraggap, skb) < 0) {
>  				err = -EFAULT;
>  				kfree_skb(skb);
>  				goto error;
> +			} else if (flags & MSG_SPLICE_PAGES) {
> +				copy = 0;
>  			}
>  
>  			offset += copy;
> @@ -1209,6 +1214,10 @@ static int __ip_append_data(struct sock *sk,
>  		} else if (flags & MSG_SPLICE_PAGES) {
>  			struct msghdr *msg = from;
>  
> +			err = -EIO;
> +			if (WARN_ON_ONCE(copy > msg->msg_iter.count))
> +				goto error;
> +
>  			err = skb_splice_from_iter(skb, &msg->msg_iter, copy,
>  						   sk->sk_allocation);
>  			if (err < 0)
>