Messages in this thread | | | Date | Tue, 7 Dec 2021 21:25:32 -0800 | From | Yury Norov <> | Subject | Re: [PATCH] find: Do not read beyond variable boundaries on small sizes |
| |
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
| |