| Message ID | YFCZ1NNfMoixOjWP@mwanda (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | lib/test_hmm.c: fix harmless shift wrapping bug | expand |
On 3/16/21 4:43 AM, Dan Carpenter wrote: > The "cmd.npages" variable is a u64 that comes from the user. I noticed > during review that it could have a shift wrapping bug when it is used > in the integer overflow test on the next line. > > It turns out this is harmless. The users all do: > > unsigned long size = cmd->npages << PAGE_SHIFT; > > and after that "size" is used consistently and "cmd->npages" is never > used again. So even when there is an integer overflow, everything works > fine. > > Even though this is harmless, I believe syzbot will complain and fixing > it makes the code easier to read. > > Fixes: b2ef9f5a5cb3 ("mm/hmm/test: add selftest driver for HMM") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > lib/test_hmm.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/lib/test_hmm.c b/lib/test_hmm.c > index 80a78877bd93..541466034a6b 100644 > --- a/lib/test_hmm.c > +++ b/lib/test_hmm.c > @@ -930,6 +930,8 @@ static long dmirror_fops_unlocked_ioctl(struct file *filp, > > if (cmd.addr & ~PAGE_MASK) > return -EINVAL; > + if (cmd.npages > ULONG_MAX >> PAGE_SHIFT) > + return -EINVAL; > if (cmd.addr >= (cmd.addr + (cmd.npages << PAGE_SHIFT))) > return -EINVAL; Looks good to me too. Thanks for sending this. Reviewed-by: Ralph Campbell <rcampbell@nvidia.com>
diff --git a/lib/test_hmm.c b/lib/test_hmm.c index 80a78877bd93..541466034a6b 100644 --- a/lib/test_hmm.c +++ b/lib/test_hmm.c @@ -930,6 +930,8 @@ static long dmirror_fops_unlocked_ioctl(struct file *filp, if (cmd.addr & ~PAGE_MASK) return -EINVAL; + if (cmd.npages > ULONG_MAX >> PAGE_SHIFT) + return -EINVAL; if (cmd.addr >= (cmd.addr + (cmd.npages << PAGE_SHIFT))) return -EINVAL;
The "cmd.npages" variable is a u64 that comes from the user. I noticed during review that it could have a shift wrapping bug when it is used in the integer overflow test on the next line. It turns out this is harmless. The users all do: unsigned long size = cmd->npages << PAGE_SHIFT; and after that "size" is used consistently and "cmd->npages" is never used again. So even when there is an integer overflow, everything works fine. Even though this is harmless, I believe syzbot will complain and fixing it makes the code easier to read. Fixes: b2ef9f5a5cb3 ("mm/hmm/test: add selftest driver for HMM") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- lib/test_hmm.c | 2 ++ 1 file changed, 2 insertions(+)