linux-kernel - Re: [PATCH v4] selinux: lsm: fix a missing-check bug in selinux_sb_eat_lsm

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]

Message-ID: <CAHC9VhT+e2Z+4=5P0g4B4F1g0w2SkQjwUnhQkmu5V+HvuZi8Cg@mail.gmail.com>
Date:   Mon, 10 Jun 2019 16:20:28 -0400
From:   Paul Moore <paul@...l-moore.com>
To:     Ondrej Mosnacek <omosnace@...hat.com>,
        Gen Zhang <blackgod016574@...il.com>
Cc:     Stephen Smalley <sds@...ho.nsa.gov>,
        Eric Paris <eparis@...isplace.org>, selinux@...r.kernel.org,
        Linux kernel mailing list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4] selinux: lsm: fix a missing-check bug in
 selinux_sb_eat_lsm_o pts()

On Fri, Jun 7, 2019 at 4:41 AM Ondrej Mosnacek <omosnace@...hat.com> wrote:
>
> On Thu, Jun 6, 2019 at 10:55 AM Gen Zhang <blackgod016574@...il.com> wrote:
> > In selinux_sb_eat_lsm_opts(), 'arg' is allocated by kmemdup_nul(). It
> > returns NULL when fails. So 'arg' should be checked. And 'mnt_opts'
> > should be freed when error.
> >
> > Signed-off-by: Gen Zhang <blackgod016574@...il.com>
> > Fixes: 99dbbb593fe6 ("selinux: rewrite selinux_sb_eat_lsm_opts()")
>
> My comments about the subject and an empty line before label apply
> here as well, but Paul can fix both easily when applying ...

Since we've been discussing general best practices for submitting
patches in this thread (and the other related thread), I wanted to
(re)clarify my thoughts around maintainers fixing patches when merging
them upstream.

When in doubt, do not ever rely on the upstream maintainer fixing your
patch while merging it, and if problems do arise during review, it is
best to not ask the maintainer to fix them for you, but for you to fix
them instead (you are the patch author after all!).  Similarly, making
comments along the lines of "X can fix both easily when applying", is
also a bad thing to say when reviewing patches.  It's the patch
author's responsibility to fix the patch by address review comments,
not the maintainer.  I'll typically let you know if you don't need to
rework a patch(set).

That said, there are times when the maintainer will change the patch
during merging, most of which are due to resolving merge
conflicts/fuzz with changes already in the tree (that *is* the
maintainer's responsibility).  Speaking for myself, sometimes I will
also make some minor changes if the patch author is away, or
unreliable, or if there is a hard deadline near and I'm worried that
the updated patch might not be ready in time.  I'll also sometimes
make the changes directly if the patch is holding up a larger, more
important patch(set), but that is really rare.  I'm sure I've made
changes for other reasons in the past, and I'm sure I'll make changes
for other reasons in the future, but hopefully this will give you a
better idea of how the process works :)

-- 
paul moore
www.paul-moore.com