linux-kernel - Re: strlcpy() notes (was Re: [GIT PULL] smb3 client fixes)

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: <527eee19-532f-b2e7-a42f-a1e199094fbe@rasmusvillemoes.dk>
Date:   Fri, 26 Aug 2022 09:40:20 +0200
From:   Rasmus Villemoes <linux@...musvillemoes.dk>
To:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Catalin Marinas <catalin.marinas@....com>
Cc:     Steve French <smfrench@...il.com>,
        CIFS <linux-cifs@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-arch <linux-arch@...r.kernel.org>
Subject: Re: strlcpy() notes (was Re: [GIT PULL] smb3 client fixes)

On 23/08/2022 19.37, Linus Torvalds wrote:
> On Tue, Aug 23, 2022 at 1:56 AM Catalin Marinas <catalin.marinas@....com> wrote:
>>
>> With load_unaligned_zeropad(), the arm64 implementation disables tag
>> checking temporarily. We could do the same with read_word_at_a_time()
>> (there is a kasan_check_read() in this function but it wrongly uses a
>> size of 1).
> 
> The "size of 1" is not wrong, it's intentional, exactly because people
> do things like
> 
>     strscpy(dst, "string", sizeof(dst));
> 
> which is a bit unfortunate, but very understandable and intended to
> work. So that thing may over-read the string by up to a word. And
> KASAN ends up being unhappy.

So, while we're doing all the churn of replacing strlcpy anyway, may I
once again suggest we add (name can be bikeshedded) literal_strcpy():

#define literal_strcpy(d, s) ({ \
  static_assert(__same_type(d, char[]), "destination must be char array"); \
  static_assert(__same_type(s, const char[]), "source must be a string
literal"); \
  static_assert(sizeof(d) >= sizeof("" s ""), "source does not fit in
destination"); \
  strcpy(d, s); \
})

That interface _cannot_ be misused, because all the checking happens at
build time, including enforcement that the source really is a string
literal (the "" s "" trick - but for nicer error message the
static_assert on the previous line is there as well). So unlike all the
uses of str[ls]cpy which don't check the return value, we cannot
silently do a truncated copy. Also, if somebody down the line changes
the size of the destination or the literal string, again it will be
caught at build time.

And since gcc knows the semantics of strcpy(), it will also generate
better code, because it will usually not emit a call at all (or even put
the string in .rodata); it will simply emit a series of "mov immediate"
instructions.

Sloppy grepping for places where that could be used shows around ~800
places.

Btw, Steve, since you're incidentally on cc here anyway, perhaps you
want to take a look at

  strscpy(extension, "___", strlen("___"));

and see if this really just wants two underscores copied to extension.

Rasmus