From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-11.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9B260C388F7 for ; Tue, 10 Nov 2020 19:07:43 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 4239120809 for ; Tue, 10 Nov 2020 19:07:43 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1731359AbgKJTHm (ORCPT ); Tue, 10 Nov 2020 14:07:42 -0500 Received: from youngberry.canonical.com ([91.189.89.112]:51538 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726400AbgKJTHl (ORCPT ); Tue, 10 Nov 2020 14:07:41 -0500 Received: from 1.general.cking.uk.vpn ([10.172.193.212]) by youngberry.canonical.com with esmtpsa (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.86_2) (envelope-from ) id 1kcYzR-0002fO-Lu; Tue, 10 Nov 2020 19:07:37 +0000 Subject: Re: [PATCH][next] cpumask: allocate enough space for string and trailing '\0' char To: paulmck@kernel.org Cc: Paul Gortmaker , Qian Cai , Stephen Rothwell , kernel-janitors@vger.kernel.org, linux-kernel@vger.kernel.org, Linux Next Mailing List References: <20201109130447.2080491-1-colin.king@canonical.com> <737d5be9eb5af55b1a61bd8bfb49b1829a3ff916.camel@redhat.com> <20201110152437.GS3249@paulmck-ThinkPad-P72> <6050d075-52cc-d1b8-51c4-4d0dac62a42e@canonical.com> <20201110183826.GV3249@paulmck-ThinkPad-P72> From: Colin Ian King Message-ID: <431c72da-1f85-ed4f-1023-08c844d572ac@canonical.com> Date: Tue, 10 Nov 2020 19:07:37 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.4.1 MIME-Version: 1.0 In-Reply-To: <20201110183826.GV3249@paulmck-ThinkPad-P72> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 10/11/2020 18:38, Paul E. McKenney wrote: > On Tue, Nov 10, 2020 at 03:34:05PM +0000, Colin Ian King wrote: >> On 10/11/2020 15:24, Paul E. McKenney wrote: >>> On Mon, Nov 09, 2020 at 11:57:15PM -0500, Paul Gortmaker wrote: >>>> >>>> >>>> On 2020-11-09 8:07 p.m., Qian Cai wrote: >>>>> On Mon, 2020-11-09 at 13:04 +0000, Colin King wrote: >>>>>> From: Colin Ian King >>>>>> >>>>>> Currently the allocation of cpulist is based on the length of buf but does >>>>>> not include the addition end of string '\0' terminator. Static analysis is >>>>>> reporting this as a potential out-of-bounds access on cpulist. Fix this by >>>>>> allocating enough space for the additional '\0' terminator. >>>>>> >>>>>> Addresses-Coverity: ("Out-of-bounds access") >>>>>> Fixes: 65987e67f7ff ("cpumask: add "last" alias for cpu list specifications") >>>>> >>>>> Yeah, this bad commit also introduced KASAN errors everywhere and then will >>>>> disable lockdep that makes our linux-next CI miserable. Confirmed that this >>>>> patch will fix it. >>>> >>>> I appreciate the reports reminding me why I hate touching string handling. >>>> >>>> But let us not lose sight of why linux-next exists. We want to >>>> encourage code to appear there as a sounding board before it goes >>>> mainline, so we can fix things and not pollute mainline git history >>>> with those trivialities. >>>> >>>> If you've decided to internalize linux-next as part of your CI, then >>>> great, but do note that does not elevate linux-next to some pristine >>>> status for the world at large. That only means you have to watch more >>>> closely what is going on. >>>> >>>> If you want to declare linux-next unbreakable -- well that would scare >>>> away others to get the multi-arch or multi-config coverage that they may >>>> not be able to do themselves. We are not going to do that. >>>> >>>> I have (hopefully) fixed the "bad commit" in v2 -- as part of the >>>> implicit linux-next rule "you broke it, you better fix it ASAP". >>>> >>>> But "bad" and "miserable" can be things that might scare people off of >>>> making use of linux-next for what it is meant to be for. And I am not >>>> OK with that. >>> >>> They would need to use much stronger language to scare me off. That said, >>> what on earth is the point of running tests if they do not from time to >>> time find bugs? ;-) >> >> For me, part of the QA process is statically analyzing linux-next to >> catch bugs before they land in linux. I think other testing is equally >> worth while as catching bugs early saves time and money. > > All kidding aside, the fact that this appeared in -next was due to a > mistake on my part, namely failing to push the changes before starting > the test. Please accept my apologies, and I will continue to do my > best to avoid this sort of thing. > > Thanx, Paul No problem. I'm glad we have tools to catch issues like this. Colin > >> Colin >> >>> >>>> Thanks, >>>> Paul. >>>> -- >>>> >>>>> >>>>>> Signed-off-by: Colin Ian King >>>>>> --- >>>>>> lib/cpumask.c | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/lib/cpumask.c b/lib/cpumask.c >>>>>> index 34ecb3005941..cb8a3ef0e73e 100644 >>>>>> --- a/lib/cpumask.c >>>>>> +++ b/lib/cpumask.c >>>>>> @@ -185,7 +185,7 @@ int __ref cpulist_parse(const char *buf, struct cpumask >>>>>> *dstp) >>>>>> { >>>>>> int r; >>>>>> char *cpulist, last_cpu[5]; /* NR_CPUS <= 9999 */ >>>>>> - size_t len = strlen(buf); >>>>>> + size_t len = strlen(buf) + 1; >>>>>> bool early = !slab_is_available(); >>>>>> if (!strcmp(buf, "all")) { >>>>> >>