| Message ID | 20201123063835.18981-1-miles.chen@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND,v1] proc: use untagged_addr() for pagemap_read addresses | expand |
Miles Chen <miles.chen@mediatek.com> writes: > 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 Sigh this patch is buggy. > === 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 > int fd = open(kPagemapPath, O_RDONLY); > ... > > if (lseek64(fd, off, SEEK_SET) != off || read(fd, &frame, 8) != 8) { > int err = errno; > string errtxt = ErrorString(err); > if (fd >= 0) > close(fd); > return 0; > } > ... > } > > === kernel fs/proc/task_mmu.c === > > static ssize_t pagemap_read(struct file *file, char __user *buf, > size_t count, loff_t *ppos) > { > ... > src = *ppos; > svpfn = src / PM_ENTRY_BYTES; // svpfn == 0xb400007662f54 > start_vaddr = svpfn << PAGE_SHIFT; // start_vaddr == 0xb400007662f54000 > end_vaddr = mm->task_size; > > /* watch out for wraparound */ > // svpfn == 0xb400007662f54 > // (mm->task_size >> PAGE) == 0x8000000 > if (svpfn > mm->task_size >> PAGE_SHIFT) // the condition is true because of the tag 0xb4 > start_vaddr = end_vaddr; > > ret = 0; > while (count && (start_vaddr < end_vaddr)) { // we cannot visit correct entry because start_vaddr is set to end_vaddr > int len; > unsigned long end; > ... > } > ... > } > > [1] https://github.com/stressapptest/stressapptest/blob/master/src/os.cc#L158 > > Signed-off-by: Miles Chen <miles.chen@mediatek.com> > --- > fs/proc/task_mmu.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c > index 217aa2705d5d..e9a70f7ee515 100644 > --- a/fs/proc/task_mmu.c > +++ b/fs/proc/task_mmu.c > @@ -1599,11 +1599,11 @@ static ssize_t pagemap_read(struct file *file, char __user *buf, > > src = *ppos; > svpfn = src / PM_ENTRY_BYTES; > - start_vaddr = svpfn << PAGE_SHIFT; > + start_vaddr = untagged_addr(svpfn << PAGE_SHIFT); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Arguably the line above is safe, but unfortunately it has the possibility of suffering from overflow. > end_vaddr = mm->task_size; > > /* watch out for wraparound */ > - if (svpfn > mm->task_size >> PAGE_SHIFT) > + if (start_vaddr > mm->task_size) > start_vaddr = end_vaddr; Overflow handling you are removing here. > > /* I suspect the proper way to handle this is to move the test for overflow earlier so the code looks something like: end_vaddr = mm->task_size; src = *ppos; svpfn = src / PM_ENTRY_BYTES; /* watch out for wraparound */ start_vaddr = end_vaddr; if (svpfn < (ULONG_MAX >> PAGE_SHIFT)) start_vaddr = untagged_addr(svpfn << PAGE_SHIFT); /* Ensure the address is inside the task */ if (start_vaddr > mm->task_size) start_vaddr = end_vaddr; Eric
> -----Original Message----- > From: Miles Chen [mailto:miles.chen@mediatek.com] > Sent: Monday, November 23, 2020 7:39 PM > To: Alexey Dobriyan <adobriyan@gmail.com>; Andrew Morton > <akpm@linux-foundation.org> > Cc: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org; > linux-mediatek@lists.infradead.org; wsd_upstream@mediatek.com; Miles > Chen <miles.chen@mediatek.com> > Subject: [RESEND PATCH v1] proc: use untagged_addr() for pagemap_read > addresses > > 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 > int fd = open(kPagemapPath, O_RDONLY); > ... > > if (lseek64(fd, off, SEEK_SET) != off || read(fd, &frame, 8) != 8) { > int err = errno; > string errtxt = ErrorString(err); > if (fd >= 0) > close(fd); > return 0; > } > ... > } > > === kernel fs/proc/task_mmu.c === > > static ssize_t pagemap_read(struct file *file, char __user *buf, > size_t count, loff_t *ppos) > { > ... > src = *ppos; > svpfn = src / PM_ENTRY_BYTES; // svpfn == 0xb400007662f54 > start_vaddr = svpfn << PAGE_SHIFT; // start_vaddr == > 0xb400007662f54000 > end_vaddr = mm->task_size; > > /* watch out for wraparound */ > // svpfn == 0xb400007662f54 > // (mm->task_size >> PAGE) == 0x8000000 > if (svpfn > mm->task_size >> PAGE_SHIFT) // the condition is true because > of the tag 0xb4 > start_vaddr = end_vaddr; > > ret = 0; > while (count && (start_vaddr < end_vaddr)) { // we cannot visit correct > entry because start_vaddr is set to end_vaddr > int len; > unsigned long end; > ... > } > ... > } > > [1] > https://github.com/stressapptest/stressapptest/blob/master/src/os.cc#L158 > > Signed-off-by: Miles Chen <miles.chen@mediatek.com> > --- > fs/proc/task_mmu.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c > index 217aa2705d5d..e9a70f7ee515 100644 > --- a/fs/proc/task_mmu.c > +++ b/fs/proc/task_mmu.c > @@ -1599,11 +1599,11 @@ static ssize_t pagemap_read(struct file *file, char > __user *buf, > > src = *ppos; > svpfn = src / PM_ENTRY_BYTES; > - start_vaddr = svpfn << PAGE_SHIFT; > + start_vaddr = untagged_addr(svpfn << PAGE_SHIFT); > end_vaddr = mm->task_size; > > /* watch out for wraparound */ > - if (svpfn > mm->task_size >> PAGE_SHIFT) > + if (start_vaddr > mm->task_size) > start_vaddr = end_vaddr; Wouldn't the untag be done by the user reading pagemap file? With this patch, even users pass an illegal address, for example, users put a tag on a virtual address which hasn't really a tag, they will still get the right pagemap. > > /* > -- > 2.18.0 Thanks Barry
On Thu, 2020-11-26 at 07:16 +0000, Song Bao Hua (Barry Song) wrote: > > > -----Original Message----- > > From: Miles Chen [mailto:miles.chen@mediatek.com] > > Sent: Monday, November 23, 2020 7:39 PM > > To: Alexey Dobriyan <adobriyan@gmail.com>; Andrew Morton > > <akpm@linux-foundation.org> > > Cc: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org; > > linux-mediatek@lists.infradead.org; wsd_upstream@mediatek.com; Miles > > Chen <miles.chen@mediatek.com> > > Subject: [RESEND PATCH v1] proc: use untagged_addr() for pagemap_read > > addresses > > > > 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 > > int fd = open(kPagemapPath, O_RDONLY); > > ... > > > > if (lseek64(fd, off, SEEK_SET) != off || read(fd, &frame, 8) != 8) { > > int err = errno; > > string errtxt = ErrorString(err); > > if (fd >= 0) > > close(fd); > > return 0; > > } > > ... > > } > > > > === kernel fs/proc/task_mmu.c === > > > > static ssize_t pagemap_read(struct file *file, char __user *buf, > > size_t count, loff_t *ppos) > > { > > ... > > src = *ppos; > > svpfn = src / PM_ENTRY_BYTES; // svpfn == 0xb400007662f54 > > start_vaddr = svpfn << PAGE_SHIFT; // start_vaddr == > > 0xb400007662f54000 > > end_vaddr = mm->task_size; > > > > /* watch out for wraparound */ > > // svpfn == 0xb400007662f54 > > // (mm->task_size >> PAGE) == 0x8000000 > > if (svpfn > mm->task_size >> PAGE_SHIFT) // the condition is true because > > of the tag 0xb4 > > start_vaddr = end_vaddr; > > > > ret = 0; > > while (count && (start_vaddr < end_vaddr)) { // we cannot visit correct > > entry because start_vaddr is set to end_vaddr > > int len; > > unsigned long end; > > ... > > } > > ... > > } > > > > [1] > > https://github.com/stressapptest/stressapptest/blob/master/src/os.cc#L158 > > > > Signed-off-by: Miles Chen <miles.chen@mediatek.com> > > --- > > fs/proc/task_mmu.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c > > index 217aa2705d5d..e9a70f7ee515 100644 > > --- a/fs/proc/task_mmu.c > > +++ b/fs/proc/task_mmu.c > > @@ -1599,11 +1599,11 @@ static ssize_t pagemap_read(struct file *file, char > > __user *buf, > > > > src = *ppos; > > svpfn = src / PM_ENTRY_BYTES; > > - start_vaddr = svpfn << PAGE_SHIFT; > > + start_vaddr = untagged_addr(svpfn << PAGE_SHIFT); > > end_vaddr = mm->task_size; > > > > /* watch out for wraparound */ > > - if (svpfn > mm->task_size >> PAGE_SHIFT) > > + if (start_vaddr > mm->task_size) > > start_vaddr = end_vaddr; > > Wouldn't the untag be done by the user reading pagemap file? > With this patch, even users pass an illegal address, for example, > users put a tag on a virtual address which hasn't really a tag, > they will still get the right pagemap. > I run arm64 devices. If the tagged pointer is enabled, the ARM top-byte Ignore is also enabled. It means the top-byte is always be ignored. I think the kernel should not break existing userspace program, so it's better to handle the tagged user pointer in kernel. grep 'untag' mm -RnH to see existing examples. thanks, Miles > > > > /* > > -- > > 2.18.0 > > Thanks > Barry >
> -----Original Message----- > From: Miles Chen [mailto:miles.chen@mediatek.com] > Sent: Thursday, November 26, 2020 8:49 PM > To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com> > Cc: Alexey Dobriyan <adobriyan@gmail.com>; Andrew Morton > <akpm@linux-foundation.org>; linux-kernel@vger.kernel.org; > linux-fsdevel@vger.kernel.org; linux-mediatek@lists.infradead.org; > wsd_upstream@mediatek.com > Subject: RE: [RESEND PATCH v1] proc: use untagged_addr() for pagemap_read > addresses > > On Thu, 2020-11-26 at 07:16 +0000, Song Bao Hua (Barry Song) wrote: > > > > > -----Original Message----- > > > From: Miles Chen [mailto:miles.chen@mediatek.com] > > > Sent: Monday, November 23, 2020 7:39 PM > > > To: Alexey Dobriyan <adobriyan@gmail.com>; Andrew Morton > > > <akpm@linux-foundation.org> > > > Cc: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org; > > > linux-mediatek@lists.infradead.org; wsd_upstream@mediatek.com; Miles > > > Chen <miles.chen@mediatek.com> > > > Subject: [RESEND PATCH v1] proc: use untagged_addr() for pagemap_read > > > addresses > > > > > > 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 > > > int fd = open(kPagemapPath, O_RDONLY); > > > ... > > > > > > if (lseek64(fd, off, SEEK_SET) != off || read(fd, &frame, 8) != 8) { > > > int err = errno; > > > string errtxt = ErrorString(err); > > > if (fd >= 0) > > > close(fd); > > > return 0; > > > } > > > ... > > > } > > > > > > === kernel fs/proc/task_mmu.c === > > > > > > static ssize_t pagemap_read(struct file *file, char __user *buf, > > > size_t count, loff_t *ppos) > > > { > > > ... > > > src = *ppos; > > > svpfn = src / PM_ENTRY_BYTES; // svpfn == 0xb400007662f54 > > > start_vaddr = svpfn << PAGE_SHIFT; // start_vaddr == > > > 0xb400007662f54000 > > > end_vaddr = mm->task_size; > > > > > > /* watch out for wraparound */ > > > // svpfn == 0xb400007662f54 > > > // (mm->task_size >> PAGE) == 0x8000000 > > > if (svpfn > mm->task_size >> PAGE_SHIFT) // the condition is true because > > > of the tag 0xb4 > > > start_vaddr = end_vaddr; > > > > > > ret = 0; > > > while (count && (start_vaddr < end_vaddr)) { // we cannot visit correct > > > entry because start_vaddr is set to end_vaddr > > > int len; > > > unsigned long end; > > > ... > > > } > > > ... > > > } > > > > > > [1] > > > > https://github.com/stressapptest/stressapptest/blob/master/src/os.cc#L158 > > > > > > Signed-off-by: Miles Chen <miles.chen@mediatek.com> > > > --- > > > fs/proc/task_mmu.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c > > > index 217aa2705d5d..e9a70f7ee515 100644 > > > --- a/fs/proc/task_mmu.c > > > +++ b/fs/proc/task_mmu.c > > > @@ -1599,11 +1599,11 @@ static ssize_t pagemap_read(struct file *file, > char > > > __user *buf, > > > > > > src = *ppos; > > > svpfn = src / PM_ENTRY_BYTES; > > > - start_vaddr = svpfn << PAGE_SHIFT; > > > + start_vaddr = untagged_addr(svpfn << PAGE_SHIFT); > > > end_vaddr = mm->task_size; > > > > > > /* watch out for wraparound */ > > > - if (svpfn > mm->task_size >> PAGE_SHIFT) > > > + if (start_vaddr > mm->task_size) > > > start_vaddr = end_vaddr; > > > > Wouldn't the untag be done by the user reading pagemap file? > > With this patch, even users pass an illegal address, for example, > > users put a tag on a virtual address which hasn't really a tag, > > they will still get the right pagemap. > > > > > I run arm64 devices. > If the tagged pointer is enabled, the ARM top-byte Ignore is also > enabled. It means the top-byte is always be ignored. > > I think the kernel should not break existing userspace program, so it's > better to handle the tagged user pointer in kernel. > > grep 'untag' mm -RnH to see existing examples. I see your point. But the existing examples such as gup etc are receiving an address from userspace. pagemap isn't getting an address, it is getting a file offset which should be figured out by userspace correctly. While we read a file, we should make sure the offset is in the range of the file length. > > > thanks, > Miles > > > > > > > /* > > > -- > > > 2.18.0 > > Thanks Barry
> -----Original Message----- > From: Song Bao Hua (Barry Song) > Sent: Thursday, November 26, 2020 8:53 PM > To: 'Miles Chen' <miles.chen@mediatek.com> > Cc: Alexey Dobriyan <adobriyan@gmail.com>; Andrew Morton > <akpm@linux-foundation.org>; linux-kernel@vger.kernel.org; > linux-fsdevel@vger.kernel.org; linux-mediatek@lists.infradead.org; > wsd_upstream@mediatek.com > Subject: RE: [RESEND PATCH v1] proc: use untagged_addr() for pagemap_read > addresses > > > > > -----Original Message----- > > From: Miles Chen [mailto:miles.chen@mediatek.com] > > Sent: Thursday, November 26, 2020 8:49 PM > > To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com> > > Cc: Alexey Dobriyan <adobriyan@gmail.com>; Andrew Morton > > <akpm@linux-foundation.org>; linux-kernel@vger.kernel.org; > > linux-fsdevel@vger.kernel.org; linux-mediatek@lists.infradead.org; > > wsd_upstream@mediatek.com > > Subject: RE: [RESEND PATCH v1] proc: use untagged_addr() for > pagemap_read > > addresses > > > > On Thu, 2020-11-26 at 07:16 +0000, Song Bao Hua (Barry Song) wrote: > > > > > > > -----Original Message----- > > > > From: Miles Chen [mailto:miles.chen@mediatek.com] > > > > Sent: Monday, November 23, 2020 7:39 PM > > > > To: Alexey Dobriyan <adobriyan@gmail.com>; Andrew Morton > > > > <akpm@linux-foundation.org> > > > > Cc: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org; > > > > linux-mediatek@lists.infradead.org; wsd_upstream@mediatek.com; Miles > > > > Chen <miles.chen@mediatek.com> > > > > Subject: [RESEND PATCH v1] proc: use untagged_addr() for pagemap_read > > > > addresses > > > > > > > > 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 > > > > int fd = open(kPagemapPath, O_RDONLY); > > > > ... > > > > > > > > if (lseek64(fd, off, SEEK_SET) != off || read(fd, &frame, 8) != 8) { > > > > int err = errno; > > > > string errtxt = ErrorString(err); > > > > if (fd >= 0) > > > > close(fd); > > > > return 0; > > > > } > > > > ... > > > > } > > > > > > > > === kernel fs/proc/task_mmu.c === > > > > > > > > static ssize_t pagemap_read(struct file *file, char __user *buf, > > > > size_t count, loff_t *ppos) > > > > { > > > > ... > > > > src = *ppos; > > > > svpfn = src / PM_ENTRY_BYTES; // svpfn == 0xb400007662f54 > > > > start_vaddr = svpfn << PAGE_SHIFT; // start_vaddr == > > > > 0xb400007662f54000 > > > > end_vaddr = mm->task_size; > > > > > > > > /* watch out for wraparound */ > > > > // svpfn == 0xb400007662f54 > > > > // (mm->task_size >> PAGE) == 0x8000000 > > > > if (svpfn > mm->task_size >> PAGE_SHIFT) // the condition is true > because > > > > of the tag 0xb4 > > > > start_vaddr = end_vaddr; > > > > > > > > ret = 0; > > > > while (count && (start_vaddr < end_vaddr)) { // we cannot visit > correct > > > > entry because start_vaddr is set to end_vaddr > > > > int len; > > > > unsigned long end; > > > > ... > > > > } > > > > ... > > > > } > > > > > > > > [1] > > > > > > https://github.com/stressapptest/stressapptest/blob/master/src/os.cc#L158 > > > > > > > > Signed-off-by: Miles Chen <miles.chen@mediatek.com> > > > > --- > > > > fs/proc/task_mmu.c | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c > > > > index 217aa2705d5d..e9a70f7ee515 100644 > > > > --- a/fs/proc/task_mmu.c > > > > +++ b/fs/proc/task_mmu.c > > > > @@ -1599,11 +1599,11 @@ static ssize_t pagemap_read(struct file *file, > > char > > > > __user *buf, > > > > > > > > src = *ppos; > > > > svpfn = src / PM_ENTRY_BYTES; > > > > - start_vaddr = svpfn << PAGE_SHIFT; > > > > + start_vaddr = untagged_addr(svpfn << PAGE_SHIFT); > > > > end_vaddr = mm->task_size; > > > > > > > > /* watch out for wraparound */ > > > > - if (svpfn > mm->task_size >> PAGE_SHIFT) > > > > + if (start_vaddr > mm->task_size) > > > > start_vaddr = end_vaddr; > > > > > > Wouldn't the untag be done by the user reading pagemap file? > > > With this patch, even users pass an illegal address, for example, > > > users put a tag on a virtual address which hasn't really a tag, > > > they will still get the right pagemap. > > > > > > > > > I run arm64 devices. > > If the tagged pointer is enabled, the ARM top-byte Ignore is also > > enabled. It means the top-byte is always be ignored. > > > > I think the kernel should not break existing userspace program, so it's > > better to handle the tagged user pointer in kernel. > > > > grep 'untag' mm -RnH to see existing examples. > > I see your point. But the existing examples such as gup etc are receiving > an address from userspace. > > pagemap isn't getting an address, it is getting a file offset which > should be figured out by userspace correctly. While we read a file, > we should make sure the offset is in the range of the file length. BTW, personally I don't object to this patch. I just feel a little bit weird as the parameter in pagemap_read is an offset in one file rather than a virtual address. if there are many user programs which are written without the awareness of tag while reading pagemap or if other people like this patch, it might make sense to have it in kernel :-) > > > > > > > thanks, > > Miles > > > > > > > > > > /* > > > > -- > > > > 2.18.0 > > > > Thanks Barry
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.
On Tue, 2020-11-24 at 12:32 -0600, Eric W. Biederman wrote: > Miles Chen <miles.chen@mediatek.com> writes: > > > 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 > > > Sigh this patch is buggy. > > > === 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 > > int fd = open(kPagemapPath, O_RDONLY); > > ... > > > > if (lseek64(fd, off, SEEK_SET) != off || read(fd, &frame, 8) != 8) { > > int err = errno; > > string errtxt = ErrorString(err); > > if (fd >= 0) > > close(fd); > > return 0; > > } > > ... > > } > > > > === kernel fs/proc/task_mmu.c === > > > > static ssize_t pagemap_read(struct file *file, char __user *buf, > > size_t count, loff_t *ppos) > > { > > ... > > src = *ppos; > > svpfn = src / PM_ENTRY_BYTES; // svpfn == 0xb400007662f54 > > start_vaddr = svpfn << PAGE_SHIFT; // start_vaddr == 0xb400007662f54000 > > end_vaddr = mm->task_size; > > > > /* watch out for wraparound */ > > // svpfn == 0xb400007662f54 > > // (mm->task_size >> PAGE) == 0x8000000 > > if (svpfn > mm->task_size >> PAGE_SHIFT) // the condition is true because of the tag 0xb4 > > start_vaddr = end_vaddr; > > > > ret = 0; > > while (count && (start_vaddr < end_vaddr)) { // we cannot visit correct entry because start_vaddr is set to end_vaddr > > int len; > > unsigned long end; > > ... > > } > > ... > > } > > > > [1] https://github.com/stressapptest/stressapptest/blob/master/src/os.cc#L158 > > > > Signed-off-by: Miles Chen <miles.chen@mediatek.com> > > --- > > fs/proc/task_mmu.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c > > index 217aa2705d5d..e9a70f7ee515 100644 > > --- a/fs/proc/task_mmu.c > > +++ b/fs/proc/task_mmu.c > > @@ -1599,11 +1599,11 @@ static ssize_t pagemap_read(struct file *file, char __user *buf, > > > > src = *ppos; > > svpfn = src / PM_ENTRY_BYTES; > > > - start_vaddr = svpfn << PAGE_SHIFT; > > + start_vaddr = untagged_addr(svpfn << PAGE_SHIFT); > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > Arguably the line above is safe, but unfortunately it has the > possibility of suffering from overflow. > > > end_vaddr = mm->task_size; > > > > /* watch out for wraparound */ > > - if (svpfn > mm->task_size >> PAGE_SHIFT) > > + if (start_vaddr > mm->task_size) > > start_vaddr = end_vaddr; > > Overflow handling you are removing here. > > > > /* > > > I suspect the proper way to handle this is to move the test for > overflow earlier so the code looks something like: > > end_vaddr = mm->task_size; > > src = *ppos; > svpfn = src / PM_ENTRY_BYTES; > > /* watch out for wraparound */ > start_vaddr = end_vaddr; > if (svpfn < (ULONG_MAX >> PAGE_SHIFT)) > start_vaddr = untagged_addr(svpfn << PAGE_SHIFT); > > /* Ensure the address is inside the task */ > if (start_vaddr > mm->task_size) > start_vaddr = end_vaddr; Thanks for the comment, I will fix that in patch v2. Miles > > Eric >
On Thu, 2020-11-26 at 11:10 +0000, Catalin Marinas wrote: > 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. thanks for the explaination. > > 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. Got it, thanks for your reviewing and suggestion. I will follow Eric's suggestion and submit patch v2 and cc stable v5.4- Miles > > 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. >
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c index 217aa2705d5d..e9a70f7ee515 100644 --- a/fs/proc/task_mmu.c +++ b/fs/proc/task_mmu.c @@ -1599,11 +1599,11 @@ static ssize_t pagemap_read(struct file *file, char __user *buf, src = *ppos; svpfn = src / PM_ENTRY_BYTES; - start_vaddr = svpfn << PAGE_SHIFT; + start_vaddr = untagged_addr(svpfn << PAGE_SHIFT); end_vaddr = mm->task_size; /* watch out for wraparound */ - if (svpfn > mm->task_size >> PAGE_SHIFT) + if (start_vaddr > mm->task_size) start_vaddr = end_vaddr; /*
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 int fd = open(kPagemapPath, O_RDONLY); ... if (lseek64(fd, off, SEEK_SET) != off || read(fd, &frame, 8) != 8) { int err = errno; string errtxt = ErrorString(err); if (fd >= 0) close(fd); return 0; } ... } === kernel fs/proc/task_mmu.c === static ssize_t pagemap_read(struct file *file, char __user *buf, size_t count, loff_t *ppos) { ... src = *ppos; svpfn = src / PM_ENTRY_BYTES; // svpfn == 0xb400007662f54 start_vaddr = svpfn << PAGE_SHIFT; // start_vaddr == 0xb400007662f54000 end_vaddr = mm->task_size; /* watch out for wraparound */ // svpfn == 0xb400007662f54 // (mm->task_size >> PAGE) == 0x8000000 if (svpfn > mm->task_size >> PAGE_SHIFT) // the condition is true because of the tag 0xb4 start_vaddr = end_vaddr; ret = 0; while (count && (start_vaddr < end_vaddr)) { // we cannot visit correct entry because start_vaddr is set to end_vaddr int len; unsigned long end; ... } ... } [1] https://github.com/stressapptest/stressapptest/blob/master/src/os.cc#L158 Signed-off-by: Miles Chen <miles.chen@mediatek.com> --- fs/proc/task_mmu.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)