netdev - Re: [PATCH] openvswitch: Fix alignment of struct sw_flow

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, 5 Sep 2013 21:20:16 +0200
From:	Geert Uytterhoeven <geert@...ux-m68k.org>
To:	David Miller <davem@...emloft.net>
Cc:	Jesse Gross <jesse@...ira.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"dev@...nvswitch.org" <dev@...nvswitch.org>,
	Andy Zhou <azhou@...ira.com>, fengguan.wu@...el.com
Subject: Re: [PATCH] openvswitch: Fix alignment of struct sw_flow_key.

On Thu, Sep 5, 2013 at 8:40 PM, David Miller <davem@...emloft.net> wrote:
> When doing comparisons, this structure is being accessed as a byte
> array in 'long' sized chunks, not by its members. Therefore, the

Like ... an optimized memcmp() does? Which handles all (un)alignment
well?

>> To completely honest, I think the correct alignment should be
>> sizeof(long) because I know that 'long' is not always 8 bytes on all
>> architectures. However, you made the point before that this could
>> break the alignment of the 64-bit values on architectures where 'long'
>> is 32 bits wide, so 8 bytes is the generic solution.
>
> Look at net/core/flow.c:flow_key_compare().
>
> And then we annotate struct flowi with
>
> } __attribute__((__aligned__(BITS_PER_LONG/8)));
>
> Don't reinvent the wheel, either mimick how existing code does
> this kind of thing or provide a justification for doing it differently
> and update the existing cases to match and be consistent.

This may still break the alignment of 64-bit values on 32-bit architectures
where __alignof__(u64) == 8.

Now let's look at the comparison function:

static bool __cmp_key(const struct sw_flow_key *key1,
                const struct sw_flow_key *key2,  int key_start, int key_end)
{
        const long *cp1 = (long *)((u8 *)key1 + key_start);
        const long *cp2 = (long *)((u8 *)key2 + key_start);
        long diffs = 0;
        int i;

        for (i = key_start; i < key_end;  i += sizeof(long))
                diffs |= *cp1++ ^ *cp2++;

        return diffs == 0;
}

Why don't you abort the loop if a difference is found?
Or is this a security-related struct where you want to protect against
timing attacks?

Furthermore, as you compare the raw bytes, I hope you always
initialize all gaps in the struct to zero.
E.g. there's a 2-byte gap immediately after "ip", as the next member
is 32-bit (except op m68k, where the 32-bit member will be 2-byte aligned).

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html