Received: by 2002:a05:6358:700f:b0:131:369:b2a3 with SMTP id 15csp337287rwo; Tue, 1 Aug 2023 18:23:44 -0700 (PDT) X-Google-Smtp-Source: APBJJlF2J7Ljd8qLomRKtuyEbISp68SSr74+dugtSQ8gmNXc6EiPDQQOCwyK9/bOZUtoE+rY971M X-Received: by 2002:a17:906:9bd0:b0:993:f9d4:eab3 with SMTP id de16-20020a1709069bd000b00993f9d4eab3mr3477248ejc.18.1690939423885; Tue, 01 Aug 2023 18:23:43 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1690939423; cv=none; d=google.com; s=arc-20160816; b=TCqmfc2ko2IdRsULFuSBBcP6sIKbE3uarFwnw81J9o7KCegXZtIvSF6V3OaW6V6I6l KSRL0cN0Nuljb23wXEQ7avlvTObylQZANF80pUqleemdFQkjMi7GECeP/ipTvF3cRUX0 TpONq/esOMWtccmLWHZ/c4/AzaSUBLwHWsRjA9AGpZGp8Qh4oApmBiKV32Ht9Lzaouk4 RiOPyReRXyjAqcXachBx8t3+hSodll2luoSQbKMcccvndDolfuiIqpXHME0ZyJHjacyJ ML0RU7dS1dWi5FoLQ4S+NM0bwsCAfRuW1wuuDH6SNYywkTP5KZ9Rlu6CBJQLRiftIZPN WWpg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:references:in-reply-to:user-agent:subject:cc:to:from :date:dkim-signature; bh=r8aem2JQB0Xg5MKuwU3hZpe82X9yXlF9tUqr28+n9AA=; fh=tCOkVaar7KDHiF8FJpw/pHE/C1wiJhb/lEXR3KAX6yQ=; b=aJQ8eClFHsYOv5RCLcgzlV06QK5Rww/iweySL+D3hSIl2wHp6J+U+0UwTN7OTJNii8 RQeV4h9culUHrPwMEuJXOC1C+5Yy4DtD6rkeGrCu0NF9u0dzdv4hI6CkbvhDXjge3imz CV5lxV7fvlwAgHyMVpXHDUM+5GeHY0vvXm2bGrEZDVHQc7X+cXn7F5ZrW4zZPkMF8lnb a8cK6eIWkfZSgJDGWq6KmbW+2LcNmhVc69VfNshMexQlufFC7O9td96xflOuFUVqWhvj 1epTaNXKaA4EybazilDbqaV9sx6cMfePgYsi/F62mlAEB1VshssdjNki/jMiqTC+vlMu tLIA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=tBbtTP9C; spf=pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-wireless-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Return-Path: Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id qp3-20020a170907206300b0097885fd66f4si313476ejb.162.2023.08.01.18.23.22; Tue, 01 Aug 2023 18:23:43 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=tBbtTP9C; spf=pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-wireless-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231304AbjHBBQd (ORCPT + 58 others); Tue, 1 Aug 2023 21:16:33 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:33464 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229628AbjHBBQb (ORCPT ); Tue, 1 Aug 2023 21:16:31 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 38BFB270E; Tue, 1 Aug 2023 18:16:26 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id CA83C61781; Wed, 2 Aug 2023 01:16:25 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id CF80FC433C9; Wed, 2 Aug 2023 01:16:24 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1690938985; bh=dRzi47V4F6qPxi+cL2DryDVpLxM7n0DuJBJlfEIFBIg=; h=Date:From:To:CC:Subject:In-Reply-To:References:From; b=tBbtTP9CBi915qLC9iXHCjyT7rc3f8Nk+Hcr/mS1PKSKmi2Dfsg2PosL1W+bsB9QA CRiKPbMjCmcGJX4Hriop8MBFOX2XnNdhExe0WDQf5C470iLQhX2LZArTarFZsgmeBI 0TQIjuzxfbF8+GFWmPOM2UtUX3OWuNT6whAgQ3wQ4T5ebe3k4VLb1MAqW5G329f87G esFhZZVmOobzuuVQFus4EKfcRQypshHOaUhGIsVf+Is7htcEFukhTvO5EXze9dDqwy kACuaP5bkxAvXbgJ42ixxUAVnoiwt3vgNR8XQfPqgnyIgNGIKqfOcmRDfDuQSheX+L rHulLxjQpv3OQ== Date: Tue, 01 Aug 2023 18:16:18 -0700 From: Kees Cook To: Justin Stitt , Stanislav Yakovlev , Kalle Valo CC: Kees Cook , linux-wireless@vger.kernel.org, linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org Subject: Re: [PATCH] wifi: ipw2x00: refactor to use kstrtoul User-Agent: K-9 Mail for Android In-Reply-To: <20230802-wifi-ipw2x00-refactor-v1-1-6047659410d4@google.com> References: <20230802-wifi-ipw2x00-refactor-v1-1-6047659410d4@google.com> Message-ID: <86B2408D-BB52-4B29-BE4A-A6A12A129121@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-7.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_HI, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-wireless@vger.kernel.org On August 1, 2023 5:51:59 PM PDT, Justin Stitt w= rote: >The current implementation seems to reinvent what `kstrtoul` already does >in terms of functionality and error handling=2E Remove uses of `simple_st= rtoul()` >in favor of `kstrtoul()`=2E > >There is the following note at `lib/vsprintf=2Ec:simple_strtoull()` which >further backs this change: >| * This function has caveats=2E Please use kstrtoull (or kstrtoul) inste= ad=2E > >And here, simple_str* are explicitly deprecated [3]=2E > >This patch also removes an instance of the deprecated `strncpy` which hel= ps [2]=2E > >Link: https://lore=2Ekernel=2Eorg/all/202308011602=2E3CC1C0244C@keescook/= [1] >Link: https://github=2Ecom/KSPP/linux/issues/90 [2] >Link: https://docs=2Ekernel=2Eorg/process/deprecated=2Ehtml#simple-strtol= -simple-strtoll-simple-strtoul-simple-strtoull [3] >Cc: linux-hardening@vger=2Ekernel=2Eorg >Suggested-by: Kees Cook >Signed-off-by: Justin Stitt >--- > > >Link: https://lore=2Ekernel=2Eorg/all/20230801-drivers-net-wireless-intel= -ipw2x00-v1-1-ffd185c91292@google=2Ecom/ >--- > drivers/net/wireless/intel/ipw2x00/ipw2200=2Ec | 43 +++++++++-----------= -------- > 1 file changed, 14 insertions(+), 29 deletions(-) > >diff --git a/drivers/net/wireless/intel/ipw2x00/ipw2200=2Ec b/drivers/net= /wireless/intel/ipw2x00/ipw2200=2Ec >index dfe0f74369e6=2E=2Eac10633f593e 100644 >--- a/drivers/net/wireless/intel/ipw2x00/ipw2200=2Ec >+++ b/drivers/net/wireless/intel/ipw2x00/ipw2200=2Ec >@@ -1176,23 +1176,20 @@ static ssize_t debug_level_show(struct device_dri= ver *d, char *buf) > static ssize_t debug_level_store(struct device_driver *d, const char *bu= f, > size_t count) > { >- char *p =3D (char *)buf; >- u32 val; >+ unsigned long *val =3D NULL; >=20 >- if (p[1] =3D=3D 'x' || p[1] =3D=3D 'X' || p[0] =3D=3D 'x' || p[0] =3D= =3D 'X') { >- p++; >- if (p[0] =3D=3D 'x' || p[0] =3D=3D 'X') >- p++; >- val =3D simple_strtoul(p, &p, 16); >- } else >- val =3D simple_strtoul(p, &p, 10); >- if (p =3D=3D buf) >+ int result =3D kstrtoul(buf, 0, val); kstrtoul needs somewhere to write the value, so val need to be actually un= signed long, and a pointer passed to that: unsigned long val; =2E=2E=2E =2E=2E=2E kstrtoul(but, 0, &val); But otherwise, yeah, this looks like the right direction to me=2E >+ >+ if (result =3D=3D -EINVAL) > printk(KERN_INFO DRV_NAME > ": %s is not in hex or decimal form=2E\n", buf); >+ else if (result =3D=3D -ERANGE) >+ printk(KERN_INFO DRV_NAME >+ ": %s has overflowed=2E\n", buf); > else >- ipw_debug_level =3D val; >+ ipw_debug_level =3D *val; >=20 >- return strnlen(buf, count); >+ return count;=2E It might be worth mentioning this return value change, but I think it's co= rrect: we're communicating how much was consumed (we consumed it all)=2E Wh= en the return value !=3D count, this function may be called again with the = "rest" of the input=2E As this is a sysfs interface, that kind of behavior = is very rare bordering on actively unwanted=2E :) So, I think these should = either return a negative error or count=2E -Kees > } > static DRIVER_ATTR_RW(debug_level); >=20 >@@ -1461,33 +1458,21 @@ static ssize_t scan_age_store(struct device *d, s= truct device_attribute *attr, > { > struct ipw_priv *priv =3D dev_get_drvdata(d); > struct net_device *dev =3D priv->net_dev; >- char buffer[] =3D "00000000"; >- unsigned long len =3D >- (sizeof(buffer) - 1) > count ? count : sizeof(buffer) - 1; >- unsigned long val; >- char *p =3D buffer; >=20 > IPW_DEBUG_INFO("enter\n"); >=20 >- strncpy(buffer, buf, len); >- buffer[len] =3D 0; >+ unsigned long *val =3D NULL; >+ int result =3D kstrtoul(buf, 0, val); >=20 >- if (p[1] =3D=3D 'x' || p[1] =3D=3D 'X' || p[0] =3D=3D 'x' || p[0] =3D= =3D 'X') { >- p++; >- if (p[0] =3D=3D 'x' || p[0] =3D=3D 'X') >- p++; >- val =3D simple_strtoul(p, &p, 16); >- } else >- val =3D simple_strtoul(p, &p, 10); >- if (p =3D=3D buffer) { >+ if (result =3D=3D -EINVAL || result =3D=3D -ERANGE) { > IPW_DEBUG_INFO("%s: user supplied invalid value=2E\n", dev->name); > } else { >- priv->ieee->scan_age =3D val; >+ priv->ieee->scan_age =3D *val; > IPW_DEBUG_INFO("set scan_age =3D %u\n", priv->ieee->scan_age); > } >=20 > IPW_DEBUG_INFO("exit\n"); >- return len; >+ return count; > } >=20 > static DEVICE_ATTR_RW(scan_age); > >--- >base-commit: 5d0c230f1de8c7515b6567d9afba1f196fb4e2f4 >change-id: 20230801-wifi-ipw2x00-refactor-fa6deb6c67ea > >Best regards, >-- >Justin Stitt > --=20 Kees Cook