Messages in this thread | | | Date | Wed, 8 Dec 2021 12:22:30 +0200 | From | Andy Shevchenko <> | Subject | Re: [PATCH] find: Do not read beyond variable boundaries on small sizes |
| |
On Tue, Dec 07, 2021 at 03:39:30PM -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?
I believe it was a reaction to your suggestion about ffs/ffz/etc.
Kees, if we _know_ that the size of the value in question will always fit 32/64-bit, then ffs/ffz/etc is okay to use. OTOH, if the type of that value is unsigned long [] and bitmap APIs() is used, then of course the consistent use of bitmap APIs is preferable.
I.o.w. uXX: ffX()/etc is fine. unsigned long: bitmap API.
I believe that's what Yury meant.
-- With Best Regards, Andy Shevchenko
| |