linux-kernel - Re: [PATCH] cgroup: reorder flexible array members of struct cgroup

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] [day] [month] [year] [list]

Message-ID: <20171026143222.GC59538@devbig577.frc2.facebook.com>
Date:   Thu, 26 Oct 2017 07:32:22 -0700
From:   Tejun Heo <tj@...nel.org>
To:     Matthias Kaehlcke <mka@...omium.org>
Cc:     Nick Desaulniers <nick.desaulniers@...il.com>,
        Li Zefan <lizefan@...wei.com>,
        Johannes Weiner <hannes@...xchg.org>, cgroups@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Michael Davidson <md@...gle.com>,
        Greg Hackmann <ghackmann@...gle.com>, android-llvm@...gle.com
Subject: Re: [PATCH] cgroup: reorder flexible array members of struct
 cgroup_root

Hello,

On Wed, Oct 25, 2017 at 02:54:23PM -0700, Matthias Kaehlcke wrote:
> From your earlier comment I understand that there is no problem in
> this case because we know that cgroup_root->cgrp will always be
> empty.
> 
> However in other instances the warning could point out actual errors
> in the code, so I think it is good to have this warning generally
> enabled. If cgroup_root was defined in a .c file we could consider to
> disable the warning locally, but since the definition is in a header
> that is widely included (indirectly through linux/cgroup.h and
> net/sock.h) this doesn't seem to be an option.
> 
> Is there a good reason for the current position of cgrp within
> cgroup_root? If there are no drawbacks in moving it to the end of
> the struct I think Nick's patch is a reasonable solution.

This all sounds really bogus to me.  Let's say we have something like
the following.

  struct flex_struct {
  	int array[];
  };

And the following two usages.

1.

  struct flex_struct *fs =
	kmalloc(sizeof(struct flex_struct) + N * sizeof(int));

2.

  struct enclosing_struct es {
	struct flex_struct fs;
	int fs_array_storage[N];
  };

  struct enclosing_struct *es =
	kmalloc(sizeof(struct enclosing_struct));

So, you're saying #1 is okay but #2 is not, which is just silly.  The
compiler can't warn correctly about flex array members whether they're
embedded or not.  Nothing prevents somebody accessing beyond N in #1
either.

This effort seems really pointless to me.  Let's please not waste any
more bandwidth on this.

Thanks.

-- 
tejun