On Sun, Jan 15, 2023 at 10:08:55PM +0800, Baoquan He wrote:
> > f181234a5a21fd0 Chen Wandun 2021-09-02 3650 if ((unsigned long)addr + count <= va->va_start)
> > f181234a5a21fd0 Chen Wandun 2021-09-02 3651 goto finished;
> > f181234a5a21fd0 Chen Wandun 2021-09-02 3652
> > f608788cd2d6cae Serapheim Dimitropoulos 2021-04-29 3653 list_for_each_entry_from(va, &vmap_area_list, list) {
> > e81ce85f960c2e2 Joonsoo Kim 2013-04-29 3654 if (!count)
> > e81ce85f960c2e2 Joonsoo Kim 2013-04-29 3655 break;
> > e81ce85f960c2e2 Joonsoo Kim 2013-04-29 3656
> > 129dbdf298d7383 Baoquan He 2023-01-13 3657 vm = va->vm;
> > 129dbdf298d7383 Baoquan He 2023-01-13 3658 flags = va->flags & VMAP_FLAGS_MASK;
> > 129dbdf298d7383 Baoquan He 2023-01-13 3659
> > 129dbdf298d7383 Baoquan He 2023-01-13 3660 if (!vm && !flags)
> > ^^^
> > vm can be NULL if a flag in VMAP_FLAGS_MASK is set.
> >
> > e81ce85f960c2e2 Joonsoo Kim 2013-04-29 3661 continue;
>
> Right, after the 'continue;' line, only two cases could happen when it
> comes here. (vm != null) or (vm->flags & VMAP_RAM) is true.
>
You're saying VMAP_RAM, but strictly speaking the code is checking
VMAP_FLAGS_MASK and not VMAP_RAM.
+#define VMAP_RAM 0x1 /* indicates vm_map_ram area*/
+#define VMAP_BLOCK 0x2 /* mark out the vmap_block sub-type*/
+#define VMAP_FLAGS_MASK 0x3
If we assume that vm is NULL, VMAP_BLOCK is set and VMAP_RAM is clear
then it would lead to a NULL dereference. There might be reasons why
that combination is impossible outside the function but we can't tell
from the information we have here.
Which is fine, outside information is a common reason for false
positives with this check. But I was just concerned about the mix of
VMAP_FLAGS_MASK and VMAP_RAM.
regards,
dan carpenter
> > e81ce85f960c2e2 Joonsoo Kim 2013-04-29 3662
> > 129dbdf298d7383 Baoquan He 2023-01-13 3663 vaddr = (char *) va->va_start;
> > 129dbdf298d7383 Baoquan He 2023-01-13 @3664 size = vm ? get_vm_area_size(vm) : va_size(va);
> > ^^
> >
> > 129dbdf298d7383 Baoquan He 2023-01-13 3665
> > 129dbdf298d7383 Baoquan He 2023-01-13 3666 if (addr >= vaddr + size)
> > ^1da177e4c3f415 Linus Torvalds 2005-04-16 3667 continue;
> > ^1da177e4c3f415 Linus Torvalds 2005-04-16 3668 while (addr < vaddr) {
> > ^1da177e4c3f415 Linus Torvalds 2005-04-16 3669 if (count == 0)
> > ^1da177e4c3f415 Linus Torvalds 2005-04-16 3670 goto finished;
> > ^1da177e4c3f415 Linus Torvalds 2005-04-16 3671 *buf = '\0';
> > ^1da177e4c3f415 Linus Torvalds 2005-04-16 3672 buf++;
> > ^1da177e4c3f415 Linus Torvalds 2005-04-16 3673 addr++;
> > ^1da177e4c3f415 Linus Torvalds 2005-04-16 3674 count--;
> > ^1da177e4c3f415 Linus Torvalds 2005-04-16 3675 }
> > 129dbdf298d7383 Baoquan He 2023-01-13 3676 n = vaddr + size - addr;
> > d0107eb07320b5d KAMEZAWA Hiroyuki 2009-09-21 3677 if (n > count)
> > d0107eb07320b5d KAMEZAWA Hiroyuki 2009-09-21 3678 n = count;
> > 129dbdf298d7383 Baoquan He 2023-01-13 3679
> > 129dbdf298d7383 Baoquan He 2023-01-13 3680 if (flags & VMAP_RAM)
> >
> > assume VMAP_RAM is not set
>
> OK, then vm is not null.
> >
> > 129dbdf298d7383 Baoquan He 2023-01-13 3681 vmap_ram_vread(buf, addr, n, flags);
> > 129dbdf298d7383 Baoquan He 2023-01-13 @3682 else if (!(vm->flags & VM_IOREMAP))
> > ^^^^^^^^^
> > Unchecked dereference. Should this be "flags" instead of "vm->flags"?
>
> Thus, here, in 'else if', vm is not null. And in this 'else if', we are
> intending to check vm->flags. I don't see issue or risk here. Please
> help check if I miss anything.
>
> Thanks
> Baoquan
>
