From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-lf1-f48.google.com (mail-lf1-f48.google.com [209.85.167.48]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id BD5DF15FA85 for ; Fri, 5 Apr 2024 09:51:48 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.167.48 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1712310711; cv=none; b=otFiJ6OdPcxnnndzaFP3GH0Kacctf48ynnX0uQfQ4ic7CS/HxmskwEjaqI5p1krVYu4xk0WUIAZG+LY5t9PmFfv9iK19SlNvqezZvxiOpGZxjocLpQ6bEvuPVSp2ihYIX9HqKdCy5EiDgntECeQE9S7C0kuOhl1C2sQ/tXF1M8g= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1712310711; c=relaxed/simple; bh=9z7NXciC8Z8/xPDHcNCTcqyOvDYKe+Q//7hKlc2E1fw=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=UecPtUEQQEKkMlFkEBjbrx6DHNeuhjnCS3P8mvI6I+oD+UbORDBDro75kbKplpfpFZelBysSNX1Rtld6WWKvIeb/YqCdCG0OLc6dLRtUk6lJ9YKGpCeEJSLXSvhGe+yyX6MNlCz7ZndwGUyBnaiF0ZoPrtYVtihx3Yoq3uto4wc= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org; spf=pass smtp.mailfrom=linaro.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b=JbrVApFv; arc=none smtp.client-ip=209.85.167.48 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linaro.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="JbrVApFv" Received: by mail-lf1-f48.google.com with SMTP id 2adb3069b0e04-516ab4b3251so2301407e87.0 for ; Fri, 05 Apr 2024 02:51:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1712310707; x=1712915507; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=XznLbi2Z1SKvx8bRj0+Hw75ngrOt6iwbmtsVcIRJxZA=; b=JbrVApFvipQKIx68+/9lcIy+ybhZTcBFyUjgjy2xBmPRu4qMbBzM6rAcCU+E8d5FPo VoUCbGKNK+ADHf0MtrP7kwdtCj4V3MNTMV8zYJonX0uKLekbHAIV+GCQXfix/qeFmSPb YzchkGUk/SJ6/w7o+EPq327QeNW5H/GeuzcYgYB/CYhJ8CJ4Z6gMlp23vJ/rtlcZ864x jEHLOW2EAUekATvZoNevH6dqjWd9B7tIWTJv5OmjVZfqTSdjfzkum7BAQmWKT0QPX4Qs 1LijJde4lV6ObRX3g621WBOHxMJJj+DyJVol/10XapQyhtdKqBQQvH8hgEulcsMEMK94 eOMQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1712310707; x=1712915507; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=XznLbi2Z1SKvx8bRj0+Hw75ngrOt6iwbmtsVcIRJxZA=; b=rs2ioD/owMMqHPa7NVcoOr3jrO9ChJoH3D8uJ9qfiqUdJKFDhwynODsDqHyqpbpMv4 a4ZgmQuiws+weyBWg14hMMG0kvGRziMdZ+wJU7UBf6cuB0DFgtJqSEl2D4Cq39NDKF+B k0RDTTLN5WiScb50eeX2GD08BT8tJR7Tzu3GlHtUorUdUftHCkVw2pGkQoyKCFwDPg8V ztrt7GIr4/O0wqWPdYHRc+FXVY85a2Wlcy3c5/B/0XylhhkdD+uYs66HGvlWj17DdJNc A+QECw6/QLfNHCrlUHKxLv9yytW4MC1HaK1LK/FvV4OmzST0HBl0XW00cP6hFU/YLp0V X9ow== X-Forwarded-Encrypted: i=1; AJvYcCW3KPEiPclI8AGUbSwxrcw4NkmTb/AdfNPXq1QpVX5/HruVGgejYLwdrpinfnokuUVyeO3DuvPFsHQEjLvBAMIf71MMb/KxB6slZsQbrmTL X-Gm-Message-State: AOJu0Yw276gJQL2/Dt/DKDLxZoFdkplyJHcobWV0pix1qbHK35a5BMUV WjiPwF/wlmbvb1RVj8fryBnIXKm/WyToVUzs41f6wIM3wtvEgWGA+O9TwdEHcK7Tc90nmFMiada 6 X-Google-Smtp-Source: AGHT+IFuRBcasfkE1HoHSCQ7OdWv+EtlpW9OQz09tguqfMAKrRd1NUyHNpjIOZuZmN0l9ZISYTgUFw== X-Received: by 2002:ac2:5ddb:0:b0:516:d2eb:6edd with SMTP id x27-20020ac25ddb000000b00516d2eb6eddmr735020lfq.26.1712310706830; Fri, 05 Apr 2024 02:51:46 -0700 (PDT) Received: from aspen.lan (aztw-34-b2-v4wan-166919-cust780.vm26.cable.virginm.net. [82.37.195.13]) by smtp.gmail.com with ESMTPSA id e28-20020adfa45c000000b00343e1c3298asm1428854wra.0.2024.04.05.02.51.45 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 05 Apr 2024 02:51:46 -0700 (PDT) Date: Fri, 5 Apr 2024 10:51:44 +0100 From: Daniel Thompson To: Justin Stitt Cc: Jason Wessel , Douglas Anderson , kgdb-bugreport@lists.sourceforge.net, linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org Subject: Re: [PATCH v2] kdb: replace deprecated strncpy Message-ID: <20240405095144.GB2890893@aspen.lan> References: <20240405-strncpy-kernel-debug-kdb-kdb_io-c-v2-1-d0bf595ab301@google.com> Precedence: bulk X-Mailing-List: linux-hardening@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240405-strncpy-kernel-debug-kdb-kdb_io-c-v2-1-d0bf595ab301@google.com> On Fri, Apr 05, 2024 at 02:33:58AM +0000, Justin Stitt wrote: > We should move away from using strncpy because it is deprecated [1]. > > Since these buffers want to be NUL-terminated, let's use strscpy() which > guarantees this behavior. > > The code in question enables the visual autocomplete when using kdb tab > completion. After pressing tab a couple times when sitting on a partial > symbol it will attempt to fill it in. FWIW the code that this patch changes is only executed when tab is pressed once. > In my testing, strscpy() provides > the exact same autocomplete behavior that strncpy() provides here (i.e: > it fills in the same number of characters for the user). > > You can confirm this by enabling kdb [3] and booting up the kernel. I > performed my tests with qemu with this incantation (wow these get long): > > $ /usr/bin/qemu-system-x86_64 -display none -nodefaults -cpu Nehalem > -append 'console=ttyS0,115200 earlycon=uart8250,io,0x3f8 rdinit=/bin/sh > kgdboc=ttyS0,115200 nokaslr' -kernel $BUILD_DIR/arch/x86/boot/bzImage > -initrd $REPOS/boot-utils/images/x86_64/rootfs.cpio -m 512m -serial > mon:stdio > > ... then you can type some symbols and see that autocomplete still kicks > in and performs exactly the same. > > For example: > tes gives you "test", > then "test_ap" gives you "test_aperfmperf" > > Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] > Link: https://github.com/KSPP/linux/issues/90 [2] > Link: https://www.kernel.org/doc/html/v5.0/dev-tools/kgdb.html#using-kdb [3] > Cc: linux-hardening@vger.kernel.org > Signed-off-by: Justin Stitt > --- > Changes in v2: > - use strscpy over memcpy (thanks Daniel T.) > - Link to v1: https://lore.kernel.org/r/20240403-strncpy-kernel-debug-kdb-kdb_io-c-v1-1-7f78a08e9ff4@google.com > --- > --- > kernel/debug/kdb/kdb_io.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c > index 9443bc63c5a2..60be22132020 100644 > --- a/kernel/debug/kdb/kdb_io.c > +++ b/kernel/debug/kdb/kdb_io.c > @@ -368,9 +368,9 @@ static char *kdb_read(char *buffer, size_t bufsize) > kdb_printf("%s", buffer); > } else if (tab != 2 && count > 0) { > len_tmp = strlen(p_tmp); > - strncpy(p_tmp+len_tmp, cp, lastchar-cp+1); > + strscpy(p_tmp+len_tmp, cp, lastchar-cp+1); This still looks like it is reproducing the obvious[1] error in the original strncpy() call. The third argument does *not* provide the number of characters in the destination buffer. Just to be really clear, I think this patch and your original memcpy() conversion is mechanically correct, in that the new code is equivalent to the original strncpy(). The problem is that neither patch acts on the warning signs that the original code is broken. [1] I know that this code is hard to read so "obvious" is a relative term. However just looking at one line tells us that the source pointer is part of a two pointer calculation that purports to give the length of the destination string! Such code is almost always going to be wrong. > len_tmp = strlen(p_tmp); > - strncpy(cp, p_tmp+len, len_tmp-len + 1); > + strscpy(cp, p_tmp+len, len_tmp-len + 1); Again, I really don't think the third argument provides the number of characters in the destination buffer. Daniel.