Date:   Tue, 7 Dec 2021 21:25:32 -0800
From:   Yury Norov <yury.norov@gmail.com>
To:     Kees Cook <keescook@chromium.org>
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: <20211208052532.GA11482@lapt>
References: <20211203100846.3977195-1-keescook@chromium.org>
 <YaoN6wnNezMvyyd5@smile.fi.intel.com>
 <20211203182638.GA450223@lapt>
 <202112031450.EFE7B7B4A@keescook>
 <20211207233930.GA3955@lapt>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211207233930.GA3955@lapt>
Precedence: bulk

On Tue, Dec 07, 2021 at 03:39:33PM -0800, Yury Norov wrote:
> On Fri, Dec 03, 2021 at 03:01:30PM -0800, Kees Cook wrote:
> > 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.
> 
> Bitmap functions work identically for all sizes from 0 to INT_MAX - 1. 
> Users don't 'choose between functions based on the size of their target'.
> 
> Can you explain more what you mean?
> 
> > This also doesn't work well
> > for the main problem which is the for_each_* usage.
> 
> for_each_*_bit() requires a pointer to an array of unsigned longs. If
> it's provided with something else, this is an error on a caller side.
> 
> > 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.
> 
> If you want to allow passing types other than unsigned long *, you need
> to be consistent and propagate this change to other bitmap functions.
> This is much more work than just fixing at most 48 wrong callers.
> (48 because I inspected some callers manually, and they are fine.)
> 
> > 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.
> 
> As you said, -Warray-bounds _correctly_ gets upset about the dangerous
> typecasting. What suggested here is an attempt to shut down the
> compiler warning with the cost of complication of the code and
> possible maintenance issues. The correct example of handling tiny
> bitmaps can be found for example in drivers/mtd/nand/raw/ams-delta.c:
> 
>         static void gpio_nand_io_write(struct gpio_nand *priv, u8 byte)
>         {
>                 struct gpio_descs *data_gpiods = priv->data_gpiods;
>                 DECLARE_BITMAP(values, BITS_PER_TYPE(byte)) = { byte, };
> 
>                 ...
>         }

Or use memweight(), if it's more appropriate.
 
> > 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. :)
> 
> I'm all for enabling -Warray-bounds, but the warnings that it spots
> only convinced me that bitmap API is used wrongly, and it should be
> fixed. Can you please share the list of bitmap-related issues found
> with -Warray-bounds that concerned you? I'll take a look and try to
> make a patch that fixes it.
> 
> Thanks,
> Yury