netdev - Re: [PATCH v1 net-next 02/14] net: Add a new socket option for a future transmit time.

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, 28 Jun 2018 10:26:03 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Jesus Sanchez-Palencia <jesus.sanchez-palencia@...el.com>
Cc:     Network Development <netdev@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        jan.altenberg@...utronix.de,
        Vinicius Gomes <vinicius.gomes@...el.com>,
        kurt.kanzenbach@...utronix.de, Henrik Austad <henrik@...tad.us>,
        Richard Cochran <richardcochran@...il.com>,
        Levi Pearson <levi.pearson@...man.com>,
        ilias.apalodimas@...aro.org, ivan.khoronzhuk@...aro.org,
        Miroslav Lichvar <mlichvar@...hat.com>,
        Willem de Bruijn <willemb@...gle.com>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jiří Pírko <jiri@...nulli.us>,
        Richard Cochran <rcochran@...utronix.de>
Subject: Re: [PATCH v1 net-next 02/14] net: Add a new socket option for a
 future transmit time.

On Wed, Jun 27, 2018 at 6:08 PM Jesus Sanchez-Palencia
<jesus.sanchez-palencia@...el.com> wrote:
>
> From: Richard Cochran <rcochran@...utronix.de>
>
> This patch introduces SO_TXTIME. User space enables this option in
> order to pass a desired future transmit time in a CMSG when calling
> sendmsg(2). The argument to this socket option is a 6-bytes long struct
> defined as:
>
> struct sock_txtime {
>         clockid_t       clockid;
>         u16             flags;
> };

clockid_t is __kernel_clockid_t is int is a variable length field.
Please use fixed
length fields. Also, as MAX_CLOCKS is 16, only 4 bits are needed. A single u16
is probably sufficient as cmsg argument. To future proof, a u32 will
allow for more
than 4 flags. But in struct sock, 16 bits should be sufficient to
encode both clock id
and flags.

> Note that two new fields were added to struct sock by filling a 4-bytes
> hole found in the struct. For that reason, neither the struct size or
> number of cachelines were altered.
>
> Signed-off-by: Richard Cochran <rcochran@...utronix.de>
> Signed-off-by: Jesus Sanchez-Palencia <jesus.sanchez-palencia@...el.com>
> ---

> +#include <asm/unaligned.h>
>  #include <linux/capability.h>
>  #include <linux/errno.h>
>  #include <linux/errqueue.h>
> @@ -697,6 +698,7 @@ EXPORT_SYMBOL(sk_mc_loop);
>  int sock_setsockopt(struct socket *sock, int level, int optname,
>                     char __user *optval, unsigned int optlen)
>  {
> +       struct sock_txtime sk_txtime;
>         struct sock *sk = sock->sk;
>         int val;
>         int valbool;
> @@ -1070,6 +1072,22 @@ int sock_setsockopt(struct socket *sock, int level, int optname,
>                 }
>                 break;
>
> +       case SO_TXTIME:
> +               if (!ns_capable(sock_net(sk)->user_ns, CAP_NET_ADMIN)) {
> +                       ret = -EPERM;
> +               } else if (optlen != sizeof(struct sock_txtime)) {
> +                       ret = -EINVAL;
> +               } else if (copy_from_user(&sk_txtime, optval,
> +                          sizeof(struct sock_txtime))) {
> +                       ret = -EFAULT;
> +                       sock_valbool_flag(sk, SOCK_TXTIME, false);

Why change sk state on failure? This is not customary.

> +               } else {
> +                       sock_valbool_flag(sk, SOCK_TXTIME, true);
> +                       sk->sk_clockid = sk_txtime.clockid;
> +                       sk->sk_txtime_flags = sk_txtime.flags;

Validate input and fail on undefined flags.

> @@ -2137,6 +2162,13 @@ int __sock_cmsg_send(struct sock *sk, struct msghdr *msg, struct cmsghdr *cmsg,
>                 sockc->tsflags &= ~SOF_TIMESTAMPING_TX_RECORD_MASK;
>                 sockc->tsflags |= tsflags;
>                 break;
> +       case SCM_TXTIME:
> +               if (!sock_flag(sk, SOCK_TXTIME))
> +                       return -EINVAL;

Note that on lockfree datapaths like udp this test can race with the
setsockopt above.
It seems harmless here.

> +               if (cmsg->cmsg_len != CMSG_LEN(sizeof(u64)))
> +                       return -EINVAL;
> +               sockc->transmit_time = get_unaligned((u64 *)CMSG_DATA(cmsg));
> +               break;
>         /* SCM_RIGHTS and SCM_CREDENTIALS are semantically in SOL_UNIX. */
>         case SCM_RIGHTS:
>         case SCM_CREDENTIALS:
> --
> 2.17.1
>