Date:   Fri, 3 Dec 2021 15:01:30 -0800
From:   Kees Cook <keescook@chromium.org>
To:     Yury Norov <yury.norov@gmail.com>
Cc:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Rasmus Villemoes <linux@rasmusvillemoes.dk>,
        Wolfram Sang <wsa+renesas@sang-engineering.com>,
        linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org,
        Steven Rostedt <rostedt@goodmis.org>
Subject: Re: [PATCH] find: Do not read beyond variable boundaries on small
 sizes
Message-ID: <202112031450.EFE7B7B4A@keescook>
References: <20211203100846.3977195-1-keescook@chromium.org>
 <YaoN6wnNezMvyyd5@smile.fi.intel.com>
 <20211203182638.GA450223@lapt>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211203182638.GA450223@lapt>
Precedence: bulk

On Fri, Dec 03, 2021 at 10:26:38AM -0800, Yury Norov wrote:
> On Fri, Dec 03, 2021 at 02:30:35PM +0200, Andy Shevchenko 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()
> 
> Not that common - I found 19 examples of this cast, and most of them
> are in drivers.

I find 51 (most are in the for_each_* wrappers):

$ RE=$(echo '\b('$(echo $(grep -E '^(unsigned long find|#define for_each)_' include/linux/find.h | cut -d'(' -f1 | awk '{print $NF}') | tr ' ' '|')')\(.*\(unsigned long \*\)')
$ git grep -E "$RE" | wc -l
51

> > > This leads to the find helper dereferencing a full unsigned long,
> > > regardless of the size of the actual variable. The unwanted bits
> > > get masked away, but strictly speaking, a read beyond the end of
> > > the target variable happens. Builds under -Warray-bounds complain
> > > about this situation, for example:
> > > 
> > > In file included from ./include/linux/bitmap.h:9,
> > >                  from drivers/iommu/intel/iommu.c:17:
> > > drivers/iommu/intel/iommu.c: In function 'domain_context_mapping_one':
> > > ./include/linux/find.h:119:37: error: array subscript 'long unsigned int[0]' is partly outside array bounds of 'int[1]' [-Werror=array-bounds]
> > >   119 |                 unsigned long val = *addr & GENMASK(size - 1, 0);
> > >       |                                     ^~~~~
> > > drivers/iommu/intel/iommu.c:2115:18: note: while referencing 'max_pde'
> > >  2115 |         int pds, max_pde;
> > >       |                  ^~~~~~~
> 
> The driver should be fixed. I would suggest using one of ffs/fls/ffz from
> include/asm/bitops.h

I don't think it's a good API design to make developers choose between
functions based on the size of their target. This also doesn't work well
for the main problem which is the for_each_* usage.

The existing API is totally fine: it already diverts the constant
expression small sizes to ffs/etc, and this change is only to that
part. It's just changing the C description of how to get at the desired
bits, so that -Warray-bounds doesn't (correctly) get upset about the
wider-than-underlying-type OOB read.

This is one of the last issues with -Warray-bounds, which has proven to
be an effective compiler flag for finding real bugs. Since this patch
doesn't change performance, doesn't change the resulting executable
instructions, doesn't require any caller changes, and helps gain global
-Warray-bounds coverage, I'm hoping to convince you of its value. :)

-Kees

-- 
Kees Cook