netdev - Re: [PATCH net] udp: Pass 2 bytes of data with UDP

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANn89iL2PCnC=6dOrozW0309W==tWcKpj2iwZgZAD_s0amvzLA@mail.gmail.com>
Date:   Tue, 31 Jan 2023 18:57:03 +0100
From:   Eric Dumazet <edumazet@...gle.com>
To:     Jakub Sitnicki <jakub@...udflare.com>
Cc:     netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, kernel-team@...udflare.com
Subject: Re: [PATCH net] udp: Pass 2 bytes of data with UDP_GRO cmsg to user-space

On Tue, Jan 31, 2023 at 6:46 PM Jakub Sitnicki <jakub@...udflare.com> wrote:
>
> While UDP_GRO cmsg interface lacks documentation, the selftests added in
> commit 3327a9c46352 ("selftests: add functionals test for UDP GRO") suggest
> that the user-space should allocate CMSG_SPACE for an u16 value and
> interpret the returned bytes as such:
>
> static int recv_msg(int fd, char *buf, int len, int *gso_size)
> {
>         char control[CMSG_SPACE(sizeof(uint16_t))] = {0};
>         ...
>                         if (cmsg->cmsg_level == SOL_UDP
>                             && cmsg->cmsg_type == UDP_GRO) {
>                                 gsosizeptr = (uint16_t *) CMSG_DATA(cmsg);
>                                 *gso_size = *gsosizeptr;
>                                 break;
>                         }
>         ...
> }
>
> Today user-space will receive 4 bytes of data with an UDP_GRO cmsg, because
> the kernel packs an int into the cmsg data, as we can confirm with strace:
>
>   recvmsg(8, {msg_name=...,
>               msg_iov=[{iov_base="\0\0..."..., iov_len=96000}],
>               msg_iovlen=1,
>               msg_control=[{cmsg_len=20,         <-- sizeof(cmsghdr) + 4
>                             cmsg_level=SOL_UDP,
>                             cmsg_type=0x68}],    <-- UDP_GRO
>                             msg_controllen=24,
>                             msg_flags=0}, 0) = 11200
>
> This means that either UDP_GRO selftests are broken on big endian, or this
> is a programming error. Assume the latter and pass only the needed 2 bytes
> of data with the cmsg.
>
> Fixing it like that has an added advantage that the cmsg becomes compatible
> with what is expected by UDP_SEGMENT cmsg. It becomes possible to reuse the
> cmsg when GSO packets are received on one socket and sent out of another.
>
> Fixes: bcd1665e3569 ("udp: add support for UDP_GRO cmsg")
> Signed-off-by: Jakub Sitnicki <jakub@...udflare.com>
> ---
>  include/linux/udp.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/udp.h b/include/linux/udp.h
> index a2892e151644..44bb8d699248 100644
> --- a/include/linux/udp.h
> +++ b/include/linux/udp.h
> @@ -125,7 +125,7 @@ static inline bool udp_get_no_check6_rx(struct sock *sk)
>  static inline void udp_cmsg_recv(struct msghdr *msg, struct sock *sk,
>                                  struct sk_buff *skb)
>  {
> -       int gso_size;
> +       __u16 gso_size;
>
>         if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4) {
>                 gso_size = skb_shinfo(skb)->gso_size;
> --
> 2.39.1
>

This would break some applications.

I think the test can be fixed instead, this seems less risky.