linux-kernel - Re: [RFC PATCH 11/14] cxl/mem: Add a "RAW" send command

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201216204205.acum2sf652aspah3@intel.com>
Date:   Wed, 16 Dec 2020 12:42:05 -0800
From:   Ben Widawsky <ben.widawsky@...el.com>
To:     Dan Williams <dan.j.williams@...el.com>
Cc:     linux-cxl@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux PCI <linux-pci@...r.kernel.org>,
        Linux ACPI <linux-acpi@...r.kernel.org>,
        Ira Weiny <ira.weiny@...el.com>,
        Vishal Verma <vishal.l.verma@...el.com>,
        "Kelley, Sean V" <sean.v.kelley@...el.com>,
        Rafael Wysocki <rafael.j.wysocki@...el.com>,
        Bjorn Helgaas <helgaas@...nel.org>,
        Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        Jon Masters <jcm@...masters.org>,
        Chris Browy <cbrowy@...ry-design.com>,
        Randy Dunlap <rdunlap@...radead.org>,
        Christoph Hellwig <hch@...radead.org>
Subject: Re: [RFC PATCH 11/14] cxl/mem: Add a "RAW" send command

On 20-12-09 14:38:49, Dan Williams wrote:
> On Tue, Dec 8, 2020 at 4:24 PM Ben Widawsky <ben.widawsky@...el.com> wrote:
> >
> > The CXL memory device send interface will have a number of supported
> > commands. The raw command is not such a command. Raw commands allow
> > userspace to send a specified opcode to the underlying hardware and
> > bypass all driver checks on the command. This is useful for a couple of
> > usecases, mainly:
> > 1. Undocumented vendor specific hardware commands
> > 2. Prototyping new hardware commands not yet supported by the driver
> >
> > While this all sounds very powerful it comes with a couple of caveats:
> > 1. Bug reports using raw commands will not get the same level of
> >    attention as bug reports using supported commands (via taint).
> > 2. Supported commands will be rejected by the RAW command.
> >
> > Signed-off-by: Ben Widawsky <ben.widawsky@...el.com>
> > ---
> >  drivers/cxl/mem.c            | 32 ++++++++++++++++++++++++++++++++
> >  include/uapi/linux/cxl_mem.h | 14 ++++++++++++--
> >  2 files changed, 44 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> > index 0bf03afc0c80..a2cea7ac7cc6 100644
> > --- a/drivers/cxl/mem.c
> > +++ b/drivers/cxl/mem.c
> > @@ -115,6 +115,7 @@ struct cxl_mem_command {
> >
> >  static struct cxl_mem_command mem_commands[] = {
> >         CXL_CMD(INVALID, NONE, 0, 0, "Reserved", false, 0),
> > +       CXL_CMD(RAW, TAINT, ~0, ~0, "Raw", true, 0),
> 
> Why is the taint indication in the ABI? It seems like it only needs to
> be documented.
> 

It's removed per the previous patch discussion.

> >  };
> >
> >  static int cxl_mem_wait_for_doorbell(struct cxl_mem *cxlm)
> > @@ -326,6 +327,20 @@ static int cxl_mem_count_commands(void)
> >         return n;
> >  };
> >
> > +static struct cxl_mem_command *cxl_mem_find_command(u16 opcode)
> > +{
> > +       int i;
> > +
> > +       for (i = 0; i < ARRAY_SIZE(mem_commands); i++) {
> > +               struct cxl_mem_command *c = &mem_commands[i];
> > +
> > +               if (c->opcode == opcode)
> > +                       return c;
> > +       }
> > +
> > +       return NULL;
> > +};
> > +
> >  /**
> >   * handle_mailbox_cmd_from_user() - Dispatch a mailbox command.
> >   * @cxlmd: The CXL memory device to communicate with.
> > @@ -421,6 +436,23 @@ static int cxl_validate_cmd_from_user(struct cxl_send_command __user *user_cmd,
> >         c = &mem_commands[cmd.id];
> >         info = &c->info;
> >
> > +       /* Checks are bypassed for raw commands but along comes the taint! */
> > +       if (cmd.id == CXL_MEM_COMMAND_ID_RAW) {
> > +               struct cxl_mem_command temp =
> > +                       CXL_CMD(RAW, NONE, cmd.size_in, cmd.size_out, "Raw",
> > +                               true, cmd.raw.opcode);
> 
> Oh, I thought CXL_CMD() was only used to populate the mem_commands
> array. Feels out of place to use it here when all it is doing is
> updating the size_{in,out} and opcode fields. Mainly I'm interested in
> CXL_CMD() enforcing that the command-id is the mem_commands index.
> 

Agreed and removed.

> > +
> > +               if (cmd.raw.rsvd)
> > +                       return -EINVAL;
> > +
> > +               if (cxl_mem_find_command(cmd.raw.opcode))
> > +                       return -EPERM;
> > +
> > +               add_taint(TAINT_WARN, LOCKDEP_STILL_OK);
> 
> TAINT_WARN seems the wrong value, especially since no WARN has
> occurred. I feel that this is more in the spirit of
> TAINT_PROPRIETARY_MODULE, TAINT_OVERRIDDEN_ACPI_TABLE, and
> TAINT_OOT_MODULE. How about a new TAINT_RAW_PASSTHROUGH? I could use
> this for the acpi/nfit driver as well to disclaim responsibility for
> system errors that can result from not using the nominal
> kernel-provided commands.

I like it.