Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 2 Apr 2021 11:23:58 -0700
From:   Kees Cook <keescook@chromium.org>
To:     Nick Desaulniers <ndesaulniers@google.com>
Cc:     Gioh Kim <gi-oh.kim@ionos.com>,
        LKML <linux-kernel@vger.kernel.org>, jinpu.wang@ionos.com,
        Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [PATCH] lib/string: Introduce sysfs_streqcase
Message-ID: <202104021122.7896C66@keescook>
References: <20210402094042.9218-1-gi-oh.kim@ionos.com>
 <CAKwvOdkFJ_WFpt2+rnNR3tbrdFky2NnEOWSG7MhgLrBHJAOEVw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAKwvOdkFJ_WFpt2+rnNR3tbrdFky2NnEOWSG7MhgLrBHJAOEVw@mail.gmail.com>
Precedence: bulk

On Fri, Apr 02, 2021 at 11:17:13AM -0700, Nick Desaulniers wrote:
> Thanks for the patch!
> 
> + akpm (please remember to run ./scripts/get_maintainer.pl on your patch files)
> 
> On Fri, Apr 2, 2021 at 2:41 AM Gioh Kim <gi-oh.kim@ionos.com> wrote:
> >
> > As the name shows, it checks if strings are equal in case insensitive
> > manner. I found some cases using strncasecmp to check the entire
> > strings and they would not work as intended.
> >
> > For example, drivers/infiniband/ulp/rtrs/rtrs-clt-sysfs.c uses
> > strncasecmp to check that the input via sysfs is "mi". But it would
> > work even-if the input is "min-wrongcommand".
> > And also drivers/pnp/interface.c checks "disable" command with
> > strncasecmp but it would also work if the command is "disable-wrong".
> 
> Perhaps those callers should be using strcasecmp then, rather than strncasecmp?
> 
> Also, if they're being liberal in accepting either case, I don't see
> why the sysfs nodes should be strict in rejecting trailing input at
> that point.

I think this shouldn't be prefixed "sysfs_" -- name it for what it does,
not where it gets used, if it's a general utility function.

-Kees

> 
> >
> > Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com>
> > ---
> >  lib/string.c | 23 +++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> >
> > diff --git a/lib/string.c b/lib/string.c
> > index 7548eb715ddb..5e6bc0d3d5c6 100644
> > --- a/lib/string.c
> > +++ b/lib/string.c
> > @@ -714,6 +714,29 @@ bool sysfs_streq(const char *s1, const char *s2)
> >  }
> >  EXPORT_SYMBOL(sysfs_streq);
> >
> > +/**
> > + * sysfs_streqcase - same to sysfs_streq and case insensitive
> > + * @s1: one string
> > + * @s2: another string
> > + *
> > + */
> > +bool sysfs_streqcase(const char *s1, const char *s2)
> > +{
> > +       while (*s1 && tolower(*s1) == tolower(*s2)) {
> > +               s1++;
> > +               s2++;
> > +       }
> > +
> > +       if (*s1 == *s2)
> > +               return true;
> > +       if (!*s1 && *s2 == '\n' && !s2[1])
> > +               return true;
> > +       if (*s1 == '\n' && !s1[1] && !*s2)
> > +               return true;
> > +       return false;
> > +}
> > +EXPORT_SYMBOL(sysfs_streqcase);
> 
> This should be declared in
> include/linux/string.h
> in order for others to use this (as 0day bot notes).
> 
> > +
> >  /**
> >   * match_string - matches given string in an array
> >   * @array:     array of strings
> > --
> > 2.25.1
> >
> 
> 
> --
> Thanks,
> ~Nick Desaulniers

-- 
Kees Cook