* [bug report] io_uring: add support for fixed wait regions
@ 2024-10-30 11:40 Dan Carpenter
2024-10-30 13:22 ` Jens Axboe
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2024-10-30 11:40 UTC (permalink / raw)
To: Jens Axboe; +Cc: io-uring
Hello Jens Axboe,
Commit 4b926ab18279 ("io_uring: add support for fixed wait regions")
from Oct 22, 2024 (linux-next), leads to the following Smatch static
checker warning:
io_uring/register.c:616 io_register_cqwait_reg()
warn: was expecting a 64 bit value instead of '~(~(((1) << 12) - 1))'
io_uring/register.c
594 static int io_register_cqwait_reg(struct io_ring_ctx *ctx, void __user *uarg)
595 {
596 struct io_uring_cqwait_reg_arg arg;
597 struct io_uring_reg_wait *reg;
598 struct page **pages;
599 unsigned long len;
600 int nr_pages, poff;
601 int ret;
602
603 if (ctx->cq_wait_page || ctx->cq_wait_arg)
604 return -EBUSY;
605 if (copy_from_user(&arg, uarg, sizeof(arg)))
606 return -EFAULT;
607 if (!arg.nr_entries || arg.flags)
608 return -EINVAL;
609 if (arg.struct_size != sizeof(*reg))
610 return -EINVAL;
611 if (check_mul_overflow(arg.struct_size, arg.nr_entries, &len))
612 return -EOVERFLOW;
613 if (len > PAGE_SIZE)
614 return -EINVAL;
615 /* offset + len must fit within a page, and must be reg_wait aligned */
--> 616 poff = arg.user_addr & ~PAGE_MASK;
This is a harmless thing, but on 32 bit systems you can put whatever you want in
the high 32 bits of arg.user_addr and it won't affect anything.
617 if (len + poff > PAGE_SIZE)
618 return -EINVAL;
619 if (poff % arg.struct_size)
620 return -EINVAL;
621
622 pages = io_pin_pages(arg.user_addr, len, &nr_pages);
There ought to be a Smatch warning about that sort of thing here really but
there isn't yet.
623 if (IS_ERR(pages))
624 return PTR_ERR(pages);
625 ret = -EINVAL;
626 if (nr_pages != 1)
627 goto out_free;
628 if (ctx->user) {
629 ret = __io_account_mem(ctx->user, 1);
630 if (ret)
631 goto out_free;
632 }
633
634 reg = vmap(pages, 1, VM_MAP, PAGE_KERNEL);
635 if (reg) {
636 ctx->cq_wait_index = arg.nr_entries - 1;
637 WRITE_ONCE(ctx->cq_wait_page, pages);
638 WRITE_ONCE(ctx->cq_wait_arg, (void *) reg + poff);
639 return 0;
640 }
641 ret = -ENOMEM;
642 if (ctx->user)
643 __io_unaccount_mem(ctx->user, 1);
644 out_free:
645 io_pages_free(&pages, nr_pages);
646 return ret;
647 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] io_uring: add support for fixed wait regions
2024-10-30 11:40 [bug report] io_uring: add support for fixed wait regions Dan Carpenter
@ 2024-10-30 13:22 ` Jens Axboe
2024-10-30 13:58 ` Dan Carpenter
0 siblings, 1 reply; 4+ messages in thread
From: Jens Axboe @ 2024-10-30 13:22 UTC (permalink / raw)
To: Dan Carpenter; +Cc: io-uring
On 10/30/24 5:40 AM, Dan Carpenter wrote:
> Hello Jens Axboe,
>
> Commit 4b926ab18279 ("io_uring: add support for fixed wait regions")
> from Oct 22, 2024 (linux-next), leads to the following Smatch static
> checker warning:
>
> io_uring/register.c:616 io_register_cqwait_reg()
> warn: was expecting a 64 bit value instead of '~(~(((1) << 12) - 1))'
>
> io_uring/register.c
> 594 static int io_register_cqwait_reg(struct io_ring_ctx *ctx, void __user *uarg)
> 595 {
> 596 struct io_uring_cqwait_reg_arg arg;
> 597 struct io_uring_reg_wait *reg;
> 598 struct page **pages;
> 599 unsigned long len;
> 600 int nr_pages, poff;
> 601 int ret;
> 602
> 603 if (ctx->cq_wait_page || ctx->cq_wait_arg)
> 604 return -EBUSY;
> 605 if (copy_from_user(&arg, uarg, sizeof(arg)))
> 606 return -EFAULT;
> 607 if (!arg.nr_entries || arg.flags)
> 608 return -EINVAL;
> 609 if (arg.struct_size != sizeof(*reg))
> 610 return -EINVAL;
> 611 if (check_mul_overflow(arg.struct_size, arg.nr_entries, &len))
> 612 return -EOVERFLOW;
> 613 if (len > PAGE_SIZE)
> 614 return -EINVAL;
> 615 /* offset + len must fit within a page, and must be reg_wait aligned */
> --> 616 poff = arg.user_addr & ~PAGE_MASK;
>
> This is a harmless thing, but on 32 bit systems you can put whatever you want in
> the high 32 bits of arg.user_addr and it won't affect anything.
That is certainly true, it'll get masked away. I suspect this kind of
thing is everywhere, though? What do you suggest?
--
Jens Axboe
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] io_uring: add support for fixed wait regions
2024-10-30 13:22 ` Jens Axboe
@ 2024-10-30 13:58 ` Dan Carpenter
2024-10-30 14:17 ` Jens Axboe
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2024-10-30 13:58 UTC (permalink / raw)
To: Jens Axboe; +Cc: io-uring
On Wed, Oct 30, 2024 at 07:22:49AM -0600, Jens Axboe wrote:
> On 10/30/24 5:40 AM, Dan Carpenter wrote:
> > Hello Jens Axboe,
> >
> > Commit 4b926ab18279 ("io_uring: add support for fixed wait regions")
> > from Oct 22, 2024 (linux-next), leads to the following Smatch static
> > checker warning:
> >
> > io_uring/register.c:616 io_register_cqwait_reg()
> > warn: was expecting a 64 bit value instead of '~(~(((1) << 12) - 1))'
> >
> > io_uring/register.c
> > 594 static int io_register_cqwait_reg(struct io_ring_ctx *ctx, void __user *uarg)
> > 595 {
> > 596 struct io_uring_cqwait_reg_arg arg;
> > 597 struct io_uring_reg_wait *reg;
> > 598 struct page **pages;
> > 599 unsigned long len;
> > 600 int nr_pages, poff;
> > 601 int ret;
> > 602
> > 603 if (ctx->cq_wait_page || ctx->cq_wait_arg)
> > 604 return -EBUSY;
> > 605 if (copy_from_user(&arg, uarg, sizeof(arg)))
> > 606 return -EFAULT;
> > 607 if (!arg.nr_entries || arg.flags)
> > 608 return -EINVAL;
> > 609 if (arg.struct_size != sizeof(*reg))
> > 610 return -EINVAL;
> > 611 if (check_mul_overflow(arg.struct_size, arg.nr_entries, &len))
> > 612 return -EOVERFLOW;
> > 613 if (len > PAGE_SIZE)
> > 614 return -EINVAL;
> > 615 /* offset + len must fit within a page, and must be reg_wait aligned */
> > --> 616 poff = arg.user_addr & ~PAGE_MASK;
> >
> > This is a harmless thing, but on 32 bit systems you can put whatever you want in
> > the high 32 bits of arg.user_addr and it won't affect anything.
>
> That is certainly true, it'll get masked away. I suspect this kind of
> thing is everywhere, though? What do you suggest?
The way that I normally see these warnings is with code like
"if (u64flags & ~mask)" where only the first 3 bits of u64flags are used. It's
not normally a real life bug. Normally fix them the warning, but I have 174 old
warnings from before I started complaining about them.
Maybe:
if (U32_MAX >= SIZE_MAX && arg.user_addr > SIZE_MAX)
return -EINVAL;
This code works fine as-is, but eventually I want this code to trigger a couple
more static checker warnings. It's so suspicious because we're truncating user
data then re-using the same untruncated variable again.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] io_uring: add support for fixed wait regions
2024-10-30 13:58 ` Dan Carpenter
@ 2024-10-30 14:17 ` Jens Axboe
0 siblings, 0 replies; 4+ messages in thread
From: Jens Axboe @ 2024-10-30 14:17 UTC (permalink / raw)
To: Dan Carpenter; +Cc: io-uring
On 10/30/24 7:58 AM, Dan Carpenter wrote:
> On Wed, Oct 30, 2024 at 07:22:49AM -0600, Jens Axboe wrote:
>> On 10/30/24 5:40 AM, Dan Carpenter wrote:
>>> Hello Jens Axboe,
>>>
>>> Commit 4b926ab18279 ("io_uring: add support for fixed wait regions")
>>> from Oct 22, 2024 (linux-next), leads to the following Smatch static
>>> checker warning:
>>>
>>> io_uring/register.c:616 io_register_cqwait_reg()
>>> warn: was expecting a 64 bit value instead of '~(~(((1) << 12) - 1))'
>>>
>>> io_uring/register.c
>>> 594 static int io_register_cqwait_reg(struct io_ring_ctx *ctx, void __user *uarg)
>>> 595 {
>>> 596 struct io_uring_cqwait_reg_arg arg;
>>> 597 struct io_uring_reg_wait *reg;
>>> 598 struct page **pages;
>>> 599 unsigned long len;
>>> 600 int nr_pages, poff;
>>> 601 int ret;
>>> 602
>>> 603 if (ctx->cq_wait_page || ctx->cq_wait_arg)
>>> 604 return -EBUSY;
>>> 605 if (copy_from_user(&arg, uarg, sizeof(arg)))
>>> 606 return -EFAULT;
>>> 607 if (!arg.nr_entries || arg.flags)
>>> 608 return -EINVAL;
>>> 609 if (arg.struct_size != sizeof(*reg))
>>> 610 return -EINVAL;
>>> 611 if (check_mul_overflow(arg.struct_size, arg.nr_entries, &len))
>>> 612 return -EOVERFLOW;
>>> 613 if (len > PAGE_SIZE)
>>> 614 return -EINVAL;
>>> 615 /* offset + len must fit within a page, and must be reg_wait aligned */
>>> --> 616 poff = arg.user_addr & ~PAGE_MASK;
>>>
>>> This is a harmless thing, but on 32 bit systems you can put whatever you want in
>>> the high 32 bits of arg.user_addr and it won't affect anything.
>>
>> That is certainly true, it'll get masked away. I suspect this kind of
>> thing is everywhere, though? What do you suggest?
>
> The way that I normally see these warnings is with code like "if
> (u64flags & ~mask)" where only the first 3 bits of u64flags are used.
> It's not normally a real life bug. Normally fix them the warning, but
> I have 174 old warnings from before I started complaining about them.
>
> Maybe:
>
> if (U32_MAX >= SIZE_MAX && arg.user_addr > SIZE_MAX)
> return -EINVAL;
>
> This code works fine as-is, but eventually I want this code to trigger
> a couple more static checker warnings. It's so suspicious because
> we're truncating user data then re-using the same untruncated variable
> again.
Agree, that's the part I don't like. It's fine masking off for offset,
but the later passing in directly is wonky. It'll get truncated to
unsigned long at that point though, so won't _actually_ be passed in.
Hence I'm a bit dubious that this really needs fixing. Yeah the app can
put garbage in the upper 32-bits, but it's never going to be seen.
Should we catch and -EINVAL on that? Potentially, at least it can't hurt
to do so.
--
Jens Axboe
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-10-30 14:17 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-10-30 11:40 [bug report] io_uring: add support for fixed wait regions Dan Carpenter
2024-10-30 13:22 ` Jens Axboe
2024-10-30 13:58 ` Dan Carpenter
2024-10-30 14:17 ` Jens Axboe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox