linux-kernel - Re: [PATCH] string: Improve the generic strlcpy() implementation

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: <CA+55aFyWOXJ9vxnd=vJqch0DfcMGAeSim1fbFwSrrd6jr7B6sw@mail.gmail.com>
Date:	Mon, 5 Oct 2015 13:28:24 +0100
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Ingo Molnar <mingo@...nel.org>
Cc:	Chris Metcalf <cmetcalf@...hip.com>,
	open list <linux-kernel@...r.kernel.org>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Thomas Gleixner <tglx@...utronix.de>,
	"H. Peter Anvin" <hpa@...or.com>, Borislav Petkov <bp@...en8.de>
Subject: Re: [PATCH] string: Improve the generic strlcpy() implementation

On Mon, Oct 5, 2015 at 12:27 PM, Ingo Molnar <mingo@...nel.org> wrote:
>
> Interesting. I noticed that strscpy() says this in its comments:
>
>  * In addition, the implementation is robust to the string changing out
>  * from underneath it, unlike the current strlcpy() implementation.
>
> The strscpy() interface is very nice, but shouldn't we also fix this strlcpy()
> unrobustness/race it refers to, in light of the 2000+ existing strlcpy() call
> sites?

Well, I'm not sure the race really matters. I personally think
strlcpy() is a horrible interface, and the thing is, the return value
of strlcpy (which is what can race) is kind of useless because it's
not actually the size of the resulting string *anyway* (because of the
overflow issue).

So I'm not sure it's worth even fixing.

Also, if you do this, then you're better off using the (hopefully
optimized) "strlen()" for the tail part of the strlcpy destination for
the overflow case that didn't get copied.

In other words, I think your patch is overly fragile and complex.
Instead, you might choose to implement strlcpy() in terms of
"strscpy()" and "strlen()".

Something like

  int strlcpy(dst, src, len)
  {
     // do the actual copy
     int n = strscpy(dst, src, len);

     // handle the insane and broken strlcpy overflow return value
     if (n < 0)
         return len + strlen(src+len);

     return n;
   }

but I didn't actually verify that the above is correct for all the corner case.

The point being, that you really shouldn't waste your time
implementing the broken BSD strlcpy crap as an actual first-class
implementation. You're better off just using a strscpy() as the
primary engine, and then implementing the broken strlcpy interfaces on
top of it.

Does the above work? I'd take a patch that implements that if it's
tested and somebody has thought about it a lot. But I don't like your
patch that open-codes the insane interface with complex and fragile
code.

          Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/