linux-kernel - Re: [PATCH v1] seq_file: convert mangle_path() to use string_escape

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] [day] [month] [year] [list]

Message-ID: <CAHp75VfZq7NhDX8c9EGa_4v-5nApiqF5RLbw49HO--H6h-YWnQ@mail.gmail.com>
Date:   Thu, 10 Jan 2019 02:19:53 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Andrew Morton <akpm@...ux-foundation.org>
Cc:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Linux FS Devel <linux-fsdevel@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Kees Cook <keescook@...omium.org>
Subject: Re: [PATCH v1] seq_file: convert mangle_path() to use string_escape_str()

On Wed, Jan 9, 2019 at 8:21 PM Andrew Morton <akpm@...ux-foundation.org> wrote:
> On Wed,  9 Jan 2019 17:40:22 +0200 Andy Shevchenko <andriy.shevchenko@...ux.intel.com> wrote:
>
> > Since string_escape_str() does not support overlapping buffer first we check if
> > there is enough room in the buffer and then update a path. The side effect of
> > this change is in case of failure the buffer is left unchanged.

> >  char *mangle_path(char *s, const char *p, const char *esc)
> >  {

> > +     size_t len = p + strlen(p) - s;
> > +     int ret;
> > +
> > +     ret = string_escape_str(p, NULL, 0, ESCAPE_OCTAL, esc);
> > +     if (ret < len)
> > +             return s + string_escape_str(p, s, len, ESCAPE_OCTAL, esc);
> > +
> >       return NULL;
> >  }

> Confusing.
>
> I think the objective of the patch is to use an existing library
> function rather than open-coding, but the library function doesn't
> support in-place operation on the string.  So the old mangle_path() was
> OK with in-place conversion, but the new mangle_path() is not.  Is that
> correct?

As long as source buffer wouldn't be changed during conversion. Here
the same technique is used as in kasprintf(), i.e. first iteration we
would like to check if the buffer has enough size for the output, on
second do actual conversion.

Probably I need to add some test cases.

>  Do we know that all existing mangle_path() callers are OK
> with this?  Please make all this clear in the changelog.
>
> Also, the identifier `ret' is widely understood to mean "the value
> which this function will return", but that is not the case here.
> Please use a more appropriate identifier.

The meaning of it is a destination length. I will rename it.

-- 
With Best Regards,
Andy Shevchenko