Messages in this thread | | | From | Catalin Marinas <> | Date | Thu, 26 Nov 2020 11:10:09 +0000 | Subject | Re: [RESEND PATCH v1] proc: use untagged_addr() for pagemap_read addresses |
| |
Hi Miles,
Could you please cc me and Andrey Konovalov on future versions of this patch (if any)?
On Mon, 23 Nov 2020 at 08:47, Miles Chen <miles.chen@mediatek.com> wrote: > When we try to visit the pagemap of a tagged userspace pointer, we find > that the start_vaddr is not correct because of the tag. > To fix it, we should untag the usespace pointers in pagemap_read(). > > I tested with 5.10-rc4 and the issue remains. > > My test code is baed on [1]: > > A userspace pointer which has been tagged by 0xb4: 0xb400007662f541c8 > > === userspace program === > > uint64 OsLayer::VirtualToPhysical(void *vaddr) { > uint64 frame, paddr, pfnmask, pagemask; > int pagesize = sysconf(_SC_PAGESIZE); > off64_t off = ((uintptr_t)vaddr) / pagesize * 8; // off = 0xb400007662f541c8 / pagesize * 8 = 0x5a00003b317aa0
Arguably, that's a user-space bug since tagged file offsets were never supported. In this case it's not even a tag at bit 56 as per the arm64 tagged address ABI but rather down to bit 47. You could say that the problem is caused by the C library (malloc()) or whoever created the tagged vaddr and passed it to this function. It's not a kernel regression as we've never supported it.
Now, pagemap is a special case where the offset is usually not generated as a classic file offset but rather derived by shifting a user virtual address. I guess we can make a concession for pagemap (only) and allow such offset with the tag at bit (56 - PAGE_SHIFT + 3).
Please fix the patch as per Eric's suggestion on avoiding the overflow. You should also add a Cc: stable v5.4- as that's when we enabled the tagged address ABI on arm64 and when it's more likely for the C library/malloc() to start generating such pointers.
If the problem is only limited to this test, I'd rather fix the user but I can't tell how widespread the /proc/pid/pagemap usage is.
Thanks.
-- Catalin
| |