Subject: Re: [PATCH] lib/string: avoid reading beyond src buffer in strscpy
To: Kees Cook <keescook@chromium.org>, Dmitry Vyukov <dvyukov@google.com>
Cc: Eryu Guan <eguan@redhat.com>, LKML <linux-kernel@vger.kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Chris Metcalf <cmetcalf@ezchip.com>,
        Alexander Potapenko <glider@google.com>,
        Linus Torvalds <torvalds@linux-foundation.org>
References: <20171207113324.24388-1-eguan@redhat.com>
 <CAGXu5jKji0Kvp0CL1YJT6oCqxPa2KfdhMD_Pi8oAnpwMc_sXLw@mail.gmail.com>
 <9f0a9cf6-51f7-cd1f-5dc6-6d510a7b8ec4@virtuozzo.com>
 <CACT4Y+YGacuRHOSM1r0+FQNMg2PcLgvUYTUTX9sS9NSqE3dYBQ@mail.gmail.com>
 <CAGXu5jJdD5u7fNd2Pnbv77fJwgCvZmOVxQLketMj5PuWs5Q1Tg@mail.gmail.com>
From: Andrey Ryabinin <aryabinin@virtuozzo.com>
Message-ID: <46584b52-f2f2-a602-1ae6-cfa0e321324a@virtuozzo.com>
Date: Mon, 11 Dec 2017 19:44:16 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.5.0
MIME-Version: 1.0
In-Reply-To: <CAGXu5jJdD5u7fNd2Pnbv77fJwgCvZmOVxQLketMj5PuWs5Q1Tg@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Dec 2017 16:40:39.8381 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 4576ff65-87c8-4580-c0cf-08d540b5ea4a
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 0bc7f26d-0264-416e-a6fc-8352af79c58f
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM4PR08MB2819
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 2065
Lines: 45

On 12/08/2017 11:54 PM, Kees Cook wrote:
> On Fri, Dec 8, 2017 at 7:29 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
>> On Fri, Dec 8, 2017 at 4:29 PM, Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
>>>
>>> So, possible solutions are:
>>>
>>> 1) Simply disable word-at-a-time optimization in strscpy(). I seriously doubt
>>> that this optimization have noticeable performance impact in real workloads.
>>>
>>> 2) Apply patch https://lkml.kernel.org/r/20170718171523.32208-1-aryabinin@virtuozzo.com
>>> It's a simple, minimally intrusive way to fix the bug.
>>> However, the patch changes semantics/implementation of the strscpy() which is bad idea.
>>> It basically means that we use one implementation of the strscpy() but test with KASAN another implementation.
>>> For that reason patch wasn't liked by Linus.
>>>
>>> 3) Introduce read_once_partial_check() function and use it in strscpy().
>>> read_once_partial_check() supposed to read one word, but KASAN will check only the first byte of the access.
>>> Dmitry didn't like this. I'm also not fan of this solution, because we may not notice some real bugs, e.g.:
>>>
>>>         char dst[8];
>>>         char *src;
>>>
>>>         src = kmalloc(5, GFP_KERNEL);
>>>         memset(src, 0xff, 5);
>>>         strscpy(dst, src, 8);
>>>
>>> stscpy() will copy from 6 up to 8 bytes (exact size depends on what is stored in src[5-7])
>>> from non-null terminated src. With read_once_partial_check() used in strscpy() KASAN will
>>> not report such bug.
>>>
>>>
>>> So, what it's gonna be? Let's finally decide something and deal with the problem.
>>> My vote belongs to 1) or 2).
>>
>>
>> My vote is for 1) if we agree that the optimization is not worth it,
>> otherwise for 2).
> 
> Who would be the best person to measure the difference for 1)?
> 

I suppose that depends on which one strscpy() caller you'd want to test.
Briefly looking at all current users, it doesn't look like they process huge amounts
of data through strscpy(), thus we shouldn't suffer from a slight
performance degradation of strscpy().