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
Reviewed-by: Willem de Bruijn <willemb@xxxxxxxxxx>
I noticed that this is still open in patchwork, no need to resend.
