Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <b63a8f5b-0bc3-3a27-416e-d0943cb0d147@intel.com>
Date:   Tue, 1 Aug 2023 15:08:17 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH net-next 1/3] virtchnl: fix fake 1-elem arrays in structs
 allocated as `nents + 1` - 1
To:     Kees Cook <keescook@chromium.org>
CC:     "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Larysa Zaremba <larysa.zaremba@intel.com>,
        "Andy Shevchenko" <andriy.shevchenko@linux.intel.com>,
        "Gustavo A. R. Silva" <gustavoars@kernel.org>,
        <netdev@vger.kernel.org>, <linux-hardening@vger.kernel.org>,
        <intel-wired-lan@lists.osuosl.org>, <linux-kernel@vger.kernel.org>
References: <20230728155207.10042-1-aleksander.lobakin@intel.com>
 <20230728155207.10042-2-aleksander.lobakin@intel.com>
 <202307281537.AC1ED9CA@keescook>
Content-Language: en-US
From:   Alexander Lobakin <aleksander.lobakin@intel.com>
In-Reply-To: <202307281537.AC1ED9CA@keescook>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?UWZabVJLSHowMVRsQjVKMEUzK0s4L3JMdEJtQit3M24xUWl0dXIwalBUTGpn?=
 =?utf-8?B?bkxRL3R3Y3lKMFZjWC80bnhDcS9DeWVpbGJoZlozQXF2MXdvRmVTR3lpclpQ?=
 =?utf-8?B?aU91VkVFNEhia0JVYXhFQW0zL1JvS0dZcTVqMUVNRk8vU01CZUNxY2s2dk40?=
 =?utf-8?B?M1ovUkpmYWJlQjdkaDV4L3p3dmZSQVJLNnRMMGdOd2h2K24yeHZPbTZWRjFZ?=
 =?utf-8?B?aXhNVEhwSDhJTlA0ZkROYk8yVElFSC9vcklLWU5Fc1dWQWRINEdzNnNjV3Bi?=
 =?utf-8?B?VkpORi9CeWtDTU9nZEFxdnd6NkdpRDhQL1ZJWEdBaTkraE0rNHJxYktOUUVQ?=
 =?utf-8?B?cGkwNEV3QmNmWmtod1JYQ1VlUnhpdjJLRzliNnNwVk5PckphTXZIR2JMUGFz?=
 =?utf-8?B?TlBYaUJPcFMyVU9KcForYnFLTFlkZkdkUmdaOFUxb1I2TEhOaG05OUdWT0Zk?=
 =?utf-8?B?RGVPc01oRStHb0NWU2Z3WVg2SlhQMk1xQmpNTjV0QXRwZERReHN1d0RaMnM0?=
 =?utf-8?B?ajhxY09CZEwyWGJDWjN5cy9RL3kxR3VyTDdVRWViUW9tRTA1QUJaQ3VJK0ll?=
 =?utf-8?B?ZGkyNGpPVERSTTQvRXZSUkIxQVUyUnBEMXVCcEg2amswUkdEakZZeWQwSFFR?=
 =?utf-8?B?MXkyRWRHb2ZkbVp1TFg2eDZQckZOYVM3WDN5QkhSUmQ3NnRxRmdxbExrcEtx?=
 =?utf-8?B?Uyt0aFUvclRRL1I5STExYUN5MGpnRWV6SjBHNzBaMjI1d3h1STdJQ3hGZWsv?=
 =?utf-8?B?UkoybVB0VXdRM3pOWFVMbEh5cnE3TGs5YnhwREFJQUd0YTFNbWV3UU9lZCtL?=
 =?utf-8?B?WmJjK04vZzliWGtvYjNveDg4NitISzhjMlFYM00wR0x5T1hnVjJCV3BKOTdW?=
 =?utf-8?B?TlZld0V2ZEJrN1k3a0NUM3hvclZ2MDlvTHJHVUZGdU5odTdNVTRBaERyRmlU?=
 =?utf-8?B?UnBCRjFZejZmTnlKMzVleUNjclFiQmxjOC9lNjFXd3NMTEdYRnFidzdGM1ls?=
 =?utf-8?B?WTJ0cVBDbnV5NS9PRnRDbEExZDVFbmRZTXZvZDhtcUYzaXgrczVMNFA0WHJR?=
 =?utf-8?B?ZnpsRWg1YmdFRXJ4TE1LRnJPb0pVZTRLa3lMUkltSFllRXp4ZUZ1SklkZ21L?=
 =?utf-8?B?M1JvNXVwaXpEb3JIblJvSFhYRnZ4SGorcGI1WE0vTXhsbmsycEEzNllwQmhJ?=
 =?utf-8?B?OFEyblliNWZlaWVCZjBZUEwyS3pPQVFVTkF3d01mVDFBRWpUNzJyWlB6MFNM?=
 =?utf-8?B?Q05ROVNkREdsVklQbkZqM0orWEFQc3prOC9UaFd6Yk5meHNOcFlGbHd1Ryts?=
 =?utf-8?B?Ui9ERGtkZHRwKzZTRlNaK0VZQzA5TzY5MTcvV3ZacGtxOTd2NzZjdXJGelFH?=
 =?utf-8?B?Mkhka0tVRUIwQzZEaXEwMFR0TCtQNXZ3cU55L0FsMWZCL09ibnFkdWlrRG4z?=
 =?utf-8?B?Y3lGZXkzaFhGNmp0d3FjM0doMDFzYU51QUxXeTBudEgzZ3NudEl5Tlk5NkQw?=
 =?utf-8?B?eXE5Yldld0M3eU1QNGVlS2RzNFBaZDN6aG1UYWl2VDhETkt4enhZM0RDLzdU?=
 =?utf-8?B?QU1JSXpLS0ZiOEJQcStaRDRwUmNOSzZ5VEVEZnkzbHdYTmhyNGlqNE9uMmdl?=
 =?utf-8?B?Ym83VXZmMk1sWmdQZDN4amZPRnpNVjkyRnpXQ0tXU3BzUGFtRTdoYUdBS0tx?=
 =?utf-8?B?NWJucXhEYUxMNVF3SlhVWlByS2IzTTVMdDNNR2J2OXNSNTk0WjJrRWRMczMw?=
 =?utf-8?B?ekN0bkpCeDd3bFFMN2Z1OGtFVkJhM3N6eEVxYnJsMnVhTWVCZktHOXQ3ZXlQ?=
 =?utf-8?B?ZXRHeFd3Ry81ckFtcnprZjNxRU1ZSTB6ZXdod2xFVVpFcERPWXJQYU5IRk1y?=
 =?utf-8?B?NVZhUzBzQ0MyY1RudjBWaXlEd2ZtSTY5OWVOeFVrZUFWbEFEZlVHTEkyL25k?=
 =?utf-8?B?aEpzbWJxQ3YwR1EzWFdkSWpaUEJVOFIySEk2cThaZ3Z2RmtsRUdaRU1jeE1J?=
 =?utf-8?B?Q3Y1MnJaWWhUaDZXajRwODVNaHlUVXhsR1dHL2xzdTdROUVNVWZudzJVWGhr?=
 =?utf-8?B?STlXOXdpOG9YbUxvZ1RnSUo5UVZpeDlDUmdxN2lsRkJmemYyTjFDaW5JM1JN?=
 =?utf-8?B?M29zeUwrVnNvVm8yNXJ0em1jbkxOMW9iQ2gxU0FKalRnWTlHRFdjWExRbDF4?=
 =?utf-8?B?cUE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: df43824c-838e-4a5a-1458-08db92909eed
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3625.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Aug 2023 13:10:04.4522
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: fgf19QnWjSkDYNeWhCtCkHFe91D4Stkin04sSiXjrteyz5CDERcpy4T/WLp4I96QQnRExUWKVJ5CsDzxtT5KkeRzTVT+8GcR1h91ySv4rfg=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR11MB6063
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.2 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_BLOCKED,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,
        T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED 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: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

From: Kees Cook <keescook@chromium.org>
Date: Fri, 28 Jul 2023 15:43:26 -0700

> On Fri, Jul 28, 2023 at 05:52:05PM +0200, Alexander Lobakin wrote:
>> The two most problematic virtchnl structures are virtchnl_rss_key and
>> virtchnl_rss_lut. Their "flex" arrays have the type of u8, thus, when
>> allocating / checking, the actual size is calculated as `sizeof +
>> nents - 1 byte`. But their sizeof() is not 1 byte larger than the size
>> of such structure with proper flex array, it's two bytes larger due to
>> the padding. That said, their size is always 1 byte larger unless
>> there are no tail elements -- then it's +2 bytes.
>> Add virtchnl_struct_size() macro which will handle this case (and later
>> other cases as well). Make its calling conv the same as we call
>> struct_size() to allow it to be drop-in, even though it's unlikely to
>> become possible to switch to generic API. The macro will calculate a
>> proper size of a structure with a flex array at the end, so that it
>> becomes transparent for the compilers, but add the difference from the
>> old values, so that the real size of sorta-ABI-messages doesn't change.
>> Use it on the allocation side in IAVF and the receiving side (defined
>> as static inline in virtchnl.h) for the mentioned two structures.
> 
> This all looks workable, but it's a unique solution in the kernel. That
> is fine, of course, but would it be easier to maintain/test if it went
> with the union style solutions?
> 
> struct foo {
> 	...
> 	union {
> 		type legacy_padding;
> 		DECLARE_FLEX_ARRAY(type, member);
> 	};
> };
> 
> Then the size doesn't change and "member" can still be used. (i.e. no
> collateral changes needed.)

This wouldn't work unfortunately. I mean, you'd still need weird
calculations.
Speaking of e.g. virtchnl_rss_lut, it's always
`struct_size(nents + 1) - 1`, you can't just use the pattern above and
then struct_size(). Not only legacy padding is needed, but also
calculation adjustments.
Or did you mean define the structures as above and leave the
calculations as they are? It makes me feel we can miss something that
way. With the series, all the broken structures are processed in one
place and I thought _this_ would be actually easier to maintain...

> 
> -Kees
> 

Thanks,
Olek