From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753120AbdLENib (ORCPT ); Tue, 5 Dec 2017 08:38:31 -0500 Received: from mail-ot0-f193.google.com ([74.125.82.193]:43414 "EHLO mail-ot0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752126AbdLENi3 (ORCPT ); Tue, 5 Dec 2017 08:38:29 -0500 X-Google-Smtp-Source: AGs4zMZHaTY2NhxxNchjePQMiJeQ7VMPudOFPKldd0ufUp4Ro5Zyh+OfvBIrrlLO2Lvuu6qNcTJd8AL5swAjaCVFGU8= MIME-Version: 1.0 In-Reply-To: References: <20171204144916.453471-1-arnd@arndb.de> From: Arnd Bergmann Date: Tue, 5 Dec 2017 14:38:28 +0100 X-Google-Sender-Auth: ut7FJM0XTn9jdeKjKYr_u3U9nEQ Message-ID: Subject: Re: [PATCH] exec: avoid gcc-8 warning for get_task_comm To: Kees Cook Cc: Alexander Viro , Serge Hallyn , James Morris , Ingo Molnar , Andrew Morton , "Eric W. Biederman" , "linux-fsdevel@vger.kernel.org" , LKML Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Dec 4, 2017 at 7:37 PM, Kees Cook wrote: > On Mon, Dec 4, 2017 at 6:49 AM, Arnd Bergmann wrote: >> gcc-8 warns about using strncpy() with the source size as the limit: >> >> fs/exec.c:1223:32: error: argument to 'sizeof' in 'strncpy' call is the same expression as the source; did you mean to use the size of the destination? [-Werror=sizeof-pointer-memaccess] >> >> This is indeed slightly suspicious, as it protects us from source >> arguments without NUL-termination, but does not guarantee that the >> destination is terminated. >> >> This changes it to strlcpy with a hardcoded length, to guarantee >> a properly terminated string. Since we already use strlcpy() for >> __set_task_comm(), the source should always be terminated properly, > > It's terminated but it's not padded. This change _might_ result in > leaving a string buffer uninitialized (and if that buffer is passed as > a whole instead of just its strlen(), this could be a problem: > proc_comm_connector() e.g. may suffer from this, I haven't checked > closely). > >> so this patch won't change the behavior, but make it a bit more robust. > > I would rather we change get_task_comm() to include the size of buf... > maybe just convert it to a macro: > > -char *get_task_comm(char *buf, struct task_struct *tsk) > +char *__get_task_comm(char *buf, size_t buf_size, struct task_struct *tsk) > > ... > > #define get_task_comm(buf, tsk) __get_task_comm(buf, sizeof(buf), tsk) > > ? > > That will only work for preallocated arrays though. Maybe a build bug > when sizeof(buf) != TASK_COMM_LEN ? Interesting idea, I'll give that a try. > I think the strncpy() should stay, but for other conversions where > NULL-padding isn't an issue, strscpy() is likely preferred to > strlcpy() to avoid the potential "overread" due to the strlen() call > in strlcpy(). Ok, I'll keep that in mind if I do more of those conversions. gcc-8 has started warning about over 100 drivers that use strncpy() incorrectly, and I suppose a lot of them should actually use strscpy(). Arnd