netdev - RE: [PATCH RFCv2 2/3] lib/vsprintf.c: make %pD print full path for file

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: <AM6PR08MB4376DD4B202E967078B45D2FF73F9@AM6PR08MB4376.eurprd08.prod.outlook.com>
Date:   Mon, 31 May 2021 00:39:50 +0000
From:   Justin He <Justin.He@....com>
To:     Matthew Wilcox <willy@...radead.org>
CC:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Petr Mladek <pmladek@...e.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Sergey Senozhatsky <senozhatsky@...omium.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Jonathan Corbet <corbet@....net>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Luca Coelho <luciano.coelho@...el.com>,
        Kalle Valo <kvalo@...eaurora.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Heiko Carstens <hca@...ux.ibm.com>,
        Vasily Gorbik <gor@...ux.ibm.com>,
        Christian Borntraeger <borntraeger@...ibm.com>,
        Johannes Berg <johannes.berg@...el.com>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-wireless@...r.kernel.org" <linux-wireless@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-s390@...r.kernel.org" <linux-s390@...r.kernel.org>
Subject: RE: [PATCH RFCv2 2/3] lib/vsprintf.c: make %pD print full path for
 file



> -----Original Message-----
> From: Matthew Wilcox <willy@...radead.org>
> Sent: Friday, May 28, 2021 11:22 PM
> To: Justin He <Justin.He@....com>
> Cc: Linus Torvalds <torvalds@...ux-foundation.org>; Petr Mladek
> <pmladek@...e.com>; Steven Rostedt <rostedt@...dmis.org>; Sergey
> Senozhatsky <senozhatsky@...omium.org>; Andy Shevchenko
> <andriy.shevchenko@...ux.intel.com>; Rasmus Villemoes
> <linux@...musvillemoes.dk>; Jonathan Corbet <corbet@....net>; Alexander
> Viro <viro@...iv.linux.org.uk>; Luca Coelho <luciano.coelho@...el.com>;
> Kalle Valo <kvalo@...eaurora.org>; David S. Miller <davem@...emloft.net>;
> Jakub Kicinski <kuba@...nel.org>; Heiko Carstens <hca@...ux.ibm.com>;
> Vasily Gorbik <gor@...ux.ibm.com>; Christian Borntraeger
> <borntraeger@...ibm.com>; Johannes Berg <johannes.berg@...el.com>; linux-
> doc@...r.kernel.org; linux-kernel@...r.kernel.org; linux-
> wireless@...r.kernel.org; netdev@...r.kernel.org; linux-
> s390@...r.kernel.org
> Subject: Re: [PATCH RFCv2 2/3] lib/vsprintf.c: make %pD print full path
> for file
>
> On Fri, May 28, 2021 at 03:09:28PM +0000, Justin He wrote:
> > > I'm not sure why it's so complicated.  p->len records how many bytes
> > > are needed for the entire path; can't you just return -p->len ?
> >
> > prepend_name() will return at the beginning if p->len is <0 in this case,
> > we can't even get the correct full path size if keep __prepend_path
> unchanged.
> > We need another new helper __prepend_path_size() to get the full path
> size
> > regardless of the negative value p->len.
>
> It's a little hard to follow, based on just the patches.  Is there a
> git tree somewhere of Al's patches that you're based on?

The git tree of Al's patches is at:
https://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git/log/?h=work.d_path

>
> Seems to me that prepend_name() is just fine because it updates p->len
> before returning false:
>
>  static bool prepend_name(struct prepend_buffer *p, const struct qstr
> *name)
>  {
>       const char *dname = smp_load_acquire(&name->name); /* ^^^ */
>       u32 dlen = READ_ONCE(name->len);
>       char *s;
>
>       p->len -= dlen + 1;
>       if (unlikely(p->len < 0))
>               return false;
>
> I think the only change you'd need to make for vsnprintf() is in
> prepend_path():
>
> -             if (!prepend_name(&b, &dentry->d_name))
> -                     break;
> +             prepend_name(&b, &dentry->d_name);
>
> Would that hurt anything else?
I will try your suggestion soon.

>
> > More than that, even the 1st vsnprintf could have _end_ > _buf_ in some
> case:
> > What if printk("%pD", filp) ? The 1st vsnprintf has positive (end-buf).
>
> I don't understand the problem ... if p->len is positive, then you
> succeeded.  if p->len is negative then -p->len is the expected return
> value from vsnprintf().  No?

There are 3 cases I once met in my debugging:
1. p->len is positive but too small (e.g. end-buf is 6). In first prepend_name
loop p-len-=dlen, then p->len is negative

2. p->len is negative at the very beginning (i.e. end-buf is negative)

3. p->len positive and large enough. Typically the 2nd vsnprintf of printk


--
Cheers,
Justin (Jia He)


IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.