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 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 77312C433F5 for ; Wed, 4 May 2022 15:08:36 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1351905AbiEDPMJ (ORCPT ); Wed, 4 May 2022 11:12:09 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:33628 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1351910AbiEDPMF (ORCPT ); Wed, 4 May 2022 11:12:05 -0400 Received: from mail-pl1-x633.google.com (mail-pl1-x633.google.com [IPv6:2607:f8b0:4864:20::633]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 9DB4B37A13 for ; Wed, 4 May 2022 08:08:28 -0700 (PDT) Received: by mail-pl1-x633.google.com with SMTP id n18so1665661plg.5 for ; Wed, 04 May 2022 08:08:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=Qb+1GrJs8SRgE6Wq+GlmzxuejfTqdKZ3b89DzJavSQw=; b=QW7CQXai9IiD71Wvf+o2VYxsrI+DMXmN1vsS0b5DV0rtrX+G8r3eGzQnrLgcSNLS6o 18VjexLmt9t17aqS0qc4jCF5CPslGIy/SB1vEauI+iOiw3aemKigwzjm7wU//Osg3Pbt 9wMF/4HUhAaX0invtmDD41DdrKFbACP1VK8u0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=Qb+1GrJs8SRgE6Wq+GlmzxuejfTqdKZ3b89DzJavSQw=; b=KymZy0hHSYbvUoYo7MRZ1K6weCR47YBDGZnobqYx1738zWnQPMfV8NqNwu2HxwJDsz mSbUg5umgX6bTP6EYMj5iGjqfVvlYv9nJJwKEurO5DLTMJfae7lQVettH+WHo3kA0rcG 1xHc05zpS3XqAmzXzc/jLrSZyjFFKIa8c+FQdHBeL5wIkv/0HGuWqVjYurTms9uagQ0X b5IV/TsJldBYx9ltBY2INYR80lUA23UwWeGoRcNptuMlmmSU1OtVZ+2glr6mx8xuFL7J qwdZH4Bxn7tthnANkPq+TiAmfgEHDBZxT+isf/TvJrq8wnuAYdXr+bLGbt1XyqEHPcRj Ki9Q== X-Gm-Message-State: AOAM532/hd3yvS8WEAFMw8JrBpIYKG1sk/p5H/CP7hr+Wmz6C6Am4Yq6 NnKjL6QjKCzM2MSxqJqVQLg4jg== X-Google-Smtp-Source: ABdhPJxppSZTl5CKjnq8a5hBmltXmdlckYUA+zJjxmwbiUJP9s+fG0NYe/ezZfchAEYBzvKm6cdN0g== X-Received: by 2002:a17:90a:e7d2:b0:1dc:3762:c72d with SMTP id kb18-20020a17090ae7d200b001dc3762c72dmr10809021pjb.243.1651676908004; Wed, 04 May 2022 08:08:28 -0700 (PDT) Received: from www.outflux.net (smtp.outflux.net. [198.145.64.163]) by smtp.gmail.com with ESMTPSA id q10-20020a170902bd8a00b0015e8d4eb2c8sm8430976pls.274.2022.05.04.08.08.27 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 04 May 2022 08:08:27 -0700 (PDT) Date: Wed, 4 May 2022 08:08:26 -0700 From: Kees Cook To: Kalle Valo Cc: "Gustavo A . R . Silva" , Loic Poulain , "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , wcn36xx@lists.infradead.org, linux-wireless@vger.kernel.org, netdev@vger.kernel.org, Alexei Starovoitov , alsa-devel@alsa-project.org, Al Viro , Andrew Gabbasov , Andrew Morton , Andy Gross , Andy Lavr , Arend van Spriel , Baowen Zheng , Bjorn Andersson , Boris Ostrovsky , Bradley Grove , brcm80211-dev-list.pdl@broadcom.com, Christian Brauner , Christian =?iso-8859-1?Q?G=F6ttsche?= , Christian Lamparter , Chris Zankel , Cong Wang , Daniel Axtens , Daniel Vetter , Dan Williams , David Gow , David Howells , Dennis Dalessandro , devicetree@vger.kernel.org, Dexuan Cui , Dmitry Kasatkin , Eli Cohen , Eric Paris , Eugeniu Rosca , Felipe Balbi , Francis Laniel , Frank Rowand , Franky Lin , Greg Kroah-Hartman , Gregory Greenman , Guenter Roeck , Haiyang Zhang , Hante Meuleman , Herbert Xu , Hulk Robot , "James E.J. Bottomley" , James Morris , Jarkko Sakkinen , Jaroslav Kysela , Jason Gunthorpe , Jens Axboe , Johan Hedberg , Johannes Berg , Johannes Berg , John Keeping , Juergen Gross , Keith Packard , keyrings@vger.kernel.org, kunit-dev@googlegroups.com, Kuniyuki Iwashima , "K. Y. Srinivasan" , Lars-Peter Clausen , Lee Jones , Leon Romanovsky , Liam Girdwood , linux1394-devel@lists.sourceforge.net, linux-afs@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linux-arm-msm@vger.kernel.org, linux-bluetooth@vger.kernel.org, linux-hardening@vger.kernel.org, linux-hyperv@vger.kernel.org, linux-integrity@vger.kernel.org, linux-rdma@vger.kernel.org, linux-scsi@vger.kernel.org, linux-security-module@vger.kernel.org, linux-usb@vger.kernel.org, linux-xtensa@linux-xtensa.org, llvm@lists.linux.dev, Louis Peens , Luca Coelho , Luiz Augusto von Dentz , Marcel Holtmann , Mark Brown , "Martin K. Petersen" , Max Filippov , Mimi Zohar , Muchun Song , Nathan Chancellor , Nick Desaulniers , Nuno =?iso-8859-1?Q?S=E1?= , Paul Moore , Rich Felker , Rob Herring , Russell King , selinux@vger.kernel.org, "Serge E. Hallyn" , SHA-cyfmac-dev-list@infineon.com, Simon Horman , Stefano Stabellini , Stefan Richter , Steffen Klassert , Stephen Hemminger , Stephen Smalley , Tadeusz Struk , Takashi Iwai , Tom Rix , Udipto Goswami , Vincenzo Frascino , Wei Liu , xen-devel@lists.xenproject.org, Xiu Jianfeng , Yang Yingliang Subject: Re: [PATCH 10/32] wcn36xx: Use mem_to_flex_dup() with struct wcn36xx_hal_ind_msg Message-ID: <202205040730.161645EC@keescook> References: <20220504014440.3697851-1-keescook@chromium.org> <20220504014440.3697851-11-keescook@chromium.org> <8735hpc0q1.fsf@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <8735hpc0q1.fsf@kernel.org> Precedence: bulk List-ID: X-Mailing-List: linux-bluetooth@vger.kernel.org On Wed, May 04, 2022 at 08:42:46AM +0300, Kalle Valo wrote: > Kees Cook writes: > > > As part of the work to perform bounds checking on all memcpy() uses, > > replace the open-coded a deserialization of bytes out of memory into a > > trailing flexible array by using a flex_array.h helper to perform the > > allocation, bounds checking, and copying. > > > > Cc: Loic Poulain > > Cc: Kalle Valo > > Cc: "David S. Miller" > > Cc: Eric Dumazet > > Cc: Jakub Kicinski > > Cc: Paolo Abeni > > Cc: wcn36xx@lists.infradead.org > > Cc: linux-wireless@vger.kernel.org > > Cc: netdev@vger.kernel.org > > Signed-off-by: Kees Cook > > [...] > > > --- a/drivers/net/wireless/ath/wcn36xx/smd.h > > +++ b/drivers/net/wireless/ath/wcn36xx/smd.h > > @@ -46,8 +46,8 @@ struct wcn36xx_fw_msg_status_rsp { > > > > struct wcn36xx_hal_ind_msg { > > struct list_head list; > > - size_t msg_len; > > - u8 msg[]; > > + DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(size_t, msg_len); > > + DECLARE_FLEX_ARRAY_ELEMENTS(u8, msg); > > This affects readability quite a lot and tbh I don't like it. Isn't > there any simpler way to solve this? Similar to how I plumbed member names into __mem_to_flex(), I could do the same for __mem_to_flex_dup(). That way if the struct member aliases (DECLARE_FLEX...) aren't added, the longer form of the helper could be used. Instead of: if (mem_to_flex_dup(&msg_ind, buf, len, GFP_ATOMIC)) { it would be: if (__mem_to_flex_dup(&msg_ind, /* self */, msg, msg_len, buf, len, GFP_ATOMIC)) { This was how I'd written the helpers in an earlier version, but it seemed much cleaner to avoid repeating structure layout details at each call site. I couldn't find any other way to encode the needed information. It'd be wonderful if C would let us do: struct wcn36xx_hal_ind_msg { struct list_head list; size_t msg_len; u8 msg[msg_len]; } And provide some kind of interrogation: __builtin_flex_array_member(msg_ind) -> msg_ind->msg __builtin_flex_array_count(msg_ind) -> msg_ind->msg_len My hope would be to actually use the member aliases to teach things like -fsanitize=array-bounds about flexible arrays. If it encounters a structure with the aliases, it could add the instrumentation to do the bounds checking of things like: msg_ind->msg[42]; /* check that 42 is < msg_ind->msg_len */ I also wish I could find a way to make the proposed macros "forward portable" into proposed C syntax above, but this eluded me as well. For example: struct wcn36xx_hal_ind_msg { size_t msg_len; struct list_head list; BOUNDED_FLEX_ARRAY(u8, msg, msg_len); } #ifdef CC_HAS_DYNAMIC_ARRAY_LEN # define BOUNDED_FLEX_ARRAY(type, name, bounds) type name[bounds] #else # define BOUNDED_FLEX_ARRAY(type, name, bounds) \ magic_alias_of msg_len __flex_array_elements_count; \ union { \ type name[]; \ type __flex_array_elements[]; \ } #endif But I couldn't sort out the "magic_alias_of" syntax that wouldn't force structures into having the count member immediately before the flex array, which would impose more limitations on where this could be used... Anyway, I'm open to ideas on how to improve this! -Kees -- Kees Cook