Re: [PATCH] coccinelle: semantic patch to check for potential struct_size calls

[Jacob Keller <jacob.e.keller@intel.com>]

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