Received-SPF: pass (google.com: domain of linux-kernel+bounces-60162-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Fri, 9 Feb 2024 15:55:33 -0800
From: Kees Cook <keescook@chromium.org>
To: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Cc: linux-kernel@vger.kernel.org, David.Laight@aculab.com,
	pmalani@chromium.org, andriy.shevchenko@linux.intel.com,
	torvalds@linux-foundation.org,
	Andrew Morton <akpm@linux-foundation.org>,
	Herve Codina <herve.codina@bootlin.com>,
	"Matthew Wilcox (Oracle)" <willy@infradead.org>
Subject: Re: [PATCH] minmax: Add notes to min_t and max_t
Message-ID: <202402091551.16A4A2A7F@keescook>
References: <20240209150657.1.I45addf7579e1233fa97c05ba72120cd1c57b4310@changeid>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240209150657.1.I45addf7579e1233fa97c05ba72120cd1c57b4310@changeid>

On Fri, Feb 09, 2024 at 03:07:02PM -0800, Abhishek Pandit-Subedi wrote:
> Both min_t and max_t are problematic as they can hide issues when
> comparing differently sized types (and especially differently signed
> types). Update the comments to nudge users to other options until
> there is a better fix for these macros.
> 
> Link: https://lore.kernel.org/all/01e3e09005e9434b8f558a893a47c053@AcuMS.aculab.com/
> Link: https://lore.kernel.org/all/CAHk-=whwEAc22wm8h9FESPB5X+P4bLDgv0erBQMa1buTNQW7tA@mail.gmail.com/
> 
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> ---
> Andy Shevchenko made me aware of this particular footgun in
> https://lore.kernel.org/linux-usb/ZcZ_he1jYx8w57mK@smile.fi.intel.com/.
> 
> While David + others work on the full fix, I'm hoping to apply a
> bandaid in the form of comments so the problem doesn't get worse by devs
> (**cough** me **cough**) inadvertently doing the wrong thing.

I think a better example for the docs would be something like u16
(rather than size_t) which shows very quickly the potential for
truncation. See, for example:

https://lore.kernel.org/all/20230811054528.never.165-kees@kernel.org/

> 
> 
>  include/linux/minmax.h | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/include/linux/minmax.h b/include/linux/minmax.h
> index 2ec559284a9f..96646f840a1f 100644
> --- a/include/linux/minmax.h
> +++ b/include/linux/minmax.h
> @@ -154,6 +154,18 @@
>  
>  /**
>   * min_t - return minimum of two values, using the specified type
> + *
> + * Note: Downcasting types in this macro can cause incorrect results. Prefer to
> + * use min() which does typechecking.
> + *
> + * Prefer to use clamp if you are trying to compare to size_t.
> + *
> + * Don't:
> + *   min_t(size_t, buf_size, sizeof(foobar))
> + *
> + * Do:
> + *  clamp(buf_size, 0, sizeof(foobar))
> + *
>   * @type: data type to use
>   * @x: first value
>   * @y: second value

Please keep the types immediately after the definition -- notes can go
after.

> @@ -162,6 +174,10 @@
>  
>  /**
>   * max_t - return maximum of two values, using the specified type
> + *
> + * Note: Downcasting types in this macro can cause incorrect results. Prefer to
> + * use max() which does typechecking.
> + *
>   * @type: data type to use
>   * @x: first value
>   * @y: second value

Same.

But yes, I welcome the added comments! :)

-- 
Kees Cook