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: <a61b7923-fc3f-c96d-ea84-0ae4fa176eac@intel.com>
Date:   Tue, 29 Aug 2023 12:25:04 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.14.0
Subject: Re: [PATCH] coccinelle: semantic patch to check for potential
 struct_size calls
To:     Kees Cook <keescook@chromium.org>
CC:     Julia Lawall <Julia.Lawall@lip6.fr>,
        "Gustavo A . R . Silva" <gustavoars@kernel.org>,
        <cocci@systeme.lip6.fr>, <linux-kernel@vger.kernel.org>,
        <linux-hardening@vger.kernel.org>
References: <20230227202428.3657443-1-jacob.e.keller@intel.com>
 <202308261813.A26FA1A@keescook>
Content-Language: en-US
From:   Jacob Keller <jacob.e.keller@intel.com>
In-Reply-To: <202308261813.A26FA1A@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?VG1MU0ViQ2w2cjV1WjlEeFN1RVQyQXIzNlcrcTVQWnZDWWdOOWdrbFF2OWpC?=
 =?utf-8?B?Z0FuZHVBOUxJQVNoa0I2VlRiN1dsRi9YMmlpS3dVNkxkRXcvdjAxWmN5T1FZ?=
 =?utf-8?B?OEEvVkxIREtsN2hNYjhWbTJKVURvcU9UTmR1eFg5cmhmZVRSZzl0QTFDK0VT?=
 =?utf-8?B?RVZEb3RkQjNCdTdpaWlSMStpRDRKZzdsOHRFcGpiMFBOYjJYZDRKWHpDV0w3?=
 =?utf-8?B?RklzdG9paFpoZEZFeEN2eEluOGNyM3R5dVYyTytkYmNFVVhSbXdCbmZQZmdp?=
 =?utf-8?B?OXBhT0EzcTI4RGVqWFJEWUJvRy9OcStxaXM3aXF1aTZ0WTBha2p6V2VLaFVK?=
 =?utf-8?B?UW9ad25qb1Z5Y3dHdFBZVi9SNFVvOUM5WWk4YkdtSkRiK0hzaUJjMmhweGhV?=
 =?utf-8?B?Y1A0ZG8rM2N3dmdiVnRpYUI2M1BrVkVQWVpzSlNIMHR4RDgrd3Jld2tmbHN4?=
 =?utf-8?B?RG1FeXgxcnBDamRSUVBEbnRXVGpPWXRHK1k1RWZXUW1GTGJhWFJhRDlnQlZv?=
 =?utf-8?B?K3UrdDdZcjhEN1J2TjNRZjdxd2RqV0dxdnRqNTNSR0ZkV1FFUnFMVTVLYStO?=
 =?utf-8?B?QXNOV2UzSmtFdjd2VS9Pby9zdU1rN3V1aHNBWlNJcGdTMkZPZjFoV3M5aStu?=
 =?utf-8?B?RU83MHdtUzZkVDU0YlJtRlROTEgwRzhxeUlKeHovVUxoS0xwanMrMWlER0xY?=
 =?utf-8?B?VjEvYllSTE13Rkk3V3VZZ2dJWnlObFBiTldKT2VXVTBHU2VCZDIyZlV1QVgx?=
 =?utf-8?B?RklqS20yU3V2WnE5VjVhR0NDZlBiWmhFMGFISFl5SFV4QXZNNHJVZEpaenhN?=
 =?utf-8?B?Vm9CRkc5UXhiNFNZQUZTZ3lTenZQMUtXYjl1VzFwZjh6ZHB6V0prQkJPcElU?=
 =?utf-8?B?NFR3T2tZOEZ1YUVOU2tQajF2ekV0OW9Vdlh3Rjg4MGFaRHo3N3BsUmJtdHpw?=
 =?utf-8?B?TWZzRUhNcjQrSGxROWtQKzBTRGpSU1pIL2R3TkcxWm5VOVhPZVFnK0NDQTFI?=
 =?utf-8?B?aWVHVGU2VzdteXdsVzRyOWtydFFyc2dXU1NZTURabDJ5c0JTZG9FNTZkNlpW?=
 =?utf-8?B?ODRIWUNKdzNidGRhSGd4OUJEcDBLbzJ3UUtSY280dVBERVRrMkZ0WHNmTDBL?=
 =?utf-8?B?S1ZRZ0hPd05Ua3U5Tk5uNnBLWGlsV0RHeGFkU2xVbnc0ajM1WCs0L1lIcjVh?=
 =?utf-8?B?S29ScHY5YjZ6QndESzRxcmhheDM2R1JoN0NXK0ErWXBVdUxnUUhMR2JUL3U3?=
 =?utf-8?B?V1RpVmswUXhTNjl3b1JBWGZ5bytXWUQ4TmtmTWR5cjJWRUR5QU1JOHk4QjlY?=
 =?utf-8?B?SUlDUmd5cVZ6T1l0b0dYbHRraEVOZko0Nm41aVlLQkRxNStaTTQ2UHFrNXNq?=
 =?utf-8?B?Zyt1N3lRbU0xdEc4Y0pNRFA4aFNkY0xEMW0vMnRvRlRKSjBPY3Y4ZVZhTjVJ?=
 =?utf-8?B?UXptQjhEOU5nOHM4VU5oUWwxSHkvTjh5Z2dVbTBjbE9EajJjVXpBOStUeUsx?=
 =?utf-8?B?V0pVTUtkOG5uNWxwV2ZSa1kzUGtBMlR5eG15ZFZBdlRhYTl5eU5FOUtvNjFV?=
 =?utf-8?B?eUlieVVEeTY3WWN3UjFrU1k3REh2cDg4clBGNEJuUVNBM3dqNG5HNlI5bFR0?=
 =?utf-8?B?dG1xanhjeWNYRmNCalZGSDg3dndnY0dobjA5Y25nTG4yOHpjVzBFQ3F3Q2JX?=
 =?utf-8?B?a2g2K1prZ1BkSWpUbW9rdDNsQjhVWE02SVMvWDdjWlN1bnFORWVXQkJRV1VG?=
 =?utf-8?B?S29BR1pxVi81VElMUXZNaCtNU2JYdndBMm13cmlDZi8zTSsxVnZkWS9pQWdW?=
 =?utf-8?B?V3I5VmtqZjVJTW9iMExieExxOHEzblB2Y3V0ZzY3a1RPUjdoOHUrVnNuOTcx?=
 =?utf-8?B?Y0tndzNvbndaSDU1WFZ0S1RCaFBXbXJvV2kvUkxacWE2dkhSczBCcmZNdG1G?=
 =?utf-8?B?UUE1TTFwbWhFRFNVYzhyd0MvUktoc1hqbXRDK3BwV1RSd1B5dCtILytTQnI3?=
 =?utf-8?B?ekdQNHlKUlErNXVGNk9OVGRTT0FsVXVtaDFIVE1pWlNpaXozd1QyR2pxQXM2?=
 =?utf-8?B?MXhGcWNVSTVLU3lPUjhVZHdqRGt5a29FdUhsQ0UvS3hKWkhVZXBVdU5qMHA1?=
 =?utf-8?B?RzloWFJGbmoyOXJiOTlzNHVmNFFpcXl4K3B4R2ZMR0U3NHZBWnRiTktaZUcv?=
 =?utf-8?B?ZHc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 48da7248-8697-406e-9b1d-08dba8c5a684
X-MS-Exchange-CrossTenant-AuthSource: CO1PR11MB5089.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Aug 2023 19:25:06.0687
 (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: N+7PVpA/H8ZTESLFm/PsatEXgHtoq4i7eVEvA4V4ocTM4UzuhFhDDbFChucy9oBsSTU8yyJG9mdqo3kuoyNvcSwf1P1GgyDqteaa/cJimMM=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR11MB5491
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-3.3 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_H3,RCVD_IN_MSPIKE_WL,
        SPF_HELO_NONE,SPF_NONE 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


On 8/26/2023 6:19 PM, Kees Cook wrote:
> Hi!
> 
> I'm sorry I lost this email! I just found it while trying to clean up
> my inbox.
> 
> On Mon, Feb 27, 2023 at 12:24:28PM -0800, Jacob Keller wrote:
>> include/linux/overflow.h includes helper macros intended for calculating
>> sizes of allocations. These macros prevent accidental overflow by
>> saturating at SIZE_MAX.
>>
>> In general when calculating such sizes use of the macros is preferred. Add
>> a semantic patch which can detect code patterns which can be replaced by
>> struct_size.
>>
>> Note that I set the confidence to medium because this patch doesn't make an
>> attempt to ensure that the relevant array is actually a flexible array. The
>> struct_size macro does specifically require a flexible array. In many cases
>> the detected code could be refactored to a flexible array, but this is not
>> always possible (such as if there are multiple over-allocations).
>>
>> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
>> Cc: Julia Lawall <Julia.Lawall@lip6.fr>
>> Cc: Kees Cook <keescook@chromium.org>
>> Cc: Gustavo A. R. Silva <gustavoars@kernel.org>
>> Cc: cocci@systeme.lip6.fr
>> Cc: linux-kernel@vger.kernel.org
>>
>>  scripts/coccinelle/misc/struct_size.cocci | 74 +++++++++++++++++++++++
>>  1 file changed, 74 insertions(+)
>>  create mode 100644 scripts/coccinelle/misc/struct_size.cocci
> 
> Yes! I'd really like to get something like this into the Coccinelle
> scripts.
> 
>> diff --git a/scripts/coccinelle/misc/struct_size.cocci b/scripts/coccinelle/misc/struct_size.cocci
>> new file mode 100644
>> index 000000000000..4ede9586e3c6
>> --- /dev/null
>> +++ b/scripts/coccinelle/misc/struct_size.cocci
>> @@ -0,0 +1,74 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +///
>> +/// Check for code that could use struct_size().
>> +///
>> +// Confidence: Medium
>> +// Author: Jacob Keller <jacob.e.keller@intel.com>
>> +// Copyright: (C) 2023 Intel Corporation
>> +// Options: --no-includes --include-headers
>> +
>> +virtual patch
>> +virtual context
>> +virtual org
>> +virtual report
>> +
>> +// the overflow Kunit tests have some code which intentionally does not use
>> +// the macros, so we want to ignore this code when reporting potential
>> +// issues.
>> +@overflow_tests@
>> +identifier f = overflow_size_helpers_test;
>> +@@
>> +
>> +f
>> +
>> +//----------------------------------------------------------
>> +//  For context mode
>> +//----------------------------------------------------------
>> +
>> +@depends on !overflow_tests && context@
>> +expression E1, E2;
>> +identifier m;
>> +@@
>> +(
>> +* (sizeof(*E1) + (E2 * sizeof(*E1->m)))
>> +)
>> +
>> +//----------------------------------------------------------
>> +//  For patch mode
>> +//----------------------------------------------------------
>> +
>> +@depends on !overflow_tests && patch@
>> +expression E1, E2;
>> +identifier m;
>> +@@
>> +(
>> +- (sizeof(*E1) + (E2 * sizeof(*E1->m)))
>> ++ struct_size(E1, m, E2)
>> +)
> 
> Two notes:
> 
> This can lead to false positives (like for struct mux_chip) which
> doesn't use a flexible array member, which means struct_size() will
> actually fail to build (it requires the 2nd arg to be an array).
> 

I actually sent a fix for mux chip to refactor it to struct_size too :)

https://lore.kernel.org/all/20230223014221.1710307-1-jacob.e.keller@intel.com/

> This can miss cases that have more than a single struct depth (which is
> uncommon but happens). I don't know how to match only "substruct.member"
> from "ptr->substruct.member". (I know how to match the whole thing[1],
> though.)
> 
Yea I couldn't figure out how to get it to handle both cases here but I
actually prefer reporting cases like mux_chip, since they can usually be
refactored to use struct size properly.

> That isn't reason not to take this patch, though. It's a good start!
> 

Right. Both cases like this are why I set the confidence to only medium,
and mentioned it in the commit :D


> Thanks for writing this up!
> 
> -Kees
> 

Thanks for reviewing. I also sent some struct_size cleanups that look to
have stalled and could use some review or a re-send if necessary at this
point.

I think the full list can be found with this lore.kernel.org search:

https://lore.kernel.org/all/?q=f%3Ajacob.e.keller+AND+%28+s%3Astruct_size+OR+s%3A%22flexible+array%22+%29