| Message ID | YsbzgQQ4bg6v+iTS@kili (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio/mlx5: clean up overflow check | expand |
Hi Dan,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on awilliam-vfio/next]
[also build test WARNING on rdma/for-next linus/master v5.19-rc5 next-20220707]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Dan-Carpenter/vfio-mlx5-clean-up-overflow-check/20220707-225657
base: https://github.com/awilliam/linux-vfio.git next
config: i386-allyesconfig (https://download.01.org/0day-ci/archive/20220708/202207080331.FTVSHxW8-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/44607f8f3817e1af6622db7d70ad5bc457b8f203
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Dan-Carpenter/vfio-mlx5-clean-up-overflow-check/20220707-225657
git checkout 44607f8f3817e1af6622db7d70ad5bc457b8f203
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/vfio/pci/mlx5/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from include/linux/device.h:29,
from drivers/vfio/pci/mlx5/main.c:6:
drivers/vfio/pci/mlx5/main.c: In function 'mlx5vf_resume_write':
>> include/linux/overflow.h:67:22: warning: comparison of distinct pointer types lacks a cast
67 | (void) (&__a == &__b); \
| ^~
drivers/vfio/pci/mlx5/main.c:282:13: note: in expansion of macro 'check_add_overflow'
282 | check_add_overflow(len, (unsigned long)*pos, &requested_length))
| ^~~~~~~~~~~~~~~~~~
include/linux/overflow.h:68:22: warning: comparison of distinct pointer types lacks a cast
68 | (void) (&__a == __d); \
| ^~
drivers/vfio/pci/mlx5/main.c:282:13: note: in expansion of macro 'check_add_overflow'
282 | check_add_overflow(len, (unsigned long)*pos, &requested_length))
| ^~~~~~~~~~~~~~~~~~
vim +67 include/linux/overflow.h
9b80e4c4ddaca35 Kees Cook 2020-08-12 54
f0907827a8a9152 Rasmus Villemoes 2018-05-08 55 /*
f0907827a8a9152 Rasmus Villemoes 2018-05-08 56 * For simplicity and code hygiene, the fallback code below insists on
f0907827a8a9152 Rasmus Villemoes 2018-05-08 57 * a, b and *d having the same type (similar to the min() and max()
f0907827a8a9152 Rasmus Villemoes 2018-05-08 58 * macros), whereas gcc's type-generic overflow checkers accept
f0907827a8a9152 Rasmus Villemoes 2018-05-08 59 * different types. Hence we don't just make check_add_overflow an
f0907827a8a9152 Rasmus Villemoes 2018-05-08 60 * alias for __builtin_add_overflow, but add type checks similar to
f0907827a8a9152 Rasmus Villemoes 2018-05-08 61 * below.
f0907827a8a9152 Rasmus Villemoes 2018-05-08 62 */
9b80e4c4ddaca35 Kees Cook 2020-08-12 63 #define check_add_overflow(a, b, d) __must_check_overflow(({ \
f0907827a8a9152 Rasmus Villemoes 2018-05-08 64 typeof(a) __a = (a); \
f0907827a8a9152 Rasmus Villemoes 2018-05-08 65 typeof(b) __b = (b); \
f0907827a8a9152 Rasmus Villemoes 2018-05-08 66 typeof(d) __d = (d); \
f0907827a8a9152 Rasmus Villemoes 2018-05-08 @67 (void) (&__a == &__b); \
f0907827a8a9152 Rasmus Villemoes 2018-05-08 68 (void) (&__a == __d); \
f0907827a8a9152 Rasmus Villemoes 2018-05-08 69 __builtin_add_overflow(__a, __b, __d); \
9b80e4c4ddaca35 Kees Cook 2020-08-12 70 }))
f0907827a8a9152 Rasmus Villemoes 2018-05-08 71
Hi Dan,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on awilliam-vfio/next]
[also build test WARNING on rdma/for-next linus/master v5.19-rc6 next-20220711]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Dan-Carpenter/vfio-mlx5-clean-up-overflow-check/20220707-225657
base: https://github.com/awilliam/linux-vfio.git next
config: i386-allyesconfig (https://download.01.org/0day-ci/archive/20220712/202207121350.fs2JOFWt-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 6ce63e267aab79ca87bf63453d34dd3909ab978d)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/44607f8f3817e1af6622db7d70ad5bc457b8f203
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Dan-Carpenter/vfio-mlx5-clean-up-overflow-check/20220707-225657
git checkout 44607f8f3817e1af6622db7d70ad5bc457b8f203
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/hid/ drivers/md/ drivers/vfio/pci/mlx5/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/vfio/pci/mlx5/main.c:282:6: warning: comparison of distinct pointer types ('typeof (len) *' (aka 'unsigned int *') and 'typeof ((unsigned long)*pos) *' (aka 'unsigned long *')) [-Wcompare-distinct-pointer-types]
check_add_overflow(len, (unsigned long)*pos, &requested_length))
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/overflow.h:67:15: note: expanded from macro 'check_add_overflow'
(void) (&__a == &__b); \
~~~~ ^ ~~~~
>> drivers/vfio/pci/mlx5/main.c:282:6: warning: comparison of distinct pointer types ('typeof (len) *' (aka 'unsigned int *') and 'typeof (&requested_length)' (aka 'unsigned long *')) [-Wcompare-distinct-pointer-types]
check_add_overflow(len, (unsigned long)*pos, &requested_length))
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/overflow.h:68:15: note: expanded from macro 'check_add_overflow'
(void) (&__a == __d); \
~~~~ ^ ~~~
2 warnings generated.
vim +282 drivers/vfio/pci/mlx5/main.c
269
270 static ssize_t mlx5vf_resume_write(struct file *filp, const char __user *buf,
271 size_t len, loff_t *pos)
272 {
273 struct mlx5_vf_migration_file *migf = filp->private_data;
274 unsigned long requested_length;
275 ssize_t done = 0;
276
277 if (pos)
278 return -ESPIPE;
279 pos = &filp->f_pos;
280
281 if (*pos < 0 || *pos > ULONG_MAX ||
> 282 check_add_overflow(len, (unsigned long)*pos, &requested_length))
283 return -EINVAL;
284
285 if (requested_length > MAX_MIGRATION_SIZE)
286 return -ENOMEM;
287
288 mutex_lock(&migf->lock);
289 if (migf->disabled) {
290 done = -ENODEV;
291 goto out_unlock;
292 }
293
294 if (migf->allocated_length < requested_length) {
295 done = mlx5vf_add_migration_pages(
296 migf,
297 DIV_ROUND_UP(requested_length - migf->allocated_length,
298 PAGE_SIZE));
299 if (done)
300 goto out_unlock;
301 }
302
303 while (len) {
304 size_t page_offset;
305 struct page *page;
306 size_t page_len;
307 u8 *to_buff;
308 int ret;
309
310 page_offset = (*pos) % PAGE_SIZE;
311 page = mlx5vf_get_migration_page(migf, *pos - page_offset);
312 if (!page) {
313 if (done == 0)
314 done = -EINVAL;
315 goto out_unlock;
316 }
317
318 page_len = min_t(size_t, len, PAGE_SIZE - page_offset);
319 to_buff = kmap_local_page(page);
320 ret = copy_from_user(to_buff + page_offset, buf, page_len);
321 kunmap_local(to_buff);
322 if (ret) {
323 done = -EFAULT;
324 goto out_unlock;
325 }
326 *pos += page_len;
327 len -= page_len;
328 done += page_len;
329 buf += page_len;
330 migf->total_length += page_len;
331 }
332 out_unlock:
333 mutex_unlock(&migf->lock);
334 return done;
335 }
336
Sorry for these! I need to resend, of course. I'm sligtly delayed because it was a three day weekend. I will try do that tomorrow. regards, dan carpenter On Fri, Jul 08, 2022 at 03:37:32AM +0800, kernel test robot wrote: > Hi Dan, > > Thank you for the patch! Perhaps something to improve: > > [auto build test WARNING on awilliam-vfio/next] > [also build test WARNING on rdma/for-next linus/master v5.19-rc5 next-20220707] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch#_base_tree_information] > > url: https://github.com/intel-lab-lkp/linux/commits/Dan-Carpenter/vfio-mlx5-clean-up-overflow-check/20220707-225657 > base: https://github.com/awilliam/linux-vfio.git next > config: i386-allyesconfig (https://download.01.org/0day-ci/archive/20220708/202207080331.FTVSHxW8-lkp@intel.com/config) > compiler: gcc-11 (Debian 11.3.0-3) 11.3.0 > reproduce (this is a W=1 build): > # https://github.com/intel-lab-lkp/linux/commit/44607f8f3817e1af6622db7d70ad5bc457b8f203 > git remote add linux-review https://github.com/intel-lab-lkp/linux > git fetch --no-tags linux-review Dan-Carpenter/vfio-mlx5-clean-up-overflow-check/20220707-225657 > git checkout 44607f8f3817e1af6622db7d70ad5bc457b8f203 > # save the config file > mkdir build_dir && cp config build_dir/.config > make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/vfio/pci/mlx5/ > > If you fix the issue, kindly add following tag where applicable > Reported-by: kernel test robot <lkp@intel.com> > > All warnings (new ones prefixed by >>): > > In file included from include/linux/device.h:29, > from drivers/vfio/pci/mlx5/main.c:6: > drivers/vfio/pci/mlx5/main.c: In function 'mlx5vf_resume_write': > >> include/linux/overflow.h:67:22: warning: comparison of distinct pointer types lacks a cast > 67 | (void) (&__a == &__b); \ > | ^~ > drivers/vfio/pci/mlx5/main.c:282:13: note: in expansion of macro 'check_add_overflow' > 282 | check_add_overflow(len, (unsigned long)*pos, &requested_length)) > | ^~~~~~~~~~~~~~~~~~ > include/linux/overflow.h:68:22: warning: comparison of distinct pointer types lacks a cast > 68 | (void) (&__a == __d); \ > | ^~ > drivers/vfio/pci/mlx5/main.c:282:13: note: in expansion of macro 'check_add_overflow' > 282 | check_add_overflow(len, (unsigned long)*pos, &requested_length)) > | ^~~~~~~~~~~~~~~~~~ > > > vim +67 include/linux/overflow.h > > 9b80e4c4ddaca35 Kees Cook 2020-08-12 54 > f0907827a8a9152 Rasmus Villemoes 2018-05-08 55 /* > f0907827a8a9152 Rasmus Villemoes 2018-05-08 56 * For simplicity and code hygiene, the fallback code below insists on > f0907827a8a9152 Rasmus Villemoes 2018-05-08 57 * a, b and *d having the same type (similar to the min() and max() > f0907827a8a9152 Rasmus Villemoes 2018-05-08 58 * macros), whereas gcc's type-generic overflow checkers accept > f0907827a8a9152 Rasmus Villemoes 2018-05-08 59 * different types. Hence we don't just make check_add_overflow an > f0907827a8a9152 Rasmus Villemoes 2018-05-08 60 * alias for __builtin_add_overflow, but add type checks similar to > f0907827a8a9152 Rasmus Villemoes 2018-05-08 61 * below. > f0907827a8a9152 Rasmus Villemoes 2018-05-08 62 */ > 9b80e4c4ddaca35 Kees Cook 2020-08-12 63 #define check_add_overflow(a, b, d) __must_check_overflow(({ \ > f0907827a8a9152 Rasmus Villemoes 2018-05-08 64 typeof(a) __a = (a); \ > f0907827a8a9152 Rasmus Villemoes 2018-05-08 65 typeof(b) __b = (b); \ > f0907827a8a9152 Rasmus Villemoes 2018-05-08 66 typeof(d) __d = (d); \ > f0907827a8a9152 Rasmus Villemoes 2018-05-08 @67 (void) (&__a == &__b); \ > f0907827a8a9152 Rasmus Villemoes 2018-05-08 68 (void) (&__a == __d); \ > f0907827a8a9152 Rasmus Villemoes 2018-05-08 69 __builtin_add_overflow(__a, __b, __d); \ > 9b80e4c4ddaca35 Kees Cook 2020-08-12 70 })) > f0907827a8a9152 Rasmus Villemoes 2018-05-08 71 > > -- > 0-DAY CI Kernel Test Service > https://01.org/lkp
diff --git a/drivers/vfio/pci/mlx5/main.c b/drivers/vfio/pci/mlx5/main.c index a9b63d15c5d3..c65dca59caec 100644 --- a/drivers/vfio/pci/mlx5/main.c +++ b/drivers/vfio/pci/mlx5/main.c @@ -271,15 +271,15 @@ static ssize_t mlx5vf_resume_write(struct file *filp, const char __user *buf, size_t len, loff_t *pos) { struct mlx5_vf_migration_file *migf = filp->private_data; - loff_t requested_length; + unsigned long requested_length; ssize_t done = 0; if (pos) return -ESPIPE; pos = &filp->f_pos; - if (*pos < 0 || - check_add_overflow((loff_t)len, *pos, &requested_length)) + if (*pos < 0 || *pos > ULONG_MAX || + check_add_overflow(len, (unsigned long)*pos, &requested_length)) return -EINVAL; if (requested_length > MAX_MIGRATION_SIZE)
The casting on this overflow check is not done correctly, but fortunately checks in the callers should prevent this from affecting runtime. The "len" variable is unsigned long while "*pos" and "requested_length" are signed long long. Imagine "len" was ULONG_MAX and "*pos" was 2. Then "ULONG_MAX + 2 = 1" which is an integer overflow so it will be caught. However if we cast "len" to a long long then it becomes "-1 + 2 = 1" which is not an integer overflow and will not be caught. However "len" cannot actually be that high and the check for "*pos < 0" means that this cannot happen. Still it's worth cleaning up just as a hardenning measure and so that it's not copy and pasted to other places. Fixes: 6fadb021266d ("vfio/mlx5: Implement vfio_pci driver for mlx5 devices") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- drivers/vfio/pci/mlx5/main.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)