Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <20181026180042.52199-1-joel@joelfernandes.org>
References: <20181026180042.52199-1-joel@joelfernandes.org>
From:   Kees Cook <keescook@chromium.org>
Date:   Fri, 26 Oct 2018 20:04:24 +0100
Message-ID: <CAGXu5jLKLKDwVgRoMx57Y76V9FVJqQ+EeTHB2CnxAN5CnC2Jmw@mail.gmail.com>
Subject: Re: [RFC 1/6] pstore: map pstore types to names
To:     "Joel Fernandes (Google)" <joel@joelfernandes.org>
Cc:     LKML <linux-kernel@vger.kernel.org>, kernel-team@android.com,
        Anton Vorontsov <anton@enomsg.org>,
        Colin Cross <ccross@android.com>,
        Tony Luck <tony.luck@intel.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Oct 26, 2018 at 7:00 PM, Joel Fernandes (Google)
<joel@joelfernandes.org> wrote:
> In later patches we will need to map types to names, so create a table
> for that which can also be used and reused in different parts of old and
> new code. Also use it to save the type in the PRZ which will be useful
> in later patches.

Yes, I like it. :) Comments below...

>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> ---
>  fs/pstore/inode.c          | 44 ++++++++++++++++++++------------------
>  fs/pstore/ram.c            |  4 +++-
>  include/linux/pstore.h     | 29 +++++++++++++++++++++++++
>  include/linux/pstore_ram.h |  2 ++
>  4 files changed, 57 insertions(+), 22 deletions(-)
>
> diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
> index 5fcb845b9fec..43757049d384 100644
> --- a/fs/pstore/inode.c
> +++ b/fs/pstore/inode.c
> @@ -304,6 +304,7 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
>         struct dentry           *dentry;
>         struct inode            *inode;
>         int                     rc = 0;
> +       enum pstore_type_id     type;
>         char                    name[PSTORE_NAMELEN];
>         struct pstore_private   *private, *pos;
>         unsigned long           flags;
> @@ -335,43 +336,44 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
>                 goto fail_alloc;
>         private->record = record;
>
> -       switch (record->type) {
> +       type = record->type;

Let's rename PSTORE_TYPE_UNKNOWN in the enum to be PSTORE_TYPE_MAX and
!= 255 (just leave it at the end). The value is never exposed to
userspace (nor to backend storage), so we can instead use it as the
bounds-check for doing type -> name mappings. (The one use in erst can
just be renamed.)

Then we can add a function to do the bounds checking and mapping
(instead of using a bare array lookup).

> +       switch (type) {
>         case PSTORE_TYPE_DMESG:
> -               scnprintf(name, sizeof(name), "dmesg-%s-%llu%s",
> -                         record->psi->name, record->id,
> -                         record->compressed ? ".enc.z" : "");
> +               scnprintf(name, sizeof(name), "%s-%s-%llu%s",
> +                        pstore_names[type], record->psi->name, record->id,
> +                        record->compressed ? ".enc.z" : "");
>                 break;
>         case PSTORE_TYPE_CONSOLE:
> -               scnprintf(name, sizeof(name), "console-%s-%llu",
> -                         record->psi->name, record->id);
> +               scnprintf(name, sizeof(name), "%s-%s-%llu",
> +                         pstore_names[type], record->psi->name, record->id);
>                 break;
>         case PSTORE_TYPE_FTRACE:
> -               scnprintf(name, sizeof(name), "ftrace-%s-%llu",
> -                         record->psi->name, record->id);
> +               scnprintf(name, sizeof(name), "%s-%s-%llu",
> +                         pstore_names[type], record->psi->name, record->id);
>                 break;
>         case PSTORE_TYPE_MCE:
> -               scnprintf(name, sizeof(name), "mce-%s-%llu",
> -                         record->psi->name, record->id);
> +               scnprintf(name, sizeof(name), "%s-%s-%llu",
> +                         pstore_names[type], record->psi->name, record->id);
>                 break;
>         case PSTORE_TYPE_PPC_RTAS:
> -               scnprintf(name, sizeof(name), "rtas-%s-%llu",
> -                         record->psi->name, record->id);
> +               scnprintf(name, sizeof(name), "%s-%s-%llu",
> +                         pstore_names[type], record->psi->name, record->id);
>                 break;
>         case PSTORE_TYPE_PPC_OF:
> -               scnprintf(name, sizeof(name), "powerpc-ofw-%s-%llu",
> -                         record->psi->name, record->id);
> +               scnprintf(name, sizeof(name), "%s-%s-%llu",
> +                         pstore_names[type], record->psi->name, record->id);
>                 break;
>         case PSTORE_TYPE_PPC_COMMON:
> -               scnprintf(name, sizeof(name), "powerpc-common-%s-%llu",
> -                         record->psi->name, record->id);
> +               scnprintf(name, sizeof(name), "%s-%s-%llu",
> +                         pstore_names[type], record->psi->name, record->id);
>                 break;
>         case PSTORE_TYPE_PMSG:
> -               scnprintf(name, sizeof(name), "pmsg-%s-%llu",
> -                         record->psi->name, record->id);
> +               scnprintf(name, sizeof(name), "%s-%s-%llu",
> +                         pstore_names[type], record->psi->name, record->id);
>                 break;
>         case PSTORE_TYPE_PPC_OPAL:
> -               scnprintf(name, sizeof(name), "powerpc-opal-%s-%llu",
> -                         record->psi->name, record->id);
> +               scnprintf(name, sizeof(name), "%s-%s-%llu",
> +                         pstore_names[type], record->psi->name, record->id);
>                 break;
>         case PSTORE_TYPE_UNKNOWN:
>                 scnprintf(name, sizeof(name), "unknown-%s-%llu",

All of these, including PSTORE_TYPE_UNKNOWN are now identical (you can
include the .enc.z logic in for all of them. I think the entire switch
can be dropped, yes?

scnprintf(name, sizeof(name), "%s-%s-%llu%s",
               pstore_name(record->type), record->psi->name, record->id,
               record->compressed ? ".enc.z" : "")

> @@ -379,7 +381,7 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
>                 break;
>         default:
>                 scnprintf(name, sizeof(name), "type%d-%s-%llu",
> -                         record->type, record->psi->name, record->id);
> +                         type, record->psi->name, record->id);
>                 break;
>         }
>
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index f4fd2e72add4..c7cd858adce7 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -604,6 +604,7 @@ static int ramoops_init_przs(const char *name,
>                         goto fail;
>                 }
>                 *paddr += zone_sz;
> +               prz_ar[i]->type = pstore_name_to_type(name);
>         }
>
>         *przs = prz_ar;
> @@ -642,6 +643,7 @@ static int ramoops_init_prz(const char *name,
>         persistent_ram_zap(*prz);
>
>         *paddr += sz;
> +       (*prz)->type = pstore_name_to_type(name);
>
>         return 0;
>  }
> @@ -777,7 +779,7 @@ static int ramoops_probe(struct platform_device *pdev)
>
>         dump_mem_sz = cxt->size - cxt->console_size - cxt->ftrace_size
>                         - cxt->pmsg_size;
> -       err = ramoops_init_przs("dump", dev, cxt, &cxt->dprzs, &paddr,
> +       err = ramoops_init_przs("dmesg", dev, cxt, &cxt->dprzs, &paddr,

Yup. That's a funny mismatch. Not exposed to userspace in a released kernel. ;)

>                                 dump_mem_sz, cxt->record_size,
>                                 &cxt->max_dump_cnt, 0, 0);
>         if (err)
> diff --git a/include/linux/pstore.h b/include/linux/pstore.h
> index a15bc4d48752..4a3dbdffd8d3 100644
> --- a/include/linux/pstore.h
> +++ b/include/linux/pstore.h
> @@ -47,6 +47,21 @@ enum pstore_type_id {
>         PSTORE_TYPE_UNKNOWN     = 255
>  };
>
> +/* names should be in the same order as the above table */
> +static char __maybe_unused *pstore_names[] = {

This should be static const char * const pstore_names[], I think?

> +       "dmesg",
> +       "mce",
> +       "console",
> +       "ftrace",
> +       "rtas",
> +       "powerpc-ofw",
> +       "powerpc-common",
> +       "pmsg",
> +       "powerpc-opal",
> +       "event",
> +       "unknown", /* unknown should be the last string */

End this with a NULL for a cheaper compare below.

> +};
> +
>  struct pstore_info;
>  /**
>   * struct pstore_record - details of a pstore record entry
> @@ -274,4 +289,18 @@ pstore_ftrace_write_timestamp(struct pstore_ftrace_record *rec, u64 val)
>  }
>  #endif
>
> +static inline enum pstore_type_id pstore_name_to_type(const char *name)
> +{
> +       char *str;
> +       int i = 0;
> +
> +       for (; strncmp(pstore_names[i], "unknown", 7); i++) {
char **ptr;

for (ptr = pstores_names; *ptr; ptr++) {

> +               str = pstore_names[i];
> +               if (!strncmp(name, str, strlen(str)))

"n" version not needed: the LHS is controlled and we want full matching:

    if (!strcmp(*ptr, name))

> +                       return (enum pstore_type_id)i;

I don't think this cast is needed?

        return (ptr - pstores_names);

> +       }
> +
> +       return PSTORE_TYPE_UNKNOWN;
> +}
> +
>  #endif /*_LINUX_PSTORE_H*/
> diff --git a/include/linux/pstore_ram.h b/include/linux/pstore_ram.h
> index e6d226464838..ee0f493254dd 100644
> --- a/include/linux/pstore_ram.h
> +++ b/include/linux/pstore_ram.h
> @@ -22,6 +22,7 @@
>  #include <linux/init.h>
>  #include <linux/kernel.h>
>  #include <linux/list.h>
> +#include <linux/pstore.h>
>  #include <linux/types.h>
>
>  /*
> @@ -47,6 +48,7 @@ struct persistent_ram_zone {
>         size_t size;
>         void *vaddr;
>         struct persistent_ram_buffer *buffer;
> +       enum pstore_type_id type;
>         size_t buffer_size;
>         u32 flags;
>         raw_spinlock_t buffer_lock;
> --
> 2.19.1.568.g152ad8e336-goog
>


-- 
Kees Cook