Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <YuGTL79tPPoalk/j@chromium.org> <20220729193107.2466530-1-jrosenth@chromium.org>
In-Reply-To: <20220729193107.2466530-1-jrosenth@chromium.org>
From:   Julius Werner <jwerner@chromium.org>
Date:   Fri, 29 Jul 2022 17:08:14 -0700
Message-ID: <CAODwPW-JzXXsEANaS+6n695YqriAQ0j0LXm31R2u1OP3MhX9Uw@mail.gmail.com>
Subject: Re: [PATCH v3] firmware: google: Implement cbmem in sysfs driver
To:     Jack Rosenthal <jrosenth@chromium.org>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        chrome-platform@lists.linux.dev,
        Stephen Boyd <swboyd@chromium.org>,
        Guenter Roeck <groeck@chromium.org>,
        Julius Werner <jwerner@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Thanks, I think this looks great in general!

> + * Copyright 2022 Google Inc.

LLC

> +/*
> + * This list should be kept in sync with
> + * src/commonlib/bsd/include/commonlib/bsd/cbmem_id.h from coreboot.
> + */
> +static const struct {
> +       const char *alias;
> +       u32 id;
> +} cbmem_aliases[] = {
> +       { "acpi", 0x41435049 },
> +       { "acpi-bert", 0x42455254 },
> +       { "acpi-cnvs", 0x434e5653 },

I don't think we really want to keep this in the kernel? New entries
get added here all the time, I think it would really be way more
effort than benefit to keep this in sync. It also doesn't really mesh
with the idea that this is a thin interface in the kernel that doesn't
do any heavyweight interpretation. I'd say keep out the alias stuff
and just have the numeric nodes only.

> +static ssize_t cbmem_entry_mem_write(struct file *filp, struct kobject *kobp,
> +                                    struct bin_attribute *bin_attr, char *buf,
> +                                    loff_t pos, size_t count)
> +{
> +       struct cbmem_entry *entry = bin_attr->private;
> +
> +       if (!capable(CAP_SYS_ADMIN))
> +               return -EPERM;

Not sure what the current standard kernel policy on these things is,
but we already have file permission bits to control this so generally
I'd leave it at that and let userspace decide how to manage access.
(Forcing everything that could possibly be security-sensitive to be
"root only" just means more things are forced to run as root, which is
also not a good thing.)

> +static int cbmem_entry_setup(struct cbmem_entry *entry)
> +{
> +       int ret;
> +
> +       entry->mem_file_buf = memremap(entry->dev->cbmem_entry.address,
> +                                      entry->dev->cbmem_entry.entry_size,
> +                                      MEMREMAP_WB);
> +       if (!entry->mem_file_buf)
> +               return -ENOMEM;

Can this use devm_memremap() for automatic cleanup?

> +MODULE_AUTHOR("Google, Inc.");

LLC