linux-kernel - Re: [PATCH v36 22/24] selftests/x86: Add a selftest for SGX

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: <20200827152051.GB22351@sjchrist-ice>
Date:   Thu, 27 Aug 2020 08:20:51 -0700
From:   Sean Christopherson <sean.j.christopherson@...el.com>
To:     Nathaniel McCallum <npmccallum@...hat.com>
Cc:     Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>,
        X86 ML <x86@...nel.org>, linux-sgx@...r.kernel.org,
        LKML <linux-kernel@...r.kernel.org>,
        linux-kselftest@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        asapek@...gle.com, Borislav Petkov <bp@...en8.de>,
        "Xing, Cedric" <cedric.xing@...el.com>, chenalexchen@...gle.com,
        Conrad Parker <conradparker@...gle.com>, cyhanish@...gle.com,
        Dave Hansen <dave.hansen@...el.com>,
        "Huang, Haitao" <haitao.huang@...el.com>,
        Josh Triplett <josh@...htriplett.org>,
        "Huang, Kai" <kai.huang@...el.com>,
        "Svahn, Kai" <kai.svahn@...el.com>, Keith Moyer <kmoy@...gle.com>,
        Christian Ludloff <ludloff@...gle.com>,
        Andy Lutomirski <luto@...nel.org>,
        Neil Horman <nhorman@...hat.com>,
        Patrick Uiterwijk <puiterwijk@...hat.com>,
        David Rientjes <rientjes@...gle.com>,
        Thomas Gleixner <tglx@...utronix.de>, yaozhangx@...gle.com
Subject: Re: [PATCH v36 22/24] selftests/x86: Add a selftest for SGX

On Thu, Aug 27, 2020 at 12:47:04AM -0400, Nathaniel McCallum wrote:
> > +int main(int argc, char *argv[], char *envp[])
> > +{
> > +       struct sgx_enclave_exception exception;
> > +       struct vdso_symtab symtab;
> > +       Elf64_Sym *eenter_sym;
> > +       uint64_t result = 0;
> > +       struct encl encl;
> > +       unsigned int i;
> > +       void *addr;
> > +
> > +       if (!encl_load("test_encl.elf", &encl))
> > +               goto err;
> > +
> > +       if (!encl_measure(&encl))
> > +               goto err;
> > +
> > +       if (!encl_build(&encl))
> > +               goto err;
> > +
> > +       /*
> > +        * An enclave consumer only must do this.
> > +        */
> > +       for (i = 0; i < encl.nr_segments; i++) {
> > +               struct encl_segment *seg = &encl.segment_tbl[i];
> > +
> > +               addr = mmap((void *)encl.encl_base + seg->offset, seg->size,
> > +                           seg->prot, MAP_SHARED | MAP_FIXED, encl.fd, 0);
> 
> My patch version is a bit behind (v32), but I suspect this still
> applies. I discovered the following by accident.
> 
> In the Enarx code base, this invocation succeeds:
> mmap(0x200000000000, 0x1000, PROT_READ | PROT_WRITE, MAP_SHARED |
> MAP_FIXED, sgxfd, 0)
> 
> However, this one fails with -EINVAL:
> mmap(0x200000000000, 0x1000, PROT_READ | PROT_WRITE,
> MAP_SHARED_VALIDATE | MAP_FIXED, sgxfd, 0)
> 
> From man mmap:
> 
>        MAP_SHARED_VALIDATE (since Linux 4.15)
>               This flag provides the same behavior as MAP_SHARED
> except that MAP_SHARED mappings ignore unknown
>               flags in flags.  By contrast, when creating a mapping
> using MAP_SHARED_VALIDATE, the kernel veri‐
>               fies  all  passed  flags  are  known  and fails the
> mapping with the error EOPNOTSUPP for unknown
>               flags.  This mapping type is also required to be able to
> use some mapping flags (e.g., MAP_SYNC).
> 
> I can try again on a newer patch set tomorrow if need be. But the
> documentation of mmap() doesn't match the behavior I'm seeing. A brief
> look through the patch set didn't turn up anything obvious that could
> be causing this.

This is a bug in sgx_get_unmapped_area().  EPC must be mapped SHARED, and
so MAP_PRIVATE is disallowed.  The current check is:

  if (flags & MAP_PRIVATE)
          return -EINVAL;

and the base "flags" are:

  #define MAP_SHARED      0x01            /* Share changes */
  #define MAP_PRIVATE     0x02            /* Changes are private */
  #define MAP_SHARED_VALIDATE 0x03        /* share + validate extension flags */

which causes the SGX check to interpret MAP_SHARED_VALIDATE as MAP_PRIVATE.
The types are just that, types, not flag modifiers.  So the SGX code needs
to be:

  if ((flags & MAP_TYPE) == MAP_PRIVATE)
          return -EINVAL;

or

  unsigned long map_type = (flags & MAP_TYPE);

  if (map_type != MAP_SHARED && map_type != MAP_SHARED_VALIDATE)
          return -EINVAL;


Side topic, there is at least one existing bug of this nature, in mm/nommu.c.
I'll send a patch for that and look for any other instances of the bad
pattern.