netdev - Re: [PATCH v2 net-next 3/9] openvswitch: Simplify labels length logic.

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:   Wed, 8 Feb 2017 14:47:02 -0800
From:   Joe Stringer <joe@....org>
To:     Jarno Rajahalme <jarno@....org>
Cc:     netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH v2 net-next 3/9] openvswitch: Simplify labels length logic.

On 8 February 2017 at 11:32, Jarno Rajahalme <jarno@....org> wrote:
> Since 23014011ba42 ("netfilter: conntrack: support a fixed size of 128
> distinct labels"), the size of conntrack labels extension has fixed to
> 128 bits, so we do not need to check for labels sizes shorter than 128
> at run-time.  This patch simplifies labels length logic accordingly,
> but allows the conntrack labels size to be increased in the future
> without breaking the build.  In the event of conntrack labels
> increasing in size OVS would still be able to deal with the 128 first
> label bits.
>
> Suggested-by: Joe Stringer <joe@....org>
> Signed-off-by: Jarno Rajahalme <jarno@....org>
> ---
>  net/openvswitch/conntrack.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> index 6730f09..a07e5cd 100644
> --- a/net/openvswitch/conntrack.c
> +++ b/net/openvswitch/conntrack.c
> @@ -129,22 +129,22 @@ static u32 ovs_ct_get_mark(const struct nf_conn *ct)
>  #endif
>  }
>
> +/* Guard against conntrack labels max size shrinking below 128 bits. */
> +#if NF_CT_LABELS_MAX_SIZE < 16
> +#error NF_CT_LABELS_MAX_SIZE must be at least 16 bytes
> +#endif
> +
>  static void ovs_ct_get_labels(const struct nf_conn *ct,
>                               struct ovs_key_ct_labels *labels)
>  {
>         struct nf_conn_labels *cl = ct ? nf_ct_labels_find(ct) : NULL;
>
> -       if (cl) {
> -               size_t len = sizeof(cl->bits);
> -
> -               if (len > OVS_CT_LABELS_LEN)
> -                       len = OVS_CT_LABELS_LEN;
> -               else if (len < OVS_CT_LABELS_LEN)
> -                       memset(labels, 0, OVS_CT_LABELS_LEN);
> -               memcpy(labels, cl->bits, len);
> -       } else {
> +       if (cl)
> +               memcpy(labels, cl->bits,
> +                      sizeof(cl->bits) > OVS_CT_LABELS_LEN
> +                      ? OVS_CT_LABELS_LEN : sizeof(cl->bits));

Is this to be defensive? If sizeof(cl->bits) is larger than
OVS_CT_LABELS_LEN, we'll use OVS_CT_LABELS_LEN; if it's equal, we'll
still use OVS_CT_LABELS_LEN; if it's less, the precompiler will fail
above. Why not memcpy(.., OVS_CT_LABELS_LEN) ?