[RESEND PATCH v1] proc: use untagged_addr() for pagemap_read addresses

[RESEND PATCH v1] proc: use untagged_addr() for pagemap_read addresses

[Miles Chen]

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;
 
 	/*
-- 
2.18.0

Re: [RESEND PATCH v1] proc: use untagged_addr() for pagemap_read addresses

[Eric W. Biederman]

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

Re: [RESEND PATCH v1] proc: use untagged_addr() for pagemap_read addresses

[Miles Chen]

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
> 

RE: [RESEND PATCH v1] proc: use untagged_addr() for pagemap_read addresses

[Song Bao Hua (Barry Song)]

> -----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

RE: [RESEND PATCH v1] proc: use untagged_addr() for pagemap_read addresses

[Miles Chen]

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
> 

RE: [RESEND PATCH v1] proc: use untagged_addr() for pagemap_read addresses

[Song Bao Hua (Barry Song)]

> -----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

RE: [RESEND PATCH v1] proc: use untagged_addr() for pagemap_read addresses

[Song Bao Hua (Barry Song)]

> -----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

Re: [RESEND PATCH v1] proc: use untagged_addr() for pagemap_read addresses

[Catalin Marinas]

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

Re: [RESEND PATCH v1] proc: use untagged_addr() for pagemap_read addresses

[Miles Chen]

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.
>