lkml.org 
[lkml]   [2023]   [Jan]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v3 04/24] powerpc/secvar: Handle format string in the consumer
    From
    Date
    On Thu, 2023-01-19 at 11:17 +1000, Nicholas Piggin wrote:
    > On Wed Jan 18, 2023 at 4:10 PM AEST, Andrew Donnellan wrote:
    > > From: Russell Currey <ruscur@russell.cc>
    > >
    > > The code that handles the format string in secvar-sysfs.c is
    > > entirely
    > > OPAL specific, so create a new "format" op in secvar_operations to
    > > make
    > > the secvar code more generic.  No functional change.
    > >
    > > Signed-off-by: Russell Currey <ruscur@russell.cc>
    > > Signed-off-by: Andrew Donnellan <ajd@linux.ibm.com>
    > >
    > > ---
    > >
    > > v2: Use sysfs_emit() instead of sprintf() (gregkh)
    > >
    > > v3: Enforce format string size limit (ruscur)
    > > ---
    > >  arch/powerpc/include/asm/secvar.h            |  3 +++
    > >  arch/powerpc/kernel/secvar-sysfs.c           | 23 ++++------------
    > > --
    > >  arch/powerpc/platforms/powernv/opal-secvar.c | 25
    > > ++++++++++++++++++++
    > >  3 files changed, 33 insertions(+), 18 deletions(-)
    > >
    > > diff --git a/arch/powerpc/include/asm/secvar.h
    > > b/arch/powerpc/include/asm/secvar.h
    > > index 07ba36f868a7..8b6475589120 100644
    > > --- a/arch/powerpc/include/asm/secvar.h
    > > +++ b/arch/powerpc/include/asm/secvar.h
    > > @@ -11,12 +11,15 @@
    > >  #include <linux/types.h>
    > >  #include <linux/errno.h>
    > >  
    > > +#define SECVAR_MAX_FORMAT_LEN  30 // max length of string returned
    > > by ->format()
    > > +
    > >  extern const struct secvar_operations *secvar_ops;
    > >  
    > >  struct secvar_operations {
    > >         int (*get)(const char *key, u64 key_len, u8 *data, u64
    > > *data_size);
    > >         int (*get_next)(const char *key, u64 *key_len, u64
    > > keybufsize);
    > >         int (*set)(const char *key, u64 key_len, u8 *data, u64
    > > data_size);
    > > +       ssize_t (*format)(char *buf);
    > >  };
    > >  
    > >  #ifdef CONFIG_PPC_SECURE_BOOT
    > > diff --git a/arch/powerpc/kernel/secvar-sysfs.c
    > > b/arch/powerpc/kernel/secvar-sysfs.c
    > > index 462cacc0ca60..d3858eedd72c 100644
    > > --- a/arch/powerpc/kernel/secvar-sysfs.c
    > > +++ b/arch/powerpc/kernel/secvar-sysfs.c
    > > @@ -21,26 +21,13 @@ static struct kset *secvar_kset;
    > >  static ssize_t format_show(struct kobject *kobj, struct
    > > kobj_attribute *attr,
    > >                            char *buf)
    > >  {
    > > -       ssize_t rc = 0;
    > > -       struct device_node *node;
    > > -       const char *format;
    > > -
    > > -       node = of_find_compatible_node(NULL, NULL, "ibm,secvar-
    > > backend");
    > > -       if (!of_device_is_available(node)) {
    > > -               rc = -ENODEV;
    > > -               goto out;
    > > -       }
    > > +       char tmp[SECVAR_MAX_FORMAT_LEN];
    > > +       ssize_t len = secvar_ops->format(tmp);
    > >  
    > > -       rc = of_property_read_string(node, "format", &format);
    > > -       if (rc)
    > > -               goto out;
    > > +       if (len <= 0)
    > > +               return -EIO;
    >
    > AFAIKS this does have a functional change, it loses the return value.
    > Why not return len if it is < 0, and -EIO if len == 0?

    In v2 mpe suggested the following:

    I'm not sure you should pass that raw error back to sysfs. Some of
    the
    values could be confusing, eg. if you return -EINVAL it looks like a
    parameter to the read() syscall was invalid. Might be better to just
    return -EIO.

    Following that advice, I don't think we should return something other
    than -EIO, but we should at least pr_err() to document the error - this
    isn't something that should ever fail.

    >
    > Thanks,
    > Nick

    \
     
     \ /
      Last update: 2023-03-26 23:48    [W:2.762 / U:0.028 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site