Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <cover.1600661418.git.yifeifz2@illinois.edu> <6af89348c08a4820039e614a090d35aa1583acff.1600661419.git.yifeifz2@illinois.edu>
 <CAG48ez0OqZavgm0BkGjCAJUr5UfRgbeCbmLOZFJ=Rj46COcN3Q@mail.gmail.com>
In-Reply-To: <CAG48ez0OqZavgm0BkGjCAJUr5UfRgbeCbmLOZFJ=Rj46COcN3Q@mail.gmail.com>
From:   YiFei Zhu <zhuyifei1999@gmail.com>
Date:   Mon, 21 Sep 2020 18:44:13 -0500
Message-ID: <CABqSeAQhVFeG1Frvu60XfUnRQ78YRS2Uaw1EsBobKVku-vVoDQ@mail.gmail.com>
Subject: Re: [RFC PATCH seccomp 1/2] seccomp/cache: Add "emulator" to check if
 filter is arg-dependent
To:     Jann Horn <jannh@google.com>
Cc:     Linux Containers <containers@lists.linux-foundation.org>,
        YiFei Zhu <yifeifz2@illinois.edu>, bpf <bpf@vger.kernel.org>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Dimitrios Skarlatos <dskarlat@cs.cmu.edu>,
        Giuseppe Scrivano <gscrivan@redhat.com>,
        Hubertus Franke <frankeh@us.ibm.com>,
        Jack Chen <jianyan2@illinois.edu>,
        Josep Torrellas <torrella@illinois.edu>,
        Kees Cook <keescook@chromium.org>,
        Tianyin Xu <tyxu@illinois.edu>,
        Tobin Feldman-Fitzthum <tobin@ibm.com>,
        Valentin Rothberg <vrothber@redhat.com>,
        Andy Lutomirski <luto@amacapital.net>,
        Will Drewry <wad@chromium.org>,
        Aleksa Sarai <cyphar@cyphar.com>,
        kernel list <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Sep 21, 2020 at 12:47 PM Jann Horn <jannh@google.com> wrote:
> Is this actually necessary, or can we just bail out on any branch that
> we can't statically resolve?

I think after we do enumerate the arch numbers it would make much more
sense. Since if there is a branch after arch number and syscall
numbers are fixed we can assume that the return values will be
different if one or the other case is followed.

> Also: If it turns out that the time spent in seccomp_cache_prepare()
> is measurable for large filters, a possible improvement would be to
> keep track of the last syscall number for which the result would be
> the same as for the current one, such that instead of evaluating the
> filter for one instruction at a time, it would effectively be
> evaluated for a range at a time. That should be pretty straightforward
> to implement, I think.

My concern was more of the possibly-exponential amount of time &
memory needed to evaluate an adversarial filter containing full of
unresolveable branches, hence the max pending states. If we never
follow both branches then evaluation should not be much of a concern.

> > +       depends on SECCOMP
> > +       depends on SECCOMP_FILTER
>
> SECCOMP_FILTER already depends on SECCOMP, so the "depends on SECCOMP"
> line is unnecessary.

The reason that this is here is because of the looks in menuconfig.
SECCOMP is the direct previous entry, so if this depends on SECCOMP
then the config would be indented. Is this looks not worth keeping or
is there some better way to do this?

> > +       help
> > +         Seccomp filters can potentially incur large overhead for each
> > +         system call. This can alleviate some of the overhead.
> > +
> > +         If in doubt, select 'none'.
>
> This should not be in arch/x86. Other architectures, such as arm64,
> should also be able to use this without extra work.

In the initial RFC patch I only added to x86. I could add it to any
arch that has seccomp filters. Though, I'm wondering, why is SECCOMP
in the arch-specific Kconfigs?

> I think we should probably just bail out if we see anything that's
> BPF_ST/BPF_STX. I've never seen seccomp filters that actually use that
> part of cBPF.
>
> But in case we do need this, maybe instead of using "2 +" for all
> these things, the cBPF memory slots should be in a separate array.

Ok I'll just bail.

YiFei Zhu