netdev - Re: [PATCH net] selftests/net: Interpret 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]

Date:   Thu, 16 Feb 2023 14:03:50 +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] selftests/net: Interpret UDP_GRO cmsg data as an int value

On Thu, Feb 16, 2023 at 1:43 PM Jakub Sitnicki <jakub@...udflare.com> wrote:
>
> Data passed to user-space with a (SOL_UDP, UDP_GRO) cmsg carries an
> int (see udp_cmsg_recv), not a u16 value, as strace confirms:
>
>   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
>
> Interpreting the data as an u16 value won't work on big-endian platforms.
> Since it is too late to back out of this API decision [1], fix the test.
>
> [1]: https://lore.kernel.org/netdev/20230131174601.203127-1-jakub@cloudflare.com/
>
> Fixes: 3327a9c46352 ("selftests: add functionals test for UDP GRO")
> Suggested-by: Eric Dumazet <edumazet@...gle.com>
> Signed-off-by: Jakub Sitnicki <jakub@...udflare.com>
> ---
>  tools/testing/selftests/net/udpgso_bench_rx.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/tools/testing/selftests/net/udpgso_bench_rx.c b/tools/testing/selftests/net/udpgso_bench_rx.c
> index 4058c7451e70..f35a924d4a30 100644
> --- a/tools/testing/selftests/net/udpgso_bench_rx.c
> +++ b/tools/testing/selftests/net/udpgso_bench_rx.c
> @@ -214,11 +214,10 @@ static void do_verify_udp(const char *data, int len)
>
>  static int recv_msg(int fd, char *buf, int len, int *gso_size)
>  {
> -       char control[CMSG_SPACE(sizeof(uint16_t))] = {0};
> +       char control[CMSG_SPACE(sizeof(int))] = {0};
>         struct msghdr msg = {0};
>         struct iovec iov = {0};
>         struct cmsghdr *cmsg;
> -       uint16_t *gsosizeptr;
>         int ret;
>
>         iov.iov_base = buf;
> @@ -237,8 +236,7 @@ static int recv_msg(int fd, char *buf, int len, int *gso_size)
>                      cmsg = CMSG_NXTHDR(&msg, cmsg)) {
>                         if (cmsg->cmsg_level == SOL_UDP
>                             && cmsg->cmsg_type == UDP_GRO) {
> -                               gsosizeptr = (uint16_t *) CMSG_DATA(cmsg);
> -                               *gso_size = *gsosizeptr;
> +                               *gso_size = *(int *)CMSG_DATA(cmsg);
>                                 break;
>                         }
>                 }
> --
> 2.39.1
>

Yes, it seems user space often do not bother verifying cmsg->cmsg_len
against CMSG_LEN(sizeof(expected type))
(such sanity tests could be added to many of our selftests)

Reviewed-by: Eric Dumazet <edumazet@...gle.com>

Thanks.