linux-kernel - Re: [PATCH v3] firmware: google: Implement cbmem in sysfs driver

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: <CAODwPW-JzXXsEANaS+6n695YqriAQ0j0LXm31R2u1OP3MhX9Uw@mail.gmail.com>
Date:   Fri, 29 Jul 2022 17:08:14 -0700
From:   Julius Werner <jwerner@...omium.org>
To:     Jack Rosenthal <jrosenth@...omium.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        chrome-platform@...ts.linux.dev,
        Stephen Boyd <swboyd@...omium.org>,
        Guenter Roeck <groeck@...omium.org>,
        Julius Werner <jwerner@...omium.org>
Subject: Re: [PATCH v3] firmware: google: Implement cbmem in sysfs driver

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