Date:   Fri, 03 Dec 2021 08:37:59 -0800
From:   Kees Cook <keescook@chromium.org>
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>
CC:     Yury Norov <yury.norov@gmail.com>,
        Rasmus Villemoes <linux@rasmusvillemoes.dk>,
        Wolfram Sang <wsa+renesas@sang-engineering.com>,
        linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org
Subject: =?US-ASCII?Q?Re=3A_=5BPATCH=5D_find=3A_Do_not_read_beyon?= =?US-ASCII?Q?d_variable_boundaries_on_small_sizes?=
User-Agent: K-9 Mail for Android
In-Reply-To: <YaoN6wnNezMvyyd5@smile.fi.intel.com>
References: <20211203100846.3977195-1-keescook@chromium.org> <YaoN6wnNezMvyyd5@smile.fi.intel.com>
Message-ID: <E1A4953A-8801-48FA-A744-63DA548C5924@chromium.org>
MIME-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk


On December 3, 2021 4:30:35 AM PST, Andy Shevchenko <andriy=2Eshevchenko@l=
inux=2Eintel=2Ecom> wrote:
>On Fri, Dec 03, 2021 at 02:08:46AM -0800, Kees Cook wrote:
>> It's common practice to cast small variable arguments to the find_*_bit=
()
>
>It's a bad practice and should be fixed accordingly, no?

There's an argument to be made that the first arg should be void * but tha=
t's a pretty invasive change at this point (and orthogonal to this fix)=2E

I'd be happy to send a treewide change for that too, if folks wanted?

>
>> helpers to unsigned long and then use a size argument smaller than
>> sizeof(unsigned long):
>>=20
>> 	unsigned int bits;
>> 	=2E=2E=2E
>> 	out =3D find_first_bit((unsigned long *)&bits, 32);
>>=20
>> This leads to the find helper dereferencing a full unsigned long,
>> regardless of the size of the actual variable=2E The unwanted bits
>> get masked away, but strictly speaking, a read beyond the end of
>> the target variable happens=2E Builds under -Warray-bounds complain
>> about this situation, for example:
>>=20
>> In file included from =2E/include/linux/bitmap=2Eh:9,
>>                  from drivers/iommu/intel/iommu=2Ec:17:
>> drivers/iommu/intel/iommu=2Ec: In function 'domain_context_mapping_one'=
:
>> =2E/include/linux/find=2Eh:119:37: error: array subscript 'long unsigne=
d int[0]' is partly outside array bounds of 'int[1]' [-Werror=3Darray-bound=
s]
>>   119 |                 unsigned long val =3D *addr & GENMASK(size - 1,=
 0);
>>       |                                     ^~~~~
>> drivers/iommu/intel/iommu=2Ec:2115:18: note: while referencing 'max_pde=
'
>>  2115 |         int pds, max_pde;
>>       |                  ^~~~~~~
>>=20
>> Instead, just carefully read the correct variable size, all of which
>> happens at compile time since small_const_nbits(size) has already
>> determined that arguments are constant expressions=2E
>
>What is the performance impact?

There should be none=2E It's entirely using constant expressions, so all o=
f it gets reduce at compile time to a single path without conditionals=2E T=
he spot checks I did on the machine code showed no differences either (sinc=
e I think optimization was doing the masking vis smaller width dereference)=
=2E


>

--=20
Kees Cook