linux-kernel - Re: [PATCH v2 0/8] drivers/usb: refactor min/max with min_t/max

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ZzpKOuZeWidsyGis@surfacebook.localdomain>
Date: Sun, 17 Nov 2024 21:55:38 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Sabyrzhan Tasbolatov <snovitoll@...il.com>,
	David Laight <David.Laight@...lab.com>
Cc: gregkh@...uxfoundation.org, andreyknvl@...il.com, b-liu@...com,
	johan@...nel.org, oneukum@...e.com, stern@...land.harvard.edu,
	linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
	usb-storage@...ts.one-eyed-alien.net
Subject: Re: [PATCH v2 0/8] drivers/usb: refactor min/max with min_t/max_t

Tue, Nov 12, 2024 at 08:58:09PM +0500, Sabyrzhan Tasbolatov kirjoitti:
> This patch series improves type safety in the drivers/usb/*
> by using `min_t()` and `max_t()` instead of min(), max()
> with the casting inside. It should address potential type promotion issues.
> 
> Scanned the current drivers/usb code with `max\(.*\(` and `min\(.*\(`
> regexp queries to find casting inside of min() and max() which
> may lead to subtle bugs or even security vulnerabilities,
> especially if negative values are involved.
> 
> Let's refactor to min_t() and max_t() specifying the data type
> to ensure it's applicable for the both compareable arguments.

max_t() is okay to use, but min_t() is quite a beast. Please, reconsider the
entire series and tell how had this been tested? I can't imagine how many
subtle bugs it might have introduced.

min_t() explicitly casts to the given type and this is a huge problem for
the cases when one of the parameter is of signed type. Besisdes that it
diminishes the type checking done in the min().

As Linus told, the many cases when you want to have min_t() is actually 
clamp(). In some cases you wanted umin().

+Cc: David.


-- 
With Best Regards,
Andy Shevchenko