linux-kernel - Re: [PATCH] kernel/taskstats: fix wrong nla type for {cgroup,task}stats policy

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: <CALOAHbCwUYGK_cbdBmz5Htr6a+KuVsTQT8q=wBw73CemGhuuFQ@mail.gmail.com>
Date:   Fri, 27 Mar 2020 08:43:28 +0800
From:   Yafang Shao <laoar.shao@...il.com>
To:     Johannes Berg <johannes@...solutions.net>
Cc:     Andrew Morton <akpm@...ux-foundation.org>, bsingharora@...il.com,
        LKML <linux-kernel@...r.kernel.org>, stable@...r.kernel.org,
        David Ahern <dsahern@...il.com>,
        "David S.Miller" <davem@...emloft.net>
Subject: Re: [PATCH] kernel/taskstats: fix wrong nla type for
 {cgroup,task}stats policy

On Fri, Mar 27, 2020 at 4:18 AM Johannes Berg <johannes@...solutions.net> wrote:
>
> On Thu, 2020-03-26 at 13:08 -0700, Andrew Morton wrote:
>
> > > After our server is upgraded to a newer kernel, we found that it
> > > continuesly print a warning in the kernel message. The warning is,
> > > [832984.946322] netlink: 'irmas.lc': attribute type 1 has an invalid length.
> > >
> > > irmas.lc is one of our container monitor daemons, and it will use
> > > CGROUPSTATS_CMD_GET to get the cgroupstats, that is similar with
> > > tools/accounting/getdelays.c. We can also produce this warning with
> > > getdelays. For example, after running bellow command
> > >     $ ./getdelays -C /sys/fs/cgroup/memory
> > > then you can find a warning in dmesg,
> > > [61607.229318] netlink: 'getdelays': attribute type 1 has an invalid length.
> > >
> > > This warning is introduced in commit 6e237d099fac ("netlink: Relax attr
> > > validation for fixed length types"), which is used to check whether
> > > attributes using types NLA_U* and NLA_S* have an exact length.
> > >
> > > Regarding this issue, the root cause is cgroupstats_cmd_get_policy defines
> > > a wrong type as NLA_U32, while it should be NLA_NESTED an its minimal
> > > length is NLA_HDRLEN. That is similar to taskstats_cmd_get_policy.
> > >
> > > As this behavior change really breaks our application, we'd better
> > > cc stable as well.
>
> Can you explain how it breaks the application? I mean, it's really only
> printing a message to the kernel log in this case? At least that's what
> you're describing.
>
> I think you may be describing it wrong, because an NLA_NESTED is allowed
> to be *empty* (but otherwise must have at least 4 bytes just like an
> NLA_U32).
>
> That said, I'm not even sure I agree that this fix is right? See below.
>
> > Is it correct to say that although the code has always been incorrect,
> > but only kernels after 6e237d099fac need this change?  If so, I'll add
> > Fixes:6e237d099fac to guide the -stable backporting.
>
> That doesn't really seem right - 6e237d099fac *relaxed* the checks. If
> anything then it ought to point to 28033ae4e0f5 which may have actually
> returned an error; but again, need to understand better what really the
> issue is.
>
> > > diff --git a/kernel/taskstats.c b/kernel/taskstats.c
> > > index e2ac0e3..b90a520 100644
> > > --- a/kernel/taskstats.c
> > > +++ b/kernel/taskstats.c
> > > @@ -35,8 +35,8 @@
> > >  static struct genl_family family;
> > >
> > >  static const struct nla_policy taskstats_cmd_get_policy[TASKSTATS_CMD_ATTR_MAX+1] = {
> > > -   [TASKSTATS_CMD_ATTR_PID]  = { .type = NLA_U32 },
> > > -   [TASKSTATS_CMD_ATTR_TGID] = { .type = NLA_U32 },
> > > +   [TASKSTATS_CMD_ATTR_PID]  = { .type = NLA_NESTED },
> > > +   [TASKSTATS_CMD_ATTR_TGID] = { .type = NLA_NESTED },
>
>
> I'm not sure where this is coming from - the kernel evidently uses them
> as nested attributes in *outgoing* data (see mk_reply()), but as NLA_U32
> in *incoming* data, (see cmd_attr_pid() and cmd_attr_tgid()).
>

Thanks for the explanation.
The nested attributes is only used in *outgoing* data, rather than the
'incoming' data.

> I would generally recommend not doing such a thing as it's messy, but we
> do have quite a few such instances cases. In all those cases must the
> policy list the incoming policy since that's what the kernel uses to
> validate the attributes.
>
> IOW, this part of the change seems _wrong_.
>
>
> > >   * Make sure they are always aligned.
> > >   */
> > >  static const struct nla_policy cgroupstats_cmd_get_policy[TASKSTATS_CMD_ATTR_MAX+1] = {
> > > -   [CGROUPSTATS_CMD_ATTR_FD] = { .type = NLA_U32 },
> > > +   [CGROUPSTATS_CMD_ATTR_FD] = { .type = NLA_NESTED },
> > >  };
>
> And same here, actually.
>
> johannes
>


Thanks
Yafang