[BUG] arm64: an infinite loop in generic_perform_write()

[BUG] arm64: an infinite loop in generic_perform_write()

[Chen Huang]

When we access a device memory in userspace, then perform an unaligned write to a file.
For example, we register a uio device and mmap the device, then perform an write to a
file, like that:

	device_addr = mmap(device_fd);
	write(file_fd, device_addr + unaligned_num, size);
	
We found that the infinite loop happened in generic_perform_write function:

again:
	copied = copy_page_from_iter_atomic(); //copied = 0
	status = ops->write_end(); //status = 0
	if (status == 0)
		goto again;

In copy_page_from_iter_atomic, the copyin() function finally call
__arch_copy_from_user which create an exception table entry for 'insn'.
Then when kernel handles the alignment_fault, it will not panic. As the
arm64 memory model spec said, when the address is not a multiple of the
element size, the access is unaligned. Unaligned accesses are allowed to
addresses marked as Normal, but not to Device regions. An unaligned access
to a Device region will trigger an exception (alignment fault).
	
do_alignment_fault
    do_bad_area
	__do_kernel_fault
           fixup_exception

But that fixup cann't handle the unaligned copy, so the
copy_page_from_iter_atomic returns 0 and traps in loop.

Reported-by: Chen Huang <chenhuang5@huawei.com>

[BUG] arm64: an infinite loop in generic_perform_write()

[Chen Huang]

When we access a device memory in userspace, then perform an unaligned write to a file.
For example, we register a uio device and mmap the device, then perform an write to a
file, like that:

	device_addr = mmap(device_fd);
	write(file_fd, device_addr + unaligned_num, size);
	
We found that the infinite loop happened in generic_perform_write function:

again:
	copied = copy_page_from_iter_atomic(); //copied = 0
	status = ops->write_end(); //status = 0
	if (status == 0)
		goto again;

In copy_page_from_iter_atomic, the copyin() function finally call
__arch_copy_from_user which create an exception table entry for 'insn'.
Then when kernel handles the alignment_fault, it will not panic. As the
arm64 memory model spec said, when the address is not a multiple of the
element size, the access is unaligned. Unaligned accesses are allowed to
addresses marked as Normal, but not to Device regions. An unaligned access
to a Device region will trigger an exception (alignment fault).
	
do_alignment_fault
    do_bad_area
	__do_kernel_fault
           fixup_exception

But that fixup cann't handle the unaligned copy, so the
copy_page_from_iter_atomic returns 0 and traps in loop.

Reported-by: Chen Huang <chenhuang5@huawei.com>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Al Viro]

On Wed, Jun 23, 2021 at 10:39:31AM +0800, Chen Huang wrote:

> Then when kernel handles the alignment_fault, it will not panic. As the
> arm64 memory model spec said, when the address is not a multiple of the
> element size, the access is unaligned. Unaligned accesses are allowed to
> addresses marked as Normal, but not to Device regions. An unaligned access
> to a Device region will trigger an exception (alignment fault).
> 	
> do_alignment_fault
>     do_bad_area
> 	__do_kernel_fault
>            fixup_exception
> 
> But that fixup cann't handle the unaligned copy, so the
> copy_page_from_iter_atomic returns 0 and traps in loop.

Looks like you need to fix your raw_copy_from_user(), then...

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Al Viro]

On Wed, Jun 23, 2021 at 10:39:31AM +0800, Chen Huang wrote:

> Then when kernel handles the alignment_fault, it will not panic. As the
> arm64 memory model spec said, when the address is not a multiple of the
> element size, the access is unaligned. Unaligned accesses are allowed to
> addresses marked as Normal, but not to Device regions. An unaligned access
> to a Device region will trigger an exception (alignment fault).
> 	
> do_alignment_fault
>     do_bad_area
> 	__do_kernel_fault
>            fixup_exception
> 
> But that fixup cann't handle the unaligned copy, so the
> copy_page_from_iter_atomic returns 0 and traps in loop.

Looks like you need to fix your raw_copy_from_user(), then...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Xiaoming Ni]

On 2021/6/23 10:50, Al Viro wrote:
> On Wed, Jun 23, 2021 at 10:39:31AM +0800, Chen Huang wrote:
> 
>> Then when kernel handles the alignment_fault, it will not panic. As the
>> arm64 memory model spec said, when the address is not a multiple of the
>> element size, the access is unaligned. Unaligned accesses are allowed to
>> addresses marked as Normal, but not to Device regions. An unaligned access
>> to a Device region will trigger an exception (alignment fault).
>> 	
>> do_alignment_fault
>>      do_bad_area
>> 	__do_kernel_fault
>>             fixup_exception
>>
>> But that fixup cann't handle the unaligned copy, so the
>> copy_page_from_iter_atomic returns 0 and traps in loop.
> 
> Looks like you need to fix your raw_copy_from_user(), then...
> .
> 

Exit loop when iov_iter_copy_from_user_atomic() returns 0.
This should solve the problem, too, and it's easier.

Thanks.
Xiaoming Ni





	

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Xiaoming Ni]

On 2021/6/23 10:50, Al Viro wrote:
> On Wed, Jun 23, 2021 at 10:39:31AM +0800, Chen Huang wrote:
> 
>> Then when kernel handles the alignment_fault, it will not panic. As the
>> arm64 memory model spec said, when the address is not a multiple of the
>> element size, the access is unaligned. Unaligned accesses are allowed to
>> addresses marked as Normal, but not to Device regions. An unaligned access
>> to a Device region will trigger an exception (alignment fault).
>> 	
>> do_alignment_fault
>>      do_bad_area
>> 	__do_kernel_fault
>>             fixup_exception
>>
>> But that fixup cann't handle the unaligned copy, so the
>> copy_page_from_iter_atomic returns 0 and traps in loop.
> 
> Looks like you need to fix your raw_copy_from_user(), then...
> .
> 

Exit loop when iov_iter_copy_from_user_atomic() returns 0.
This should solve the problem, too, and it's easier.

Thanks.
Xiaoming Ni





	

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Al Viro]

On Wed, Jun 23, 2021 at 11:24:54AM +0800, Xiaoming Ni wrote:
> On 2021/6/23 10:50, Al Viro wrote:
> > On Wed, Jun 23, 2021 at 10:39:31AM +0800, Chen Huang wrote:
> > 
> > > Then when kernel handles the alignment_fault, it will not panic. As the
> > > arm64 memory model spec said, when the address is not a multiple of the
> > > element size, the access is unaligned. Unaligned accesses are allowed to
> > > addresses marked as Normal, but not to Device regions. An unaligned access
> > > to a Device region will trigger an exception (alignment fault).
> > > 	
> > > do_alignment_fault
> > >      do_bad_area
> > > 	__do_kernel_fault
> > >             fixup_exception
> > > 
> > > But that fixup cann't handle the unaligned copy, so the
> > > copy_page_from_iter_atomic returns 0 and traps in loop.
> > 
> > Looks like you need to fix your raw_copy_from_user(), then...
> > .
> > 
> 
> Exit loop when iov_iter_copy_from_user_atomic() returns 0.
> This should solve the problem, too, and it's easier.

It might be easier, but it's not going to work correctly.
If the page gets evicted by memory pressure, you are going
to get spurious short write.

Besides, it's simply wrong - write(2) does *NOT* require an
aligned source.  It (and raw_copy_from_user()) should act the
same way memcpy(3) does.

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Al Viro]

On Wed, Jun 23, 2021 at 11:24:54AM +0800, Xiaoming Ni wrote:
> On 2021/6/23 10:50, Al Viro wrote:
> > On Wed, Jun 23, 2021 at 10:39:31AM +0800, Chen Huang wrote:
> > 
> > > Then when kernel handles the alignment_fault, it will not panic. As the
> > > arm64 memory model spec said, when the address is not a multiple of the
> > > element size, the access is unaligned. Unaligned accesses are allowed to
> > > addresses marked as Normal, but not to Device regions. An unaligned access
> > > to a Device region will trigger an exception (alignment fault).
> > > 	
> > > do_alignment_fault
> > >      do_bad_area
> > > 	__do_kernel_fault
> > >             fixup_exception
> > > 
> > > But that fixup cann't handle the unaligned copy, so the
> > > copy_page_from_iter_atomic returns 0 and traps in loop.
> > 
> > Looks like you need to fix your raw_copy_from_user(), then...
> > .
> > 
> 
> Exit loop when iov_iter_copy_from_user_atomic() returns 0.
> This should solve the problem, too, and it's easier.

It might be easier, but it's not going to work correctly.
If the page gets evicted by memory pressure, you are going
to get spurious short write.

Besides, it's simply wrong - write(2) does *NOT* require an
aligned source.  It (and raw_copy_from_user()) should act the
same way memcpy(3) does.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Catalin Marinas]

On Wed, Jun 23, 2021 at 04:27:37AM +0000, Al Viro wrote:
> On Wed, Jun 23, 2021 at 11:24:54AM +0800, Xiaoming Ni wrote:
> > On 2021/6/23 10:50, Al Viro wrote:
> > > On Wed, Jun 23, 2021 at 10:39:31AM +0800, Chen Huang wrote:
> > > 
> > > > Then when kernel handles the alignment_fault, it will not panic. As the
> > > > arm64 memory model spec said, when the address is not a multiple of the
> > > > element size, the access is unaligned. Unaligned accesses are allowed to
> > > > addresses marked as Normal, but not to Device regions. An unaligned access
> > > > to a Device region will trigger an exception (alignment fault).
> > > > 	
> > > > do_alignment_fault
> > > >      do_bad_area
> > > > 	__do_kernel_fault
> > > >             fixup_exception
> > > > 
> > > > But that fixup cann't handle the unaligned copy, so the
> > > > copy_page_from_iter_atomic returns 0 and traps in loop.
> > > 
> > > Looks like you need to fix your raw_copy_from_user(), then...
> > 
> > Exit loop when iov_iter_copy_from_user_atomic() returns 0.
> > This should solve the problem, too, and it's easier.
> 
> It might be easier, but it's not going to work correctly.
> If the page gets evicted by memory pressure, you are going
> to get spurious short write.
> 
> Besides, it's simply wrong - write(2) does *NOT* require an
> aligned source.  It (and raw_copy_from_user()) should act the
> same way memcpy(3) does.

On arm64, neither memcpy() nor raw_copy_from_user() are expected to work
on Device mappings, we have memcpy_fromio() for this but only for
ioremap(). There's no (easy) way to distinguish in the write() syscall
how the source buffer is mapped. generic_perform_write() does an
iov_iter_fault_in_readable() check but that's not sufficient and it also
breaks the cases where you can get intra-page faults (arm64 MTE or SPARC
ADI). I think in the general case it's racy anyway (another thread doing
an mprotect(PROT_NONE) after the readable check passed).

So I think generic_perform_write() returning -EFAULT if copied == 0
would make sense (well, unless it breaks other cases I'm not aware of).

-- 
Catalin

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Catalin Marinas]

On Wed, Jun 23, 2021 at 04:27:37AM +0000, Al Viro wrote:
> On Wed, Jun 23, 2021 at 11:24:54AM +0800, Xiaoming Ni wrote:
> > On 2021/6/23 10:50, Al Viro wrote:
> > > On Wed, Jun 23, 2021 at 10:39:31AM +0800, Chen Huang wrote:
> > > 
> > > > Then when kernel handles the alignment_fault, it will not panic. As the
> > > > arm64 memory model spec said, when the address is not a multiple of the
> > > > element size, the access is unaligned. Unaligned accesses are allowed to
> > > > addresses marked as Normal, but not to Device regions. An unaligned access
> > > > to a Device region will trigger an exception (alignment fault).
> > > > 	
> > > > do_alignment_fault
> > > >      do_bad_area
> > > > 	__do_kernel_fault
> > > >             fixup_exception
> > > > 
> > > > But that fixup cann't handle the unaligned copy, so the
> > > > copy_page_from_iter_atomic returns 0 and traps in loop.
> > > 
> > > Looks like you need to fix your raw_copy_from_user(), then...
> > 
> > Exit loop when iov_iter_copy_from_user_atomic() returns 0.
> > This should solve the problem, too, and it's easier.
> 
> It might be easier, but it's not going to work correctly.
> If the page gets evicted by memory pressure, you are going
> to get spurious short write.
> 
> Besides, it's simply wrong - write(2) does *NOT* require an
> aligned source.  It (and raw_copy_from_user()) should act the
> same way memcpy(3) does.

On arm64, neither memcpy() nor raw_copy_from_user() are expected to work
on Device mappings, we have memcpy_fromio() for this but only for
ioremap(). There's no (easy) way to distinguish in the write() syscall
how the source buffer is mapped. generic_perform_write() does an
iov_iter_fault_in_readable() check but that's not sufficient and it also
breaks the cases where you can get intra-page faults (arm64 MTE or SPARC
ADI). I think in the general case it's racy anyway (another thread doing
an mprotect(PROT_NONE) after the readable check passed).

So I think generic_perform_write() returning -EFAULT if copied == 0
would make sense (well, unless it breaks other cases I'm not aware of).

-- 
Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Matthew Wilcox]

On Wed, Jun 23, 2021 at 10:32:21AM +0100, Catalin Marinas wrote:
> On Wed, Jun 23, 2021 at 04:27:37AM +0000, Al Viro wrote:
> > On Wed, Jun 23, 2021 at 11:24:54AM +0800, Xiaoming Ni wrote:
> > > On 2021/6/23 10:50, Al Viro wrote:
> > > > On Wed, Jun 23, 2021 at 10:39:31AM +0800, Chen Huang wrote:
> > > > 
> > > > > Then when kernel handles the alignment_fault, it will not panic. As the
> > > > > arm64 memory model spec said, when the address is not a multiple of the
> > > > > element size, the access is unaligned. Unaligned accesses are allowed to
> > > > > addresses marked as Normal, but not to Device regions. An unaligned access
> > > > > to a Device region will trigger an exception (alignment fault).
> > > > > 	
> > > > > do_alignment_fault
> > > > >      do_bad_area
> > > > > 	__do_kernel_fault
> > > > >             fixup_exception
> > > > > 
> > > > > But that fixup cann't handle the unaligned copy, so the
> > > > > copy_page_from_iter_atomic returns 0 and traps in loop.
> > > > 
> > > > Looks like you need to fix your raw_copy_from_user(), then...
> > > 
> > > Exit loop when iov_iter_copy_from_user_atomic() returns 0.
> > > This should solve the problem, too, and it's easier.
> > 
> > It might be easier, but it's not going to work correctly.
> > If the page gets evicted by memory pressure, you are going
> > to get spurious short write.
> > 
> > Besides, it's simply wrong - write(2) does *NOT* require an
> > aligned source.  It (and raw_copy_from_user()) should act the
> > same way memcpy(3) does.
> 
> On arm64, neither memcpy() nor raw_copy_from_user() are expected to work
> on Device mappings, we have memcpy_fromio() for this but only for
> ioremap(). There's no (easy) way to distinguish in the write() syscall
> how the source buffer is mapped. generic_perform_write() does an
> iov_iter_fault_in_readable() check but that's not sufficient and it also
> breaks the cases where you can get intra-page faults (arm64 MTE or SPARC
> ADI). I think in the general case it's racy anyway (another thread doing
> an mprotect(PROT_NONE) after the readable check passed).
> 
> So I think generic_perform_write() returning -EFAULT if copied == 0
> would make sense (well, unless it breaks other cases I'm not aware of).

It does break other cases -- that's what happens if the page has gone
missing after being faulted in.  You need to fix your copy_from_user().

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Matthew Wilcox]

On Wed, Jun 23, 2021 at 10:32:21AM +0100, Catalin Marinas wrote:
> On Wed, Jun 23, 2021 at 04:27:37AM +0000, Al Viro wrote:
> > On Wed, Jun 23, 2021 at 11:24:54AM +0800, Xiaoming Ni wrote:
> > > On 2021/6/23 10:50, Al Viro wrote:
> > > > On Wed, Jun 23, 2021 at 10:39:31AM +0800, Chen Huang wrote:
> > > > 
> > > > > Then when kernel handles the alignment_fault, it will not panic. As the
> > > > > arm64 memory model spec said, when the address is not a multiple of the
> > > > > element size, the access is unaligned. Unaligned accesses are allowed to
> > > > > addresses marked as Normal, but not to Device regions. An unaligned access
> > > > > to a Device region will trigger an exception (alignment fault).
> > > > > 	
> > > > > do_alignment_fault
> > > > >      do_bad_area
> > > > > 	__do_kernel_fault
> > > > >             fixup_exception
> > > > > 
> > > > > But that fixup cann't handle the unaligned copy, so the
> > > > > copy_page_from_iter_atomic returns 0 and traps in loop.
> > > > 
> > > > Looks like you need to fix your raw_copy_from_user(), then...
> > > 
> > > Exit loop when iov_iter_copy_from_user_atomic() returns 0.
> > > This should solve the problem, too, and it's easier.
> > 
> > It might be easier, but it's not going to work correctly.
> > If the page gets evicted by memory pressure, you are going
> > to get spurious short write.
> > 
> > Besides, it's simply wrong - write(2) does *NOT* require an
> > aligned source.  It (and raw_copy_from_user()) should act the
> > same way memcpy(3) does.
> 
> On arm64, neither memcpy() nor raw_copy_from_user() are expected to work
> on Device mappings, we have memcpy_fromio() for this but only for
> ioremap(). There's no (easy) way to distinguish in the write() syscall
> how the source buffer is mapped. generic_perform_write() does an
> iov_iter_fault_in_readable() check but that's not sufficient and it also
> breaks the cases where you can get intra-page faults (arm64 MTE or SPARC
> ADI). I think in the general case it's racy anyway (another thread doing
> an mprotect(PROT_NONE) after the readable check passed).
> 
> So I think generic_perform_write() returning -EFAULT if copied == 0
> would make sense (well, unless it breaks other cases I'm not aware of).

It does break other cases -- that's what happens if the page has gone
missing after being faulted in.  You need to fix your copy_from_user().

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Al Viro]

On Wed, Jun 23, 2021 at 10:32:21AM +0100, Catalin Marinas wrote:

> On arm64, neither memcpy() nor raw_copy_from_user() are expected to work
> on Device mappings, we have memcpy_fromio() for this but only for
> ioremap(). There's no (easy) way to distinguish in the write() syscall
> how the source buffer is mapped. generic_perform_write() does an
> iov_iter_fault_in_readable() check but that's not sufficient and it also
> breaks the cases where you can get intra-page faults (arm64 MTE or SPARC
> ADI). I think in the general case it's racy anyway (another thread doing
> an mprotect(PROT_NONE) after the readable check passed).
> 
> So I think generic_perform_write() returning -EFAULT if copied == 0
> would make sense (well, unless it breaks other cases I'm not aware of).

It does break the cases of source page eviction under memory pressure.
That aside, why the hell is that memory allowed to be mmaped?

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Al Viro]

On Wed, Jun 23, 2021 at 10:32:21AM +0100, Catalin Marinas wrote:

> On arm64, neither memcpy() nor raw_copy_from_user() are expected to work
> on Device mappings, we have memcpy_fromio() for this but only for
> ioremap(). There's no (easy) way to distinguish in the write() syscall
> how the source buffer is mapped. generic_perform_write() does an
> iov_iter_fault_in_readable() check but that's not sufficient and it also
> breaks the cases where you can get intra-page faults (arm64 MTE or SPARC
> ADI). I think in the general case it's racy anyway (another thread doing
> an mprotect(PROT_NONE) after the readable check passed).
> 
> So I think generic_perform_write() returning -EFAULT if copied == 0
> would make sense (well, unless it breaks other cases I'm not aware of).

It does break the cases of source page eviction under memory pressure.
That aside, why the hell is that memory allowed to be mmaped?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Mark Rutland]

On Wed, Jun 23, 2021 at 10:39:31AM +0800, Chen Huang wrote:
> When we access a device memory in userspace, then perform an unaligned write to a file.
> For example, we register a uio device and mmap the device, then perform an write to a
> file, like that:
> 
> 	device_addr = mmap(device_fd);
> 	write(file_fd, device_addr + unaligned_num, size);

What exactly is this device, and why do you want the kernel to do a
direct memcpy from MMIO? Why can't you copy that in userspace (where you
have knowledge of the device), then pass the copy to a syscall?

Ignoring the lockup below, this isn't going to work in general, since
uaccess routines do not guarantee alignment, single-copy, access sizes,
monotonically increasing addresses, etc. Any one of those can cause a
device to raise an external abort which may or may not be synchronous.

It does not make sense to tell the kernel to access this, since the
kernel cannot know how to access it safely, and we can;t do that without
knowledge of the device that we do not have.

Thanks,
Mark.

> 	
> We found that the infinite loop happened in generic_perform_write function:
> 
> again:
> 	copied = copy_page_from_iter_atomic(); //copied = 0
> 	status = ops->write_end(); //status = 0
> 	if (status == 0)
> 		goto again;
> 
> In copy_page_from_iter_atomic, the copyin() function finally call
> __arch_copy_from_user which create an exception table entry for 'insn'.
> Then when kernel handles the alignment_fault, it will not panic. As the
> arm64 memory model spec said, when the address is not a multiple of the
> element size, the access is unaligned. Unaligned accesses are allowed to
> addresses marked as Normal, but not to Device regions. An unaligned access
> to a Device region will trigger an exception (alignment fault).
> 	
> do_alignment_fault
>     do_bad_area
> 	__do_kernel_fault
>            fixup_exception
> 
> But that fixup cann't handle the unaligned copy, so the
> copy_page_from_iter_atomic returns 0 and traps in loop.
> 
> Reported-by: Chen Huang <chenhuang5@huawei.com>

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Mark Rutland]

On Wed, Jun 23, 2021 at 10:39:31AM +0800, Chen Huang wrote:
> When we access a device memory in userspace, then perform an unaligned write to a file.
> For example, we register a uio device and mmap the device, then perform an write to a
> file, like that:
> 
> 	device_addr = mmap(device_fd);
> 	write(file_fd, device_addr + unaligned_num, size);

What exactly is this device, and why do you want the kernel to do a
direct memcpy from MMIO? Why can't you copy that in userspace (where you
have knowledge of the device), then pass the copy to a syscall?

Ignoring the lockup below, this isn't going to work in general, since
uaccess routines do not guarantee alignment, single-copy, access sizes,
monotonically increasing addresses, etc. Any one of those can cause a
device to raise an external abort which may or may not be synchronous.

It does not make sense to tell the kernel to access this, since the
kernel cannot know how to access it safely, and we can;t do that without
knowledge of the device that we do not have.

Thanks,
Mark.

> 	
> We found that the infinite loop happened in generic_perform_write function:
> 
> again:
> 	copied = copy_page_from_iter_atomic(); //copied = 0
> 	status = ops->write_end(); //status = 0
> 	if (status == 0)
> 		goto again;
> 
> In copy_page_from_iter_atomic, the copyin() function finally call
> __arch_copy_from_user which create an exception table entry for 'insn'.
> Then when kernel handles the alignment_fault, it will not panic. As the
> arm64 memory model spec said, when the address is not a multiple of the
> element size, the access is unaligned. Unaligned accesses are allowed to
> addresses marked as Normal, but not to Device regions. An unaligned access
> to a Device region will trigger an exception (alignment fault).
> 	
> do_alignment_fault
>     do_bad_area
> 	__do_kernel_fault
>            fixup_exception
> 
> But that fixup cann't handle the unaligned copy, so the
> copy_page_from_iter_atomic returns 0 and traps in loop.
> 
> Reported-by: Chen Huang <chenhuang5@huawei.com>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Chen Huang]

在 2021/6/23 21:22, Mark Rutland 写道:
> On Wed, Jun 23, 2021 at 10:39:31AM +0800, Chen Huang wrote:
>> When we access a device memory in userspace, then perform an unaligned write to a file.
>> For example, we register a uio device and mmap the device, then perform an write to a
>> file, like that:
>>
>> 	device_addr = mmap(device_fd);
>> 	write(file_fd, device_addr + unaligned_num, size);
> 
> What exactly is this device, and why do you want the kernel to do a
> direct memcpy from MMIO? Why can't you copy that in userspace (where you
> have knowledge of the device), then pass the copy to a syscall?
>
I'm sorry for not describing the problem well. It's an uio device:

static struct device_driver uio_dummy_driver = {
    .name = "uio_with_name",
    .bus = &platform_bus_type,
    .probe = drv_uio_with_name_probe,
    .remove = drv_uio_with_name_remove,
};

static int drv_uio_with_name_probe(struct device *dev)
{
    uio_with_name_info.mem[0].addr = 0xa0000000;
    uio_with_name_info.mem[0].memtype = UIO_MEM_PHYS;
    uio_with_name_info.mem[0].size = 0x1000;

    if (__uio_register_device(THIS_MODULE, dev, &uio_with_name_info)) {
        printk("__uio_register_device failed\n");
        return -ENODEV;
    }
    printk("UIO init end.\n");
    return 0;
}

In userspace, I perform such operation:

 	fd = open("/tmp/test", O_RDWR | O_SYNC);
        access_address = (char *)mmap(NULL, uio_size, PROT_READ, MAP_SHARED, uio_fd, 0);
        ret = write(fd, access_address + 2, sizeof(long));

> Ignoring the lockup below, this isn't going to work in general, since
> uaccess routines do not guarantee alignment, single-copy, access sizes,
> monotonically increasing addresses, etc. Any one of those can cause a
> device to raise an external abort which may or may not be synchronous.
> 
> It does not make sense to tell the kernel to access this, since the
> kernel cannot know how to access it safely, and we can;t do that without
> knowledge of the device that we do not have.
> 
> Thanks,
> Mark.
> 
>> 	
>> We found that the infinite loop happened in generic_perform_write function:
>>
>> again:
>> 	copied = copy_page_from_iter_atomic(); //copied = 0
>> 	status = ops->write_end(); //status = 0
>> 	if (status == 0)
>> 		goto again;
>>
>> In copy_page_from_iter_atomic, the copyin() function finally call
>> __arch_copy_from_user which create an exception table entry for 'insn'.
>> Then when kernel handles the alignment_fault, it will not panic. As the
>> arm64 memory model spec said, when the address is not a multiple of the
>> element size, the access is unaligned. Unaligned accesses are allowed to
>> addresses marked as Normal, but not to Device regions. An unaligned access
>> to a Device region will trigger an exception (alignment fault).
>> 	
>> do_alignment_fault
>>     do_bad_area
>> 	__do_kernel_fault
>>            fixup_exception
>>
>> But that fixup cann't handle the unaligned copy, so the
>> copy_page_from_iter_atomic returns 0 and traps in loop.
>>
>> Reported-by: Chen Huang <chenhuang5@huawei.com>
> .
> 

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Chen Huang]

在 2021/6/23 21:22, Mark Rutland 写道:
> On Wed, Jun 23, 2021 at 10:39:31AM +0800, Chen Huang wrote:
>> When we access a device memory in userspace, then perform an unaligned write to a file.
>> For example, we register a uio device and mmap the device, then perform an write to a
>> file, like that:
>>
>> 	device_addr = mmap(device_fd);
>> 	write(file_fd, device_addr + unaligned_num, size);
> 
> What exactly is this device, and why do you want the kernel to do a
> direct memcpy from MMIO? Why can't you copy that in userspace (where you
> have knowledge of the device), then pass the copy to a syscall?
>
I'm sorry for not describing the problem well. It's an uio device:

static struct device_driver uio_dummy_driver = {
    .name = "uio_with_name",
    .bus = &platform_bus_type,
    .probe = drv_uio_with_name_probe,
    .remove = drv_uio_with_name_remove,
};

static int drv_uio_with_name_probe(struct device *dev)
{
    uio_with_name_info.mem[0].addr = 0xa0000000;
    uio_with_name_info.mem[0].memtype = UIO_MEM_PHYS;
    uio_with_name_info.mem[0].size = 0x1000;

    if (__uio_register_device(THIS_MODULE, dev, &uio_with_name_info)) {
        printk("__uio_register_device failed\n");
        return -ENODEV;
    }
    printk("UIO init end.\n");
    return 0;
}

In userspace, I perform such operation:

 	fd = open("/tmp/test", O_RDWR | O_SYNC);
        access_address = (char *)mmap(NULL, uio_size, PROT_READ, MAP_SHARED, uio_fd, 0);
        ret = write(fd, access_address + 2, sizeof(long));

> Ignoring the lockup below, this isn't going to work in general, since
> uaccess routines do not guarantee alignment, single-copy, access sizes,
> monotonically increasing addresses, etc. Any one of those can cause a
> device to raise an external abort which may or may not be synchronous.
> 
> It does not make sense to tell the kernel to access this, since the
> kernel cannot know how to access it safely, and we can;t do that without
> knowledge of the device that we do not have.
> 
> Thanks,
> Mark.
> 
>> 	
>> We found that the infinite loop happened in generic_perform_write function:
>>
>> again:
>> 	copied = copy_page_from_iter_atomic(); //copied = 0
>> 	status = ops->write_end(); //status = 0
>> 	if (status == 0)
>> 		goto again;
>>
>> In copy_page_from_iter_atomic, the copyin() function finally call
>> __arch_copy_from_user which create an exception table entry for 'insn'.
>> Then when kernel handles the alignment_fault, it will not panic. As the
>> arm64 memory model spec said, when the address is not a multiple of the
>> element size, the access is unaligned. Unaligned accesses are allowed to
>> addresses marked as Normal, but not to Device regions. An unaligned access
>> to a Device region will trigger an exception (alignment fault).
>> 	
>> do_alignment_fault
>>     do_bad_area
>> 	__do_kernel_fault
>>            fixup_exception
>>
>> But that fixup cann't handle the unaligned copy, so the
>> copy_page_from_iter_atomic returns 0 and traps in loop.
>>
>> Reported-by: Chen Huang <chenhuang5@huawei.com>
> .
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Matthew Wilcox]

On Thu, Jun 24, 2021 at 11:10:41AM +0800, Chen Huang wrote:
> In userspace, I perform such operation:
> 
>  	fd = open("/tmp/test", O_RDWR | O_SYNC);
>         access_address = (char *)mmap(NULL, uio_size, PROT_READ, MAP_SHARED, uio_fd, 0);
>         ret = write(fd, access_address + 2, sizeof(long));

... you know that accessing this at unaligned offsets isn't going to
work.  It's completely meaningless.  Why are you trying to do it?

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Matthew Wilcox]

On Thu, Jun 24, 2021 at 11:10:41AM +0800, Chen Huang wrote:
> In userspace, I perform such operation:
> 
>  	fd = open("/tmp/test", O_RDWR | O_SYNC);
>         access_address = (char *)mmap(NULL, uio_size, PROT_READ, MAP_SHARED, uio_fd, 0);
>         ret = write(fd, access_address + 2, sizeof(long));

... you know that accessing this at unaligned offsets isn't going to
work.  It's completely meaningless.  Why are you trying to do it?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Chen Huang]

在 2021/6/24 11:24, Matthew Wilcox 写道:
> On Thu, Jun 24, 2021 at 11:10:41AM +0800, Chen Huang wrote:
>> In userspace, I perform such operation:
>>
>>  	fd = open("/tmp/test", O_RDWR | O_SYNC);
>>         access_address = (char *)mmap(NULL, uio_size, PROT_READ, MAP_SHARED, uio_fd, 0);
>>         ret = write(fd, access_address + 2, sizeof(long));
> 
> ... you know that accessing this at unaligned offsets isn't going to
> work.  It's completely meaningless.  Why are you trying to do it?
> .
> 

Yeah, it's a wrong usage of access. But maybe it's still a problem
an userspace operation  makes the kernel crash.

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Chen Huang]

在 2021/6/24 11:24, Matthew Wilcox 写道:
> On Thu, Jun 24, 2021 at 11:10:41AM +0800, Chen Huang wrote:
>> In userspace, I perform such operation:
>>
>>  	fd = open("/tmp/test", O_RDWR | O_SYNC);
>>         access_address = (char *)mmap(NULL, uio_size, PROT_READ, MAP_SHARED, uio_fd, 0);
>>         ret = write(fd, access_address + 2, sizeof(long));
> 
> ... you know that accessing this at unaligned offsets isn't going to
> work.  It's completely meaningless.  Why are you trying to do it?
> .
> 

Yeah, it's a wrong usage of access. But maybe it's still a problem
an userspace operation  makes the kernel crash.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Christoph Hellwig]

On Thu, Jun 24, 2021 at 04:24:46AM +0100, Matthew Wilcox wrote:
> On Thu, Jun 24, 2021 at 11:10:41AM +0800, Chen Huang wrote:
> > In userspace, I perform such operation:
> > 
> >  	fd = open("/tmp/test", O_RDWR | O_SYNC);
> >         access_address = (char *)mmap(NULL, uio_size, PROT_READ, MAP_SHARED, uio_fd, 0);
> >         ret = write(fd, access_address + 2, sizeof(long));
> 
> ... you know that accessing this at unaligned offsets isn't going to
> work.  It's completely meaningless.  Why are you trying to do it?

We still should not cause an infinite loop in kernel space due to a
a userspace programmer error.

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Christoph Hellwig]

On Thu, Jun 24, 2021 at 04:24:46AM +0100, Matthew Wilcox wrote:
> On Thu, Jun 24, 2021 at 11:10:41AM +0800, Chen Huang wrote:
> > In userspace, I perform such operation:
> > 
> >  	fd = open("/tmp/test", O_RDWR | O_SYNC);
> >         access_address = (char *)mmap(NULL, uio_size, PROT_READ, MAP_SHARED, uio_fd, 0);
> >         ret = write(fd, access_address + 2, sizeof(long));
> 
> ... you know that accessing this at unaligned offsets isn't going to
> work.  It's completely meaningless.  Why are you trying to do it?

We still should not cause an infinite loop in kernel space due to a
a userspace programmer error.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Matthew Wilcox]

On Thu, Jun 24, 2021 at 08:04:07AM +0100, Christoph Hellwig wrote:
> On Thu, Jun 24, 2021 at 04:24:46AM +0100, Matthew Wilcox wrote:
> > On Thu, Jun 24, 2021 at 11:10:41AM +0800, Chen Huang wrote:
> > > In userspace, I perform such operation:
> > > 
> > >  	fd = open("/tmp/test", O_RDWR | O_SYNC);
> > >         access_address = (char *)mmap(NULL, uio_size, PROT_READ, MAP_SHARED, uio_fd, 0);
> > >         ret = write(fd, access_address + 2, sizeof(long));
> > 
> > ... you know that accessing this at unaligned offsets isn't going to
> > work.  It's completely meaningless.  Why are you trying to do it?
> 
> We still should not cause an infinite loop in kernel space due to a
> a userspace programmer error.

They're running as root and they've mapped some device memory.  We can't
save them from themself.  Imagine if they'd done this to the NVMe BAR.

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Matthew Wilcox]

On Thu, Jun 24, 2021 at 08:04:07AM +0100, Christoph Hellwig wrote:
> On Thu, Jun 24, 2021 at 04:24:46AM +0100, Matthew Wilcox wrote:
> > On Thu, Jun 24, 2021 at 11:10:41AM +0800, Chen Huang wrote:
> > > In userspace, I perform such operation:
> > > 
> > >  	fd = open("/tmp/test", O_RDWR | O_SYNC);
> > >         access_address = (char *)mmap(NULL, uio_size, PROT_READ, MAP_SHARED, uio_fd, 0);
> > >         ret = write(fd, access_address + 2, sizeof(long));
> > 
> > ... you know that accessing this at unaligned offsets isn't going to
> > work.  It's completely meaningless.  Why are you trying to do it?
> 
> We still should not cause an infinite loop in kernel space due to a
> a userspace programmer error.

They're running as root and they've mapped some device memory.  We can't
save them from themself.  Imagine if they'd done this to the NVMe BAR.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Robin Murphy]

On 2021-06-24 12:15, Matthew Wilcox wrote:
> On Thu, Jun 24, 2021 at 08:04:07AM +0100, Christoph Hellwig wrote:
>> On Thu, Jun 24, 2021 at 04:24:46AM +0100, Matthew Wilcox wrote:
>>> On Thu, Jun 24, 2021 at 11:10:41AM +0800, Chen Huang wrote:
>>>> In userspace, I perform such operation:
>>>>
>>>>   	fd = open("/tmp/test", O_RDWR | O_SYNC);
>>>>          access_address = (char *)mmap(NULL, uio_size, PROT_READ, MAP_SHARED, uio_fd, 0);
>>>>          ret = write(fd, access_address + 2, sizeof(long));
>>>
>>> ... you know that accessing this at unaligned offsets isn't going to
>>> work.  It's completely meaningless.  Why are you trying to do it?
>>
>> We still should not cause an infinite loop in kernel space due to a
>> a userspace programmer error.
> 
> They're running as root and they've mapped some device memory.  We can't
> save them from themself.  Imagine if they'd done this to the NVMe BAR.

FWIW I think the only way to make the kernel behaviour any more robust 
here would be to make the whole uaccess API more expressive, such that 
rather than simply saying "I only got this far" it could actually 
differentiate between stopping due to a fault which may be recoverable 
and worth retrying, and one which definitely isn't.

Unless maybe there's the possibility to abandon a syscall and SIGBUS the 
process directly from the uaccess fixup path, but even to my limited 
knowledge that seems unlikely.

(I'm not counting "cap the number of retries to a very large value to 
guarantee *eventual* failure" as robust, but I suppose it's a potential 
option too)

Robin.

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Robin Murphy]

On 2021-06-24 12:15, Matthew Wilcox wrote:
> On Thu, Jun 24, 2021 at 08:04:07AM +0100, Christoph Hellwig wrote:
>> On Thu, Jun 24, 2021 at 04:24:46AM +0100, Matthew Wilcox wrote:
>>> On Thu, Jun 24, 2021 at 11:10:41AM +0800, Chen Huang wrote:
>>>> In userspace, I perform such operation:
>>>>
>>>>   	fd = open("/tmp/test", O_RDWR | O_SYNC);
>>>>          access_address = (char *)mmap(NULL, uio_size, PROT_READ, MAP_SHARED, uio_fd, 0);
>>>>          ret = write(fd, access_address + 2, sizeof(long));
>>>
>>> ... you know that accessing this at unaligned offsets isn't going to
>>> work.  It's completely meaningless.  Why are you trying to do it?
>>
>> We still should not cause an infinite loop in kernel space due to a
>> a userspace programmer error.
> 
> They're running as root and they've mapped some device memory.  We can't
> save them from themself.  Imagine if they'd done this to the NVMe BAR.

FWIW I think the only way to make the kernel behaviour any more robust 
here would be to make the whole uaccess API more expressive, such that 
rather than simply saying "I only got this far" it could actually 
differentiate between stopping due to a fault which may be recoverable 
and worth retrying, and one which definitely isn't.

Unless maybe there's the possibility to abandon a syscall and SIGBUS the 
process directly from the uaccess fixup path, but even to my limited 
knowledge that seems unlikely.

(I'm not counting "cap the number of retries to a very large value to 
guarantee *eventual* failure" as robust, but I suppose it's a potential 
option too)

Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Catalin Marinas]

On Thu, Jun 24, 2021 at 12:15:46PM +0100, Matthew Wilcox wrote:
> On Thu, Jun 24, 2021 at 08:04:07AM +0100, Christoph Hellwig wrote:
> > On Thu, Jun 24, 2021 at 04:24:46AM +0100, Matthew Wilcox wrote:
> > > On Thu, Jun 24, 2021 at 11:10:41AM +0800, Chen Huang wrote:
> > > > In userspace, I perform such operation:
> > > > 
> > > >  	fd = open("/tmp/test", O_RDWR | O_SYNC);
> > > >         access_address = (char *)mmap(NULL, uio_size, PROT_READ, MAP_SHARED, uio_fd, 0);
> > > >         ret = write(fd, access_address + 2, sizeof(long));
> > > 
> > > ... you know that accessing this at unaligned offsets isn't going to
> > > work.  It's completely meaningless.  Why are you trying to do it?
> > 
> > We still should not cause an infinite loop in kernel space due to a
> > a userspace programmer error.
> 
> They're running as root and they've mapped some device memory.  We can't
> save them from themself.  Imagine if they'd done this to the NVMe BAR.

Ignoring the MMIO case for now, I can trigger the same infinite loop
with MTE (memory tagging), something like:

	char *a;

	a = mmap(0, page_sz, PROT_READ | PROT_WRITE | PROT_MTE,
		 MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
	/* tag 0 is the default, set tag 1 for the next 16 bytes */
	set_tag((unsigned long)(a + 16) | (1UL << 56));

	/* uaccess to a[16] expected to fail */
	bytes = write(fd, a + 14, 8);

The iov_iter_fault_in_readable() check succeeds since a[14] has tag 0.
However, the copy_from_user() attempts an unaligned 8-byte load which
fails because of the mismatched tag from a[16]. The loop continues
indefinitely.

copy_from_user() is not required to squeeze in as much as possible. So I
think the 1-byte read per page via iov_iter_fault_in_readable() is not
sufficient to guarantee progress unless copy_from_user() also reads at
least 1 byte.

We could change raw_copy_from_user() to fall back to 1-byte read in case
of a fault or fix this corner case in the generic code. A quick hack,
re-attempting the access with one byte:

------------------8<-------------------------
diff --git a/mm/filemap.c b/mm/filemap.c
index 66f7e9fdfbc4..67059071460c 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -3686,8 +3686,18 @@ ssize_t generic_perform_write(struct file *file,
 			 * because not all segments in the iov can be copied at
 			 * once without a pagefault.
 			 */
-			bytes = min_t(unsigned long, PAGE_SIZE - offset,
-						iov_iter_single_seg_count(i));
+			unsigned long single_seg_bytes =
+				min_t(unsigned long, PAGE_SIZE - offset,
+				      iov_iter_single_seg_count(i));
+
+			/*
+			 * Check for intra-page faults (arm64 MTE, SPARC ADI)
+			 * and fall back to single byte.
+			 */
+			if (bytes > single_seg_bytes)
+				bytes = single_seg_bytes;
+			else
+				bytes = 1;
 			goto again;
 		}
 		pos += copied;
------------------8<-------------------------

Or a slightly different hack, trying to detect if the first segment was
crossing a page boundary:

------------------8<-------------------------
diff --git a/mm/filemap.c b/mm/filemap.c
index 66f7e9fdfbc4..7d1c03f5f559 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -3678,16 +3678,24 @@ ssize_t generic_perform_write(struct file *file,
 
 		iov_iter_advance(i, copied);
 		if (unlikely(copied == 0)) {
+			struct iovec v = iov_iter_iovec(i);
+
 			/*
 			 * If we were unable to copy any data at all, we must
-			 * fall back to a single segment length write.
+			 * fall back to a single segment length write or a
+			 * single byte write (for intra-page faults - arm64
+			 * MTE or SPARC ADI).
 			 *
 			 * If we didn't fallback here, we could livelock
-			 * because not all segments in the iov can be copied at
-			 * once without a pagefault.
+			 * because not all segments in the iov or data within
+			 * a segment can be copied at once without a fault.
 			 */
-			bytes = min_t(unsigned long, PAGE_SIZE - offset,
-						iov_iter_single_seg_count(i));
+			if (((unsigned long)v.iov_base & PAGE_MASK) ==
+			    ((unsigned long)(v.iov_base + bytes) & PAGE_MASK))
+				bytes = 1;
+			else
+				bytes = min_t(unsigned long, PAGE_SIZE - offset,
+					      iov_iter_single_seg_count(i));
 			goto again;
 		}
 		pos += copied;
------------------8<-------------------------

-- 
Catalin

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Catalin Marinas]

On Thu, Jun 24, 2021 at 12:15:46PM +0100, Matthew Wilcox wrote:
> On Thu, Jun 24, 2021 at 08:04:07AM +0100, Christoph Hellwig wrote:
> > On Thu, Jun 24, 2021 at 04:24:46AM +0100, Matthew Wilcox wrote:
> > > On Thu, Jun 24, 2021 at 11:10:41AM +0800, Chen Huang wrote:
> > > > In userspace, I perform such operation:
> > > > 
> > > >  	fd = open("/tmp/test", O_RDWR | O_SYNC);
> > > >         access_address = (char *)mmap(NULL, uio_size, PROT_READ, MAP_SHARED, uio_fd, 0);
> > > >         ret = write(fd, access_address + 2, sizeof(long));
> > > 
> > > ... you know that accessing this at unaligned offsets isn't going to
> > > work.  It's completely meaningless.  Why are you trying to do it?
> > 
> > We still should not cause an infinite loop in kernel space due to a
> > a userspace programmer error.
> 
> They're running as root and they've mapped some device memory.  We can't
> save them from themself.  Imagine if they'd done this to the NVMe BAR.

Ignoring the MMIO case for now, I can trigger the same infinite loop
with MTE (memory tagging), something like:

	char *a;

	a = mmap(0, page_sz, PROT_READ | PROT_WRITE | PROT_MTE,
		 MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
	/* tag 0 is the default, set tag 1 for the next 16 bytes */
	set_tag((unsigned long)(a + 16) | (1UL << 56));

	/* uaccess to a[16] expected to fail */
	bytes = write(fd, a + 14, 8);

The iov_iter_fault_in_readable() check succeeds since a[14] has tag 0.
However, the copy_from_user() attempts an unaligned 8-byte load which
fails because of the mismatched tag from a[16]. The loop continues
indefinitely.

copy_from_user() is not required to squeeze in as much as possible. So I
think the 1-byte read per page via iov_iter_fault_in_readable() is not
sufficient to guarantee progress unless copy_from_user() also reads at
least 1 byte.

We could change raw_copy_from_user() to fall back to 1-byte read in case
of a fault or fix this corner case in the generic code. A quick hack,
re-attempting the access with one byte:

------------------8<-------------------------
diff --git a/mm/filemap.c b/mm/filemap.c
index 66f7e9fdfbc4..67059071460c 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -3686,8 +3686,18 @@ ssize_t generic_perform_write(struct file *file,
 			 * because not all segments in the iov can be copied at
 			 * once without a pagefault.
 			 */
-			bytes = min_t(unsigned long, PAGE_SIZE - offset,
-						iov_iter_single_seg_count(i));
+			unsigned long single_seg_bytes =
+				min_t(unsigned long, PAGE_SIZE - offset,
+				      iov_iter_single_seg_count(i));
+
+			/*
+			 * Check for intra-page faults (arm64 MTE, SPARC ADI)
+			 * and fall back to single byte.
+			 */
+			if (bytes > single_seg_bytes)
+				bytes = single_seg_bytes;
+			else
+				bytes = 1;
 			goto again;
 		}
 		pos += copied;
------------------8<-------------------------

Or a slightly different hack, trying to detect if the first segment was
crossing a page boundary:

------------------8<-------------------------
diff --git a/mm/filemap.c b/mm/filemap.c
index 66f7e9fdfbc4..7d1c03f5f559 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -3678,16 +3678,24 @@ ssize_t generic_perform_write(struct file *file,
 
 		iov_iter_advance(i, copied);
 		if (unlikely(copied == 0)) {
+			struct iovec v = iov_iter_iovec(i);
+
 			/*
 			 * If we were unable to copy any data at all, we must
-			 * fall back to a single segment length write.
+			 * fall back to a single segment length write or a
+			 * single byte write (for intra-page faults - arm64
+			 * MTE or SPARC ADI).
 			 *
 			 * If we didn't fallback here, we could livelock
-			 * because not all segments in the iov can be copied at
-			 * once without a pagefault.
+			 * because not all segments in the iov or data within
+			 * a segment can be copied at once without a fault.
 			 */
-			bytes = min_t(unsigned long, PAGE_SIZE - offset,
-						iov_iter_single_seg_count(i));
+			if (((unsigned long)v.iov_base & PAGE_MASK) ==
+			    ((unsigned long)(v.iov_base + bytes) & PAGE_MASK))
+				bytes = 1;
+			else
+				bytes = min_t(unsigned long, PAGE_SIZE - offset,
+					      iov_iter_single_seg_count(i));
 			goto again;
 		}
 		pos += copied;
------------------8<-------------------------

-- 
Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Al Viro]

On Thu, Jun 24, 2021 at 04:09:11PM +0100, Catalin Marinas wrote:
> On Thu, Jun 24, 2021 at 12:15:46PM +0100, Matthew Wilcox wrote:
> > On Thu, Jun 24, 2021 at 08:04:07AM +0100, Christoph Hellwig wrote:
> > > On Thu, Jun 24, 2021 at 04:24:46AM +0100, Matthew Wilcox wrote:
> > > > On Thu, Jun 24, 2021 at 11:10:41AM +0800, Chen Huang wrote:
> > > > > In userspace, I perform such operation:
> > > > > 
> > > > >  	fd = open("/tmp/test", O_RDWR | O_SYNC);
> > > > >         access_address = (char *)mmap(NULL, uio_size, PROT_READ, MAP_SHARED, uio_fd, 0);
> > > > >         ret = write(fd, access_address + 2, sizeof(long));
> > > > 
> > > > ... you know that accessing this at unaligned offsets isn't going to
> > > > work.  It's completely meaningless.  Why are you trying to do it?
> > > 
> > > We still should not cause an infinite loop in kernel space due to a
> > > a userspace programmer error.
> > 
> > They're running as root and they've mapped some device memory.  We can't
> > save them from themself.  Imagine if they'd done this to the NVMe BAR.

> We could change raw_copy_from_user() to fall back to 1-byte read in case
> of a fault or fix this corner case in the generic code. A quick hack,
> re-attempting the access with one byte:

No.  If nothing else, iov_iter_single_seg_count() is a bad kludge.
What's more, this "do a single-byte copy" fallback is punishing
a much more common case (memory pressure evicting the page) for the sake
of a corner case specific to one architecture that should've been dealt
with in its raw_copy_from_user().

NAKed-by: Al Viro <viro@zeniv.linux.org.uk>

For some context, see include/linux/uaccess.h and description of requirements
for raw_copy_from_user() in there.

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Al Viro]

On Thu, Jun 24, 2021 at 04:09:11PM +0100, Catalin Marinas wrote:
> On Thu, Jun 24, 2021 at 12:15:46PM +0100, Matthew Wilcox wrote:
> > On Thu, Jun 24, 2021 at 08:04:07AM +0100, Christoph Hellwig wrote:
> > > On Thu, Jun 24, 2021 at 04:24:46AM +0100, Matthew Wilcox wrote:
> > > > On Thu, Jun 24, 2021 at 11:10:41AM +0800, Chen Huang wrote:
> > > > > In userspace, I perform such operation:
> > > > > 
> > > > >  	fd = open("/tmp/test", O_RDWR | O_SYNC);
> > > > >         access_address = (char *)mmap(NULL, uio_size, PROT_READ, MAP_SHARED, uio_fd, 0);
> > > > >         ret = write(fd, access_address + 2, sizeof(long));
> > > > 
> > > > ... you know that accessing this at unaligned offsets isn't going to
> > > > work.  It's completely meaningless.  Why are you trying to do it?
> > > 
> > > We still should not cause an infinite loop in kernel space due to a
> > > a userspace programmer error.
> > 
> > They're running as root and they've mapped some device memory.  We can't
> > save them from themself.  Imagine if they'd done this to the NVMe BAR.

> We could change raw_copy_from_user() to fall back to 1-byte read in case
> of a fault or fix this corner case in the generic code. A quick hack,
> re-attempting the access with one byte:

No.  If nothing else, iov_iter_single_seg_count() is a bad kludge.
What's more, this "do a single-byte copy" fallback is punishing
a much more common case (memory pressure evicting the page) for the sake
of a corner case specific to one architecture that should've been dealt
with in its raw_copy_from_user().

NAKed-by: Al Viro <viro@zeniv.linux.org.uk>

For some context, see include/linux/uaccess.h and description of requirements
for raw_copy_from_user() in there.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Al Viro]

On Thu, Jun 24, 2021 at 02:22:27PM +0100, Robin Murphy wrote:

> FWIW I think the only way to make the kernel behaviour any more robust here
> would be to make the whole uaccess API more expressive, such that rather
> than simply saying "I only got this far" it could actually differentiate
> between stopping due to a fault which may be recoverable and worth retrying,
> and one which definitely isn't.

... and propagate that "more expressive" information through what, 3 or 4
levels in the call chain?  

From include/linux/uaccess.h:

 * If raw_copy_{to,from}_user(to, from, size) returns N, size - N bytes starting
 * at to must become equal to the bytes fetched from the corresponding area
 * starting at from.  All data past to + size - N must be left unmodified.
 *
 * If copying succeeds, the return value must be 0.  If some data cannot be
 * fetched, it is permitted to copy less than had been fetched; the only
 * hard requirement is that not storing anything at all (i.e. returning size)
 * should happen only when nothing could be copied.  In other words, you don't
 * have to squeeze as much as possible - it is allowed, but not necessary.

arm64 instances violate the aforementioned hard requirement.  Please, fix
it there; it's not hard.  All you need is an exception handler in .Ltiny15
that would fall back to (short) byte-by-byte copy if the faulting address
happened to be unaligned.  Or just do one-byte copy, not that it had been
considerably cheaper than a loop.  Will be cheaper than propagating that extra
information up the call chain, let alone paying for extra ->write_begin()
and ->write_end() for single byte in generic_perform_write().

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Al Viro]

On Thu, Jun 24, 2021 at 02:22:27PM +0100, Robin Murphy wrote:

> FWIW I think the only way to make the kernel behaviour any more robust here
> would be to make the whole uaccess API more expressive, such that rather
> than simply saying "I only got this far" it could actually differentiate
> between stopping due to a fault which may be recoverable and worth retrying,
> and one which definitely isn't.

... and propagate that "more expressive" information through what, 3 or 4
levels in the call chain?  

From include/linux/uaccess.h:

 * If raw_copy_{to,from}_user(to, from, size) returns N, size - N bytes starting
 * at to must become equal to the bytes fetched from the corresponding area
 * starting at from.  All data past to + size - N must be left unmodified.
 *
 * If copying succeeds, the return value must be 0.  If some data cannot be
 * fetched, it is permitted to copy less than had been fetched; the only
 * hard requirement is that not storing anything at all (i.e. returning size)
 * should happen only when nothing could be copied.  In other words, you don't
 * have to squeeze as much as possible - it is allowed, but not necessary.

arm64 instances violate the aforementioned hard requirement.  Please, fix
it there; it's not hard.  All you need is an exception handler in .Ltiny15
that would fall back to (short) byte-by-byte copy if the faulting address
happened to be unaligned.  Or just do one-byte copy, not that it had been
considerably cheaper than a loop.  Will be cheaper than propagating that extra
information up the call chain, let alone paying for extra ->write_begin()
and ->write_end() for single byte in generic_perform_write().

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Robin Murphy]

On 2021-06-24 17:27, Al Viro wrote:
> On Thu, Jun 24, 2021 at 02:22:27PM +0100, Robin Murphy wrote:
> 
>> FWIW I think the only way to make the kernel behaviour any more robust here
>> would be to make the whole uaccess API more expressive, such that rather
>> than simply saying "I only got this far" it could actually differentiate
>> between stopping due to a fault which may be recoverable and worth retrying,
>> and one which definitely isn't.
> 
> ... and propagate that "more expressive" information through what, 3 or 4
> levels in the call chain?
> 
>  From include/linux/uaccess.h:
> 
>   * If raw_copy_{to,from}_user(to, from, size) returns N, size - N bytes starting
>   * at to must become equal to the bytes fetched from the corresponding area
>   * starting at from.  All data past to + size - N must be left unmodified.
>   *
>   * If copying succeeds, the return value must be 0.  If some data cannot be
>   * fetched, it is permitted to copy less than had been fetched; the only
>   * hard requirement is that not storing anything at all (i.e. returning size)
>   * should happen only when nothing could be copied.  In other words, you don't
>   * have to squeeze as much as possible - it is allowed, but not necessary.
> 
> arm64 instances violate the aforementioned hard requirement.  Please, fix
> it there; it's not hard.  All you need is an exception handler in .Ltiny15
> that would fall back to (short) byte-by-byte copy if the faulting address
> happened to be unaligned.  Or just do one-byte copy, not that it had been
> considerably cheaper than a loop.  Will be cheaper than propagating that extra
> information up the call chain, let alone paying for extra ->write_begin()
> and ->write_end() for single byte in generic_perform_write().

And what do we do if we then continue to fault with an external abort 
because whatever it is that warranted being mapped as Device-type memory 
in the first place doesn't support byte accesses?

Robin.

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Robin Murphy]

On 2021-06-24 17:27, Al Viro wrote:
> On Thu, Jun 24, 2021 at 02:22:27PM +0100, Robin Murphy wrote:
> 
>> FWIW I think the only way to make the kernel behaviour any more robust here
>> would be to make the whole uaccess API more expressive, such that rather
>> than simply saying "I only got this far" it could actually differentiate
>> between stopping due to a fault which may be recoverable and worth retrying,
>> and one which definitely isn't.
> 
> ... and propagate that "more expressive" information through what, 3 or 4
> levels in the call chain?
> 
>  From include/linux/uaccess.h:
> 
>   * If raw_copy_{to,from}_user(to, from, size) returns N, size - N bytes starting
>   * at to must become equal to the bytes fetched from the corresponding area
>   * starting at from.  All data past to + size - N must be left unmodified.
>   *
>   * If copying succeeds, the return value must be 0.  If some data cannot be
>   * fetched, it is permitted to copy less than had been fetched; the only
>   * hard requirement is that not storing anything at all (i.e. returning size)
>   * should happen only when nothing could be copied.  In other words, you don't
>   * have to squeeze as much as possible - it is allowed, but not necessary.
> 
> arm64 instances violate the aforementioned hard requirement.  Please, fix
> it there; it's not hard.  All you need is an exception handler in .Ltiny15
> that would fall back to (short) byte-by-byte copy if the faulting address
> happened to be unaligned.  Or just do one-byte copy, not that it had been
> considerably cheaper than a loop.  Will be cheaper than propagating that extra
> information up the call chain, let alone paying for extra ->write_begin()
> and ->write_end() for single byte in generic_perform_write().

And what do we do if we then continue to fault with an external abort 
because whatever it is that warranted being mapped as Device-type memory 
in the first place doesn't support byte accesses?

Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Al Viro]

On Thu, Jun 24, 2021 at 05:38:35PM +0100, Robin Murphy wrote:
> On 2021-06-24 17:27, Al Viro wrote:
> > On Thu, Jun 24, 2021 at 02:22:27PM +0100, Robin Murphy wrote:
> > 
> > > FWIW I think the only way to make the kernel behaviour any more robust here
> > > would be to make the whole uaccess API more expressive, such that rather
> > > than simply saying "I only got this far" it could actually differentiate
> > > between stopping due to a fault which may be recoverable and worth retrying,
> > > and one which definitely isn't.
> > 
> > ... and propagate that "more expressive" information through what, 3 or 4
> > levels in the call chain?
> > 
> >  From include/linux/uaccess.h:
> > 
> >   * If raw_copy_{to,from}_user(to, from, size) returns N, size - N bytes starting
> >   * at to must become equal to the bytes fetched from the corresponding area
> >   * starting at from.  All data past to + size - N must be left unmodified.
> >   *
> >   * If copying succeeds, the return value must be 0.  If some data cannot be
> >   * fetched, it is permitted to copy less than had been fetched; the only
> >   * hard requirement is that not storing anything at all (i.e. returning size)
> >   * should happen only when nothing could be copied.  In other words, you don't
> >   * have to squeeze as much as possible - it is allowed, but not necessary.
> > 
> > arm64 instances violate the aforementioned hard requirement.  Please, fix
> > it there; it's not hard.  All you need is an exception handler in .Ltiny15
> > that would fall back to (short) byte-by-byte copy if the faulting address
> > happened to be unaligned.  Or just do one-byte copy, not that it had been
> > considerably cheaper than a loop.  Will be cheaper than propagating that extra
> > information up the call chain, let alone paying for extra ->write_begin()
> > and ->write_end() for single byte in generic_perform_write().
> 
> And what do we do if we then continue to fault with an external abort
> because whatever it is that warranted being mapped as Device-type memory in
> the first place doesn't support byte accesses?

If it does not support byte access, it would've failed on fault-in.

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Al Viro]

On Thu, Jun 24, 2021 at 05:38:35PM +0100, Robin Murphy wrote:
> On 2021-06-24 17:27, Al Viro wrote:
> > On Thu, Jun 24, 2021 at 02:22:27PM +0100, Robin Murphy wrote:
> > 
> > > FWIW I think the only way to make the kernel behaviour any more robust here
> > > would be to make the whole uaccess API more expressive, such that rather
> > > than simply saying "I only got this far" it could actually differentiate
> > > between stopping due to a fault which may be recoverable and worth retrying,
> > > and one which definitely isn't.
> > 
> > ... and propagate that "more expressive" information through what, 3 or 4
> > levels in the call chain?
> > 
> >  From include/linux/uaccess.h:
> > 
> >   * If raw_copy_{to,from}_user(to, from, size) returns N, size - N bytes starting
> >   * at to must become equal to the bytes fetched from the corresponding area
> >   * starting at from.  All data past to + size - N must be left unmodified.
> >   *
> >   * If copying succeeds, the return value must be 0.  If some data cannot be
> >   * fetched, it is permitted to copy less than had been fetched; the only
> >   * hard requirement is that not storing anything at all (i.e. returning size)
> >   * should happen only when nothing could be copied.  In other words, you don't
> >   * have to squeeze as much as possible - it is allowed, but not necessary.
> > 
> > arm64 instances violate the aforementioned hard requirement.  Please, fix
> > it there; it's not hard.  All you need is an exception handler in .Ltiny15
> > that would fall back to (short) byte-by-byte copy if the faulting address
> > happened to be unaligned.  Or just do one-byte copy, not that it had been
> > considerably cheaper than a loop.  Will be cheaper than propagating that extra
> > information up the call chain, let alone paying for extra ->write_begin()
> > and ->write_end() for single byte in generic_perform_write().
> 
> And what do we do if we then continue to fault with an external abort
> because whatever it is that warranted being mapped as Device-type memory in
> the first place doesn't support byte accesses?

If it does not support byte access, it would've failed on fault-in.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Robin Murphy]

On 2021-06-24 17:39, Al Viro wrote:
> On Thu, Jun 24, 2021 at 05:38:35PM +0100, Robin Murphy wrote:
>> On 2021-06-24 17:27, Al Viro wrote:
>>> On Thu, Jun 24, 2021 at 02:22:27PM +0100, Robin Murphy wrote:
>>>
>>>> FWIW I think the only way to make the kernel behaviour any more robust here
>>>> would be to make the whole uaccess API more expressive, such that rather
>>>> than simply saying "I only got this far" it could actually differentiate
>>>> between stopping due to a fault which may be recoverable and worth retrying,
>>>> and one which definitely isn't.
>>>
>>> ... and propagate that "more expressive" information through what, 3 or 4
>>> levels in the call chain?
>>>
>>>   From include/linux/uaccess.h:
>>>
>>>    * If raw_copy_{to,from}_user(to, from, size) returns N, size - N bytes starting
>>>    * at to must become equal to the bytes fetched from the corresponding area
>>>    * starting at from.  All data past to + size - N must be left unmodified.
>>>    *
>>>    * If copying succeeds, the return value must be 0.  If some data cannot be
>>>    * fetched, it is permitted to copy less than had been fetched; the only
>>>    * hard requirement is that not storing anything at all (i.e. returning size)
>>>    * should happen only when nothing could be copied.  In other words, you don't
>>>    * have to squeeze as much as possible - it is allowed, but not necessary.
>>>
>>> arm64 instances violate the aforementioned hard requirement.  Please, fix
>>> it there; it's not hard.  All you need is an exception handler in .Ltiny15
>>> that would fall back to (short) byte-by-byte copy if the faulting address
>>> happened to be unaligned.  Or just do one-byte copy, not that it had been
>>> considerably cheaper than a loop.  Will be cheaper than propagating that extra
>>> information up the call chain, let alone paying for extra ->write_begin()
>>> and ->write_end() for single byte in generic_perform_write().
>>
>> And what do we do if we then continue to fault with an external abort
>> because whatever it is that warranted being mapped as Device-type memory in
>> the first place doesn't support byte accesses?
> 
> If it does not support byte access, it would've failed on fault-in.

OK, if I'm understanding the code correctly and fault-in touches the 
exact byte that copy_to_user() is going to start on, and faulting 
anywhere *after* that byte is still OK, then that seems mostly workable, 
although there are still potential corner cases like a device register 
accepting byte reads but not byte writes.

Basically if privileged userspace is going to do dumb things with 
mmap()ed MMIO, the kernel can't *guarantee* to save it from itself 
without a hell of a lot of invasive work for no other gain. Sure we can 
add some extra fallback paths in our arch code for a best-effort attempt 
to mitigate alignment faults - revamping the usercopy routines is on my 
to-do list so I'll bear this in mind, and I think it's basically the 
same idea we mooted some time ago for tag faults anyway - but I'm sure 
someone will inevitably still find some new way to trip it up. 
Fortunately on modern systems many of the aforementioned dumb things 
won't actually fault synchronously, so even if triggered by a usercopy 
accesses the payback will come slightly later via asynchronous SError 
and be considerably more terminal.

Thanks,
Robin.

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Robin Murphy]

On 2021-06-24 17:39, Al Viro wrote:
> On Thu, Jun 24, 2021 at 05:38:35PM +0100, Robin Murphy wrote:
>> On 2021-06-24 17:27, Al Viro wrote:
>>> On Thu, Jun 24, 2021 at 02:22:27PM +0100, Robin Murphy wrote:
>>>
>>>> FWIW I think the only way to make the kernel behaviour any more robust here
>>>> would be to make the whole uaccess API more expressive, such that rather
>>>> than simply saying "I only got this far" it could actually differentiate
>>>> between stopping due to a fault which may be recoverable and worth retrying,
>>>> and one which definitely isn't.
>>>
>>> ... and propagate that "more expressive" information through what, 3 or 4
>>> levels in the call chain?
>>>
>>>   From include/linux/uaccess.h:
>>>
>>>    * If raw_copy_{to,from}_user(to, from, size) returns N, size - N bytes starting
>>>    * at to must become equal to the bytes fetched from the corresponding area
>>>    * starting at from.  All data past to + size - N must be left unmodified.
>>>    *
>>>    * If copying succeeds, the return value must be 0.  If some data cannot be
>>>    * fetched, it is permitted to copy less than had been fetched; the only
>>>    * hard requirement is that not storing anything at all (i.e. returning size)
>>>    * should happen only when nothing could be copied.  In other words, you don't
>>>    * have to squeeze as much as possible - it is allowed, but not necessary.
>>>
>>> arm64 instances violate the aforementioned hard requirement.  Please, fix
>>> it there; it's not hard.  All you need is an exception handler in .Ltiny15
>>> that would fall back to (short) byte-by-byte copy if the faulting address
>>> happened to be unaligned.  Or just do one-byte copy, not that it had been
>>> considerably cheaper than a loop.  Will be cheaper than propagating that extra
>>> information up the call chain, let alone paying for extra ->write_begin()
>>> and ->write_end() for single byte in generic_perform_write().
>>
>> And what do we do if we then continue to fault with an external abort
>> because whatever it is that warranted being mapped as Device-type memory in
>> the first place doesn't support byte accesses?
> 
> If it does not support byte access, it would've failed on fault-in.

OK, if I'm understanding the code correctly and fault-in touches the 
exact byte that copy_to_user() is going to start on, and faulting 
anywhere *after* that byte is still OK, then that seems mostly workable, 
although there are still potential corner cases like a device register 
accepting byte reads but not byte writes.

Basically if privileged userspace is going to do dumb things with 
mmap()ed MMIO, the kernel can't *guarantee* to save it from itself 
without a hell of a lot of invasive work for no other gain. Sure we can 
add some extra fallback paths in our arch code for a best-effort attempt 
to mitigate alignment faults - revamping the usercopy routines is on my 
to-do list so I'll bear this in mind, and I think it's basically the 
same idea we mooted some time ago for tag faults anyway - but I'm sure 
someone will inevitably still find some new way to trip it up. 
Fortunately on modern systems many of the aforementioned dumb things 
won't actually fault synchronously, so even if triggered by a usercopy 
accesses the payback will come slightly later via asynchronous SError 
and be considerably more terminal.

Thanks,
Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Catalin Marinas]

On Thu, Jun 24, 2021 at 04:27:17PM +0000, Al Viro wrote:
> On Thu, Jun 24, 2021 at 02:22:27PM +0100, Robin Murphy wrote:
> > FWIW I think the only way to make the kernel behaviour any more robust here
> > would be to make the whole uaccess API more expressive, such that rather
> > than simply saying "I only got this far" it could actually differentiate
> > between stopping due to a fault which may be recoverable and worth retrying,
> > and one which definitely isn't.
> 
> ... and propagate that "more expressive" information through what, 3 or 4
> levels in the call chain?  
> 
> From include/linux/uaccess.h:
> 
>  * If raw_copy_{to,from}_user(to, from, size) returns N, size - N bytes starting
>  * at to must become equal to the bytes fetched from the corresponding area
>  * starting at from.  All data past to + size - N must be left unmodified.
>  *
>  * If copying succeeds, the return value must be 0.  If some data cannot be
>  * fetched, it is permitted to copy less than had been fetched; the only
>  * hard requirement is that not storing anything at all (i.e. returning size)
>  * should happen only when nothing could be copied.  In other words, you don't
>  * have to squeeze as much as possible - it is allowed, but not necessary.
> 
> arm64 instances violate the aforementioned hard requirement.

After reading the above a few more times, I think I get it. The key
sentence is: not storing anything at all should happen only when nothing
could be copied. In the MTE case, something can still be copied.

> Please, fix
> it there; it's not hard.  All you need is an exception handler in .Ltiny15
> that would fall back to (short) byte-by-byte copy if the faulting address
> happened to be unaligned.  Or just do one-byte copy, not that it had been
> considerably cheaper than a loop.  Will be cheaper than propagating that extra
> information up the call chain, let alone paying for extra ->write_begin()
> and ->write_end() for single byte in generic_perform_write().

Yeah, it's definitely fixable in the arch code. I misread the above
requirements and thought it could be fixed in the core code.

Quick hack, though I think in the actual exception handling path in .S
more sense (and it needs the copy_to_user for symmetry):

diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
index b5f08621fa29..903f8a2a457b 100644
--- a/arch/arm64/include/asm/uaccess.h
+++ b/arch/arm64/include/asm/uaccess.h
@@ -415,6 +415,15 @@ extern unsigned long __must_check __arch_copy_from_user(void *to, const void __u
 	uaccess_ttbr0_enable();						\
 	__acfu_ret = __arch_copy_from_user((to),			\
 				      __uaccess_mask_ptr(from), (n));	\
+	if (__acfu_ret == n) {						\
+		int __cfu_err = 0;					\
+		char __cfu_val;						\
+		__raw_get_mem("ldtr", __cfu_val, (char *)from, __cfu_err);\
+		if (!__cfu_err) {					\
+			*(char *)to = __cfu_val;			\
+			__acfu_ret--;					\
+		}							\
+	}								\
 	uaccess_ttbr0_disable();					\
 	__acfu_ret;							\
 })

Of course, it only fixes the MTE problem, I'll ignore the MMIO case
(though it may work in certain configurations like synchronous faults).

-- 
Catalin

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Catalin Marinas]

On Thu, Jun 24, 2021 at 04:27:17PM +0000, Al Viro wrote:
> On Thu, Jun 24, 2021 at 02:22:27PM +0100, Robin Murphy wrote:
> > FWIW I think the only way to make the kernel behaviour any more robust here
> > would be to make the whole uaccess API more expressive, such that rather
> > than simply saying "I only got this far" it could actually differentiate
> > between stopping due to a fault which may be recoverable and worth retrying,
> > and one which definitely isn't.
> 
> ... and propagate that "more expressive" information through what, 3 or 4
> levels in the call chain?  
> 
> From include/linux/uaccess.h:
> 
>  * If raw_copy_{to,from}_user(to, from, size) returns N, size - N bytes starting
>  * at to must become equal to the bytes fetched from the corresponding area
>  * starting at from.  All data past to + size - N must be left unmodified.
>  *
>  * If copying succeeds, the return value must be 0.  If some data cannot be
>  * fetched, it is permitted to copy less than had been fetched; the only
>  * hard requirement is that not storing anything at all (i.e. returning size)
>  * should happen only when nothing could be copied.  In other words, you don't
>  * have to squeeze as much as possible - it is allowed, but not necessary.
> 
> arm64 instances violate the aforementioned hard requirement.

After reading the above a few more times, I think I get it. The key
sentence is: not storing anything at all should happen only when nothing
could be copied. In the MTE case, something can still be copied.

> Please, fix
> it there; it's not hard.  All you need is an exception handler in .Ltiny15
> that would fall back to (short) byte-by-byte copy if the faulting address
> happened to be unaligned.  Or just do one-byte copy, not that it had been
> considerably cheaper than a loop.  Will be cheaper than propagating that extra
> information up the call chain, let alone paying for extra ->write_begin()
> and ->write_end() for single byte in generic_perform_write().

Yeah, it's definitely fixable in the arch code. I misread the above
requirements and thought it could be fixed in the core code.

Quick hack, though I think in the actual exception handling path in .S
more sense (and it needs the copy_to_user for symmetry):

diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
index b5f08621fa29..903f8a2a457b 100644
--- a/arch/arm64/include/asm/uaccess.h
+++ b/arch/arm64/include/asm/uaccess.h
@@ -415,6 +415,15 @@ extern unsigned long __must_check __arch_copy_from_user(void *to, const void __u
 	uaccess_ttbr0_enable();						\
 	__acfu_ret = __arch_copy_from_user((to),			\
 				      __uaccess_mask_ptr(from), (n));	\
+	if (__acfu_ret == n) {						\
+		int __cfu_err = 0;					\
+		char __cfu_val;						\
+		__raw_get_mem("ldtr", __cfu_val, (char *)from, __cfu_err);\
+		if (!__cfu_err) {					\
+			*(char *)to = __cfu_val;			\
+			__acfu_ret--;					\
+		}							\
+	}								\
 	uaccess_ttbr0_disable();					\
 	__acfu_ret;							\
 })

Of course, it only fixes the MTE problem, I'll ignore the MMIO case
(though it may work in certain configurations like synchronous faults).

-- 
Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Robin Murphy]

On 2021-06-24 19:55, Catalin Marinas wrote:
> On Thu, Jun 24, 2021 at 04:27:17PM +0000, Al Viro wrote:
>> On Thu, Jun 24, 2021 at 02:22:27PM +0100, Robin Murphy wrote:
>>> FWIW I think the only way to make the kernel behaviour any more robust here
>>> would be to make the whole uaccess API more expressive, such that rather
>>> than simply saying "I only got this far" it could actually differentiate
>>> between stopping due to a fault which may be recoverable and worth retrying,
>>> and one which definitely isn't.
>>
>> ... and propagate that "more expressive" information through what, 3 or 4
>> levels in the call chain?
>>
>>  From include/linux/uaccess.h:
>>
>>   * If raw_copy_{to,from}_user(to, from, size) returns N, size - N bytes starting
>>   * at to must become equal to the bytes fetched from the corresponding area
>>   * starting at from.  All data past to + size - N must be left unmodified.
>>   *
>>   * If copying succeeds, the return value must be 0.  If some data cannot be
>>   * fetched, it is permitted to copy less than had been fetched; the only
>>   * hard requirement is that not storing anything at all (i.e. returning size)
>>   * should happen only when nothing could be copied.  In other words, you don't
>>   * have to squeeze as much as possible - it is allowed, but not necessary.
>>
>> arm64 instances violate the aforementioned hard requirement.
> 
> After reading the above a few more times, I think I get it. The key
> sentence is: not storing anything at all should happen only when nothing
> could be copied. In the MTE case, something can still be copied.
> 
>> Please, fix
>> it there; it's not hard.  All you need is an exception handler in .Ltiny15
>> that would fall back to (short) byte-by-byte copy if the faulting address
>> happened to be unaligned.  Or just do one-byte copy, not that it had been
>> considerably cheaper than a loop.  Will be cheaper than propagating that extra
>> information up the call chain, let alone paying for extra ->write_begin()
>> and ->write_end() for single byte in generic_perform_write().
> 
> Yeah, it's definitely fixable in the arch code. I misread the above
> requirements and thought it could be fixed in the core code.
> 
> Quick hack, though I think in the actual exception handling path in .S
> more sense (and it needs the copy_to_user for symmetry):

Hmm, if anything the asm version might be even more straightforward; I 
think it's pretty much just this (untested):

diff --git a/arch/arm64/lib/copy_to_user.S b/arch/arm64/lib/copy_to_user.S
index 043da90f5dd7..632bf1f9540d 100644
--- a/arch/arm64/lib/copy_to_user.S
+++ b/arch/arm64/lib/copy_to_user.S
@@ -62,6 +62,9 @@ EXPORT_SYMBOL(__arch_copy_to_user)

         .section .fixup,"ax"
         .align  2
-9998:  sub     x0, end, dst                    // bytes not copied
+9998:  ldrb    w7, [x1]
+USER(9997f,    sttrb   w7, [x0])
+       add     x0, x0, #1
+9997:  sub     x0, end, dst                    // bytes not copied
         ret
         .previous

If we can get away without trying to finish the whole copy bytewise, 
(i.e. we don't cause any faults of our own by knowingly over-reading in 
the routine itself), I'm more than happy with that.

Robin.

> diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
> index b5f08621fa29..903f8a2a457b 100644
> --- a/arch/arm64/include/asm/uaccess.h
> +++ b/arch/arm64/include/asm/uaccess.h
> @@ -415,6 +415,15 @@ extern unsigned long __must_check __arch_copy_from_user(void *to, const void __u
>   	uaccess_ttbr0_enable();						\
>   	__acfu_ret = __arch_copy_from_user((to),			\
>   				      __uaccess_mask_ptr(from), (n));	\
> +	if (__acfu_ret == n) {						\
> +		int __cfu_err = 0;					\
> +		char __cfu_val;						\
> +		__raw_get_mem("ldtr", __cfu_val, (char *)from, __cfu_err);\
> +		if (!__cfu_err) {					\
> +			*(char *)to = __cfu_val;			\
> +			__acfu_ret--;					\
> +		}							\
> +	}								\
>   	uaccess_ttbr0_disable();					\
>   	__acfu_ret;							\
>   })
> 
> Of course, it only fixes the MTE problem, I'll ignore the MMIO case
> (though it may work in certain configurations like synchronous faults).
> 

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Robin Murphy]

On 2021-06-24 19:55, Catalin Marinas wrote:
> On Thu, Jun 24, 2021 at 04:27:17PM +0000, Al Viro wrote:
>> On Thu, Jun 24, 2021 at 02:22:27PM +0100, Robin Murphy wrote:
>>> FWIW I think the only way to make the kernel behaviour any more robust here
>>> would be to make the whole uaccess API more expressive, such that rather
>>> than simply saying "I only got this far" it could actually differentiate
>>> between stopping due to a fault which may be recoverable and worth retrying,
>>> and one which definitely isn't.
>>
>> ... and propagate that "more expressive" information through what, 3 or 4
>> levels in the call chain?
>>
>>  From include/linux/uaccess.h:
>>
>>   * If raw_copy_{to,from}_user(to, from, size) returns N, size - N bytes starting
>>   * at to must become equal to the bytes fetched from the corresponding area
>>   * starting at from.  All data past to + size - N must be left unmodified.
>>   *
>>   * If copying succeeds, the return value must be 0.  If some data cannot be
>>   * fetched, it is permitted to copy less than had been fetched; the only
>>   * hard requirement is that not storing anything at all (i.e. returning size)
>>   * should happen only when nothing could be copied.  In other words, you don't
>>   * have to squeeze as much as possible - it is allowed, but not necessary.
>>
>> arm64 instances violate the aforementioned hard requirement.
> 
> After reading the above a few more times, I think I get it. The key
> sentence is: not storing anything at all should happen only when nothing
> could be copied. In the MTE case, something can still be copied.
> 
>> Please, fix
>> it there; it's not hard.  All you need is an exception handler in .Ltiny15
>> that would fall back to (short) byte-by-byte copy if the faulting address
>> happened to be unaligned.  Or just do one-byte copy, not that it had been
>> considerably cheaper than a loop.  Will be cheaper than propagating that extra
>> information up the call chain, let alone paying for extra ->write_begin()
>> and ->write_end() for single byte in generic_perform_write().
> 
> Yeah, it's definitely fixable in the arch code. I misread the above
> requirements and thought it could be fixed in the core code.
> 
> Quick hack, though I think in the actual exception handling path in .S
> more sense (and it needs the copy_to_user for symmetry):

Hmm, if anything the asm version might be even more straightforward; I 
think it's pretty much just this (untested):

diff --git a/arch/arm64/lib/copy_to_user.S b/arch/arm64/lib/copy_to_user.S
index 043da90f5dd7..632bf1f9540d 100644
--- a/arch/arm64/lib/copy_to_user.S
+++ b/arch/arm64/lib/copy_to_user.S
@@ -62,6 +62,9 @@ EXPORT_SYMBOL(__arch_copy_to_user)

         .section .fixup,"ax"
         .align  2
-9998:  sub     x0, end, dst                    // bytes not copied
+9998:  ldrb    w7, [x1]
+USER(9997f,    sttrb   w7, [x0])
+       add     x0, x0, #1
+9997:  sub     x0, end, dst                    // bytes not copied
         ret
         .previous

If we can get away without trying to finish the whole copy bytewise, 
(i.e. we don't cause any faults of our own by knowingly over-reading in 
the routine itself), I'm more than happy with that.

Robin.

> diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
> index b5f08621fa29..903f8a2a457b 100644
> --- a/arch/arm64/include/asm/uaccess.h
> +++ b/arch/arm64/include/asm/uaccess.h
> @@ -415,6 +415,15 @@ extern unsigned long __must_check __arch_copy_from_user(void *to, const void __u
>   	uaccess_ttbr0_enable();						\
>   	__acfu_ret = __arch_copy_from_user((to),			\
>   				      __uaccess_mask_ptr(from), (n));	\
> +	if (__acfu_ret == n) {						\
> +		int __cfu_err = 0;					\
> +		char __cfu_val;						\
> +		__raw_get_mem("ldtr", __cfu_val, (char *)from, __cfu_err);\
> +		if (!__cfu_err) {					\
> +			*(char *)to = __cfu_val;			\
> +			__acfu_ret--;					\
> +		}							\
> +	}								\
>   	uaccess_ttbr0_disable();					\
>   	__acfu_ret;							\
>   })
> 
> Of course, it only fixes the MTE problem, I'll ignore the MMIO case
> (though it may work in certain configurations like synchronous faults).
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Catalin Marinas]

On Thu, Jun 24, 2021 at 09:36:54PM +0100, Robin Murphy wrote:
> On 2021-06-24 19:55, Catalin Marinas wrote:
> > On Thu, Jun 24, 2021 at 04:27:17PM +0000, Al Viro wrote:
> > > On Thu, Jun 24, 2021 at 02:22:27PM +0100, Robin Murphy wrote:
> > > > FWIW I think the only way to make the kernel behaviour any more robust here
> > > > would be to make the whole uaccess API more expressive, such that rather
> > > > than simply saying "I only got this far" it could actually differentiate
> > > > between stopping due to a fault which may be recoverable and worth retrying,
> > > > and one which definitely isn't.
> > > 
> > > ... and propagate that "more expressive" information through what, 3 or 4
> > > levels in the call chain?
> > > 
> > >  From include/linux/uaccess.h:
> > > 
> > >   * If raw_copy_{to,from}_user(to, from, size) returns N, size - N bytes starting
> > >   * at to must become equal to the bytes fetched from the corresponding area
> > >   * starting at from.  All data past to + size - N must be left unmodified.
> > >   *
> > >   * If copying succeeds, the return value must be 0.  If some data cannot be
> > >   * fetched, it is permitted to copy less than had been fetched; the only
> > >   * hard requirement is that not storing anything at all (i.e. returning size)
> > >   * should happen only when nothing could be copied.  In other words, you don't
> > >   * have to squeeze as much as possible - it is allowed, but not necessary.
> > > 
> > > arm64 instances violate the aforementioned hard requirement.
> > 
> > After reading the above a few more times, I think I get it. The key
> > sentence is: not storing anything at all should happen only when nothing
> > could be copied. In the MTE case, something can still be copied.
> > 
> > > Please, fix
> > > it there; it's not hard.  All you need is an exception handler in .Ltiny15
> > > that would fall back to (short) byte-by-byte copy if the faulting address
> > > happened to be unaligned.  Or just do one-byte copy, not that it had been
> > > considerably cheaper than a loop.  Will be cheaper than propagating that extra
> > > information up the call chain, let alone paying for extra ->write_begin()
> > > and ->write_end() for single byte in generic_perform_write().
> > 
> > Yeah, it's definitely fixable in the arch code. I misread the above
> > requirements and thought it could be fixed in the core code.
> > 
> > Quick hack, though I think in the actual exception handling path in .S
> > more sense (and it needs the copy_to_user for symmetry):
> 
> Hmm, if anything the asm version might be even more straightforward; I think
> it's pretty much just this (untested):

That's what I thought but it was too late in the day to think in asm.

> diff --git a/arch/arm64/lib/copy_to_user.S b/arch/arm64/lib/copy_to_user.S
> index 043da90f5dd7..632bf1f9540d 100644
> --- a/arch/arm64/lib/copy_to_user.S
> +++ b/arch/arm64/lib/copy_to_user.S
> @@ -62,6 +62,9 @@ EXPORT_SYMBOL(__arch_copy_to_user)
> 
>         .section .fixup,"ax"
>         .align  2
> -9998:  sub     x0, end, dst                    // bytes not copied
> +9998:  ldrb    w7, [x1]
> +USER(9997f,    sttrb   w7, [x0])
> +       add     x0, x0, #1
> +9997:  sub     x0, end, dst                    // bytes not copied
>         ret
>         .previous
> 
> If we can get away without trying to finish the whole copy bytewise, (i.e.
> we don't cause any faults of our own by knowingly over-reading in the
> routine itself), I'm more than happy with that.

I don't think we over-read/write in the routine itself as this is based
on the user memcpy() which can't handle faults. And since we got a fault
before the end of the copy, we have at least one byte left in the
buffer (which may or may not trigger a fault).

I wonder whether we should skip the extra byte copy if something was
copied, i.e. start the exception handler with:

	cmp	dstin, dst
	b.ne	9997f

That said, the fall-back to bytewise copying may have some advantage. I
think we still have the issue where we copy some data to user but report
less (STP failing on the second 8-byte when the first had been already
written first 8). A byte copy loop would solve this, unless we pass the
fault address to the exception handler (I thought you had some patch for
this at some point).

-- 
Catalin

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Catalin Marinas]

On Thu, Jun 24, 2021 at 09:36:54PM +0100, Robin Murphy wrote:
> On 2021-06-24 19:55, Catalin Marinas wrote:
> > On Thu, Jun 24, 2021 at 04:27:17PM +0000, Al Viro wrote:
> > > On Thu, Jun 24, 2021 at 02:22:27PM +0100, Robin Murphy wrote:
> > > > FWIW I think the only way to make the kernel behaviour any more robust here
> > > > would be to make the whole uaccess API more expressive, such that rather
> > > > than simply saying "I only got this far" it could actually differentiate
> > > > between stopping due to a fault which may be recoverable and worth retrying,
> > > > and one which definitely isn't.
> > > 
> > > ... and propagate that "more expressive" information through what, 3 or 4
> > > levels in the call chain?
> > > 
> > >  From include/linux/uaccess.h:
> > > 
> > >   * If raw_copy_{to,from}_user(to, from, size) returns N, size - N bytes starting
> > >   * at to must become equal to the bytes fetched from the corresponding area
> > >   * starting at from.  All data past to + size - N must be left unmodified.
> > >   *
> > >   * If copying succeeds, the return value must be 0.  If some data cannot be
> > >   * fetched, it is permitted to copy less than had been fetched; the only
> > >   * hard requirement is that not storing anything at all (i.e. returning size)
> > >   * should happen only when nothing could be copied.  In other words, you don't
> > >   * have to squeeze as much as possible - it is allowed, but not necessary.
> > > 
> > > arm64 instances violate the aforementioned hard requirement.
> > 
> > After reading the above a few more times, I think I get it. The key
> > sentence is: not storing anything at all should happen only when nothing
> > could be copied. In the MTE case, something can still be copied.
> > 
> > > Please, fix
> > > it there; it's not hard.  All you need is an exception handler in .Ltiny15
> > > that would fall back to (short) byte-by-byte copy if the faulting address
> > > happened to be unaligned.  Or just do one-byte copy, not that it had been
> > > considerably cheaper than a loop.  Will be cheaper than propagating that extra
> > > information up the call chain, let alone paying for extra ->write_begin()
> > > and ->write_end() for single byte in generic_perform_write().
> > 
> > Yeah, it's definitely fixable in the arch code. I misread the above
> > requirements and thought it could be fixed in the core code.
> > 
> > Quick hack, though I think in the actual exception handling path in .S
> > more sense (and it needs the copy_to_user for symmetry):
> 
> Hmm, if anything the asm version might be even more straightforward; I think
> it's pretty much just this (untested):

That's what I thought but it was too late in the day to think in asm.

> diff --git a/arch/arm64/lib/copy_to_user.S b/arch/arm64/lib/copy_to_user.S
> index 043da90f5dd7..632bf1f9540d 100644
> --- a/arch/arm64/lib/copy_to_user.S
> +++ b/arch/arm64/lib/copy_to_user.S
> @@ -62,6 +62,9 @@ EXPORT_SYMBOL(__arch_copy_to_user)
> 
>         .section .fixup,"ax"
>         .align  2
> -9998:  sub     x0, end, dst                    // bytes not copied
> +9998:  ldrb    w7, [x1]
> +USER(9997f,    sttrb   w7, [x0])
> +       add     x0, x0, #1
> +9997:  sub     x0, end, dst                    // bytes not copied
>         ret
>         .previous
> 
> If we can get away without trying to finish the whole copy bytewise, (i.e.
> we don't cause any faults of our own by knowingly over-reading in the
> routine itself), I'm more than happy with that.

I don't think we over-read/write in the routine itself as this is based
on the user memcpy() which can't handle faults. And since we got a fault
before the end of the copy, we have at least one byte left in the
buffer (which may or may not trigger a fault).

I wonder whether we should skip the extra byte copy if something was
copied, i.e. start the exception handler with:

	cmp	dstin, dst
	b.ne	9997f

That said, the fall-back to bytewise copying may have some advantage. I
think we still have the issue where we copy some data to user but report
less (STP failing on the second 8-byte when the first had been already
written first 8). A byte copy loop would solve this, unless we pass the
fault address to the exception handler (I thought you had some patch for
this at some point).

-- 
Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Robin Murphy]

On 2021-06-25 11:39, Catalin Marinas wrote:
> On Thu, Jun 24, 2021 at 09:36:54PM +0100, Robin Murphy wrote:
>> On 2021-06-24 19:55, Catalin Marinas wrote:
>>> On Thu, Jun 24, 2021 at 04:27:17PM +0000, Al Viro wrote:
>>>> On Thu, Jun 24, 2021 at 02:22:27PM +0100, Robin Murphy wrote:
>>>>> FWIW I think the only way to make the kernel behaviour any more robust here
>>>>> would be to make the whole uaccess API more expressive, such that rather
>>>>> than simply saying "I only got this far" it could actually differentiate
>>>>> between stopping due to a fault which may be recoverable and worth retrying,
>>>>> and one which definitely isn't.
>>>>
>>>> ... and propagate that "more expressive" information through what, 3 or 4
>>>> levels in the call chain?
>>>>
>>>>   From include/linux/uaccess.h:
>>>>
>>>>    * If raw_copy_{to,from}_user(to, from, size) returns N, size - N bytes starting
>>>>    * at to must become equal to the bytes fetched from the corresponding area
>>>>    * starting at from.  All data past to + size - N must be left unmodified.
>>>>    *
>>>>    * If copying succeeds, the return value must be 0.  If some data cannot be
>>>>    * fetched, it is permitted to copy less than had been fetched; the only
>>>>    * hard requirement is that not storing anything at all (i.e. returning size)
>>>>    * should happen only when nothing could be copied.  In other words, you don't
>>>>    * have to squeeze as much as possible - it is allowed, but not necessary.
>>>>
>>>> arm64 instances violate the aforementioned hard requirement.
>>>
>>> After reading the above a few more times, I think I get it. The key
>>> sentence is: not storing anything at all should happen only when nothing
>>> could be copied. In the MTE case, something can still be copied.
>>>
>>>> Please, fix
>>>> it there; it's not hard.  All you need is an exception handler in .Ltiny15
>>>> that would fall back to (short) byte-by-byte copy if the faulting address
>>>> happened to be unaligned.  Or just do one-byte copy, not that it had been
>>>> considerably cheaper than a loop.  Will be cheaper than propagating that extra
>>>> information up the call chain, let alone paying for extra ->write_begin()
>>>> and ->write_end() for single byte in generic_perform_write().
>>>
>>> Yeah, it's definitely fixable in the arch code. I misread the above
>>> requirements and thought it could be fixed in the core code.
>>>
>>> Quick hack, though I think in the actual exception handling path in .S
>>> more sense (and it needs the copy_to_user for symmetry):
>>
>> Hmm, if anything the asm version might be even more straightforward; I think
>> it's pretty much just this (untested):
> 
> That's what I thought but it was too late in the day to think in asm.
> 
>> diff --git a/arch/arm64/lib/copy_to_user.S b/arch/arm64/lib/copy_to_user.S
>> index 043da90f5dd7..632bf1f9540d 100644
>> --- a/arch/arm64/lib/copy_to_user.S
>> +++ b/arch/arm64/lib/copy_to_user.S
>> @@ -62,6 +62,9 @@ EXPORT_SYMBOL(__arch_copy_to_user)
>>
>>          .section .fixup,"ax"
>>          .align  2
>> -9998:  sub     x0, end, dst                    // bytes not copied
>> +9998:  ldrb    w7, [x1]
>> +USER(9997f,    sttrb   w7, [x0])
>> +       add     x0, x0, #1
>> +9997:  sub     x0, end, dst                    // bytes not copied
>>          ret
>>          .previous
>>
>> If we can get away without trying to finish the whole copy bytewise, (i.e.
>> we don't cause any faults of our own by knowingly over-reading in the
>> routine itself), I'm more than happy with that.
> 
> I don't think we over-read/write in the routine itself as this is based
> on the user memcpy() which can't handle faults. And since we got a fault
> before the end of the copy, we have at least one byte left in the
> buffer (which may or may not trigger a fault).
> 
> I wonder whether we should skip the extra byte copy if something was
> copied, i.e. start the exception handler with:
> 
> 	cmp	dstin, dst
> 	b.ne	9997f
> 
> That said, the fall-back to bytewise copying may have some advantage. I
> think we still have the issue where we copy some data to user but report
> less (STP failing on the second 8-byte when the first had been already
> written first 8). A byte copy loop would solve this, unless we pass the
> fault address to the exception handler (I thought you had some patch for
> this at some point).

OK, this is the quick-fix patch I've ended up with - I'll send it
properly at rc1. It makes both copy_from_user and copy_to_user behave
reasonably in Catalin's MTE-based test-case, so I figure copy_in_user
must be OK too (I keep hoping it might have gone away by now...)

Robin.

----->8-----
From: Robin Murphy <robin.murphy@arm.com>
Subject: [PATCH] arm64: Avoid premature usercopy failure

Al reminds us that the usercopy API must only return complete failure
if absolutely nothing could be copied. Currently, if userspace does
something silly like giving us an unaligned pointer to Device memory,
or a size which overruns MTE tag bounds, we may fail to honour that
requirement when faulting on a multi-byte access even though a smaller
access could have succeeded.

Add a mitigation to the fixup routines to fall back to a single-byte
copy if we faulted on a larger access before anything has been written
to the destination, to guarantee making *some* forward progress. We
needn't be too concerned about the overall performance since this should
only occur when callers are doing something a bit dodgy in the first
place. Particularly broken userspace might still be able to trick
generic_perform_write() into an infinite loop by targeting write() at
an mmap() of some read-only device register where the fault-in load
succeeds but any store synchronously aborts such that copy_to_user() is
genuinely unable to make progress, but, well, don't do that...

Reported-by: Chen Huang <chenhuang5@huawei.com>
Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
  arch/arm64/lib/copy_from_user.S | 14 +++++++++++---
  arch/arm64/lib/copy_in_user.S   | 21 ++++++++++++++-------
  arch/arm64/lib/copy_to_user.S   | 14 +++++++++++---
  3 files changed, 36 insertions(+), 13 deletions(-)

diff --git a/arch/arm64/lib/copy_from_user.S b/arch/arm64/lib/copy_from_user.S
index 95cd62d67371..5b720a29a242 100644
--- a/arch/arm64/lib/copy_from_user.S
+++ b/arch/arm64/lib/copy_from_user.S
@@ -29,7 +29,7 @@
  	.endm
  
  	.macro ldrh1 reg, ptr, val
-	user_ldst 9998f, ldtrh, \reg, \ptr, \val
+	user_ldst 9997f, ldtrh, \reg, \ptr, \val
  	.endm
  
  	.macro strh1 reg, ptr, val
@@ -37,7 +37,7 @@
  	.endm
  
  	.macro ldr1 reg, ptr, val
-	user_ldst 9998f, ldtr, \reg, \ptr, \val
+	user_ldst 9997f, ldtr, \reg, \ptr, \val
  	.endm
  
  	.macro str1 reg, ptr, val
@@ -45,7 +45,7 @@
  	.endm
  
  	.macro ldp1 reg1, reg2, ptr, val
-	user_ldp 9998f, \reg1, \reg2, \ptr, \val
+	user_ldp 9997f, \reg1, \reg2, \ptr, \val
  	.endm
  
  	.macro stp1 reg1, reg2, ptr, val
@@ -53,8 +53,10 @@
  	.endm
  
  end	.req	x5
+srcin	.req	x15
  SYM_FUNC_START(__arch_copy_from_user)
  	add	end, x0, x2
+	mov	srcin, x1
  #include "copy_template.S"
  	mov	x0, #0				// Nothing to copy
  	ret
@@ -63,6 +65,12 @@ EXPORT_SYMBOL(__arch_copy_from_user)
  
  	.section .fixup,"ax"
  	.align	2
+9997:	cmp	dst, dstin
+	b.ne	9998f
+	// Before being absolutely sure we couldn't copy anything, try harder
+USER(9998f, ldtrb tmp1w, [srcin])
+	strb	tmp1w, [dstin]
+	add	dst, dstin, #1
  9998:	sub	x0, end, dst			// bytes not copied
  	ret
  	.previous
diff --git a/arch/arm64/lib/copy_in_user.S b/arch/arm64/lib/copy_in_user.S
index 1f61cd0df062..0abb24a01781 100644
--- a/arch/arm64/lib/copy_in_user.S
+++ b/arch/arm64/lib/copy_in_user.S
@@ -30,33 +30,34 @@
  	.endm
  
  	.macro ldrh1 reg, ptr, val
-	user_ldst 9998f, ldtrh, \reg, \ptr, \val
+	user_ldst 9997f, ldtrh, \reg, \ptr, \val
  	.endm
  
  	.macro strh1 reg, ptr, val
-	user_ldst 9998f, sttrh, \reg, \ptr, \val
+	user_ldst 9997f, sttrh, \reg, \ptr, \val
  	.endm
  
  	.macro ldr1 reg, ptr, val
-	user_ldst 9998f, ldtr, \reg, \ptr, \val
+	user_ldst 9997f, ldtr, \reg, \ptr, \val
  	.endm
  
  	.macro str1 reg, ptr, val
-	user_ldst 9998f, sttr, \reg, \ptr, \val
+	user_ldst 9997f, sttr, \reg, \ptr, \val
  	.endm
  
  	.macro ldp1 reg1, reg2, ptr, val
-	user_ldp 9998f, \reg1, \reg2, \ptr, \val
+	user_ldp 9997f, \reg1, \reg2, \ptr, \val
  	.endm
  
  	.macro stp1 reg1, reg2, ptr, val
-	user_stp 9998f, \reg1, \reg2, \ptr, \val
+	user_stp 9997f, \reg1, \reg2, \ptr, \val
  	.endm
  
  end	.req	x5
-
+srcin	.req	x15
  SYM_FUNC_START(__arch_copy_in_user)
  	add	end, x0, x2
+	mov	srcin, x1
  #include "copy_template.S"
  	mov	x0, #0
  	ret
@@ -65,6 +66,12 @@ EXPORT_SYMBOL(__arch_copy_in_user)
  
  	.section .fixup,"ax"
  	.align	2
+9997:	cmp	dst, dstin
+	b.ne	9998f
+	// Before being absolutely sure we couldn't copy anything, try harder
+USER(9998f, ldtrb tmp1w, [srcin])
+USER(9998f, sttrb tmp1w, [dstin])
+	add	dst, dstin, #1
  9998:	sub	x0, end, dst			// bytes not copied
  	ret
  	.previous
diff --git a/arch/arm64/lib/copy_to_user.S b/arch/arm64/lib/copy_to_user.S
index 043da90f5dd7..cfb598ae4812 100644
--- a/arch/arm64/lib/copy_to_user.S
+++ b/arch/arm64/lib/copy_to_user.S
@@ -32,7 +32,7 @@
  	.endm
  
  	.macro strh1 reg, ptr, val
-	user_ldst 9998f, sttrh, \reg, \ptr, \val
+	user_ldst 9997f, sttrh, \reg, \ptr, \val
  	.endm
  
  	.macro ldr1 reg, ptr, val
@@ -40,7 +40,7 @@
  	.endm
  
  	.macro str1 reg, ptr, val
-	user_ldst 9998f, sttr, \reg, \ptr, \val
+	user_ldst 9997f, sttr, \reg, \ptr, \val
  	.endm
  
  	.macro ldp1 reg1, reg2, ptr, val
@@ -48,12 +48,14 @@
  	.endm
  
  	.macro stp1 reg1, reg2, ptr, val
-	user_stp 9998f, \reg1, \reg2, \ptr, \val
+	user_stp 9997f, \reg1, \reg2, \ptr, \val
  	.endm
  
  end	.req	x5
+srcin	.req	x15
  SYM_FUNC_START(__arch_copy_to_user)
  	add	end, x0, x2
+	mov	srcin, x1
  #include "copy_template.S"
  	mov	x0, #0
  	ret
@@ -62,6 +64,12 @@ EXPORT_SYMBOL(__arch_copy_to_user)
  
  	.section .fixup,"ax"
  	.align	2
+9997:	cmp	dst, dstin
+	b.ne	9998f
+	// Before being absolutely sure we couldn't copy anything, try harder
+	ldrb	tmp1w, [srcin]
+USER(9998f, sttrb tmp1w, [dstin])
+	add	dst, dstin, #1
  9998:	sub	x0, end, dst			// bytes not copied
  	ret
  	.previous
-- 
2.25.1

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Robin Murphy]

On 2021-06-25 11:39, Catalin Marinas wrote:
> On Thu, Jun 24, 2021 at 09:36:54PM +0100, Robin Murphy wrote:
>> On 2021-06-24 19:55, Catalin Marinas wrote:
>>> On Thu, Jun 24, 2021 at 04:27:17PM +0000, Al Viro wrote:
>>>> On Thu, Jun 24, 2021 at 02:22:27PM +0100, Robin Murphy wrote:
>>>>> FWIW I think the only way to make the kernel behaviour any more robust here
>>>>> would be to make the whole uaccess API more expressive, such that rather
>>>>> than simply saying "I only got this far" it could actually differentiate
>>>>> between stopping due to a fault which may be recoverable and worth retrying,
>>>>> and one which definitely isn't.
>>>>
>>>> ... and propagate that "more expressive" information through what, 3 or 4
>>>> levels in the call chain?
>>>>
>>>>   From include/linux/uaccess.h:
>>>>
>>>>    * If raw_copy_{to,from}_user(to, from, size) returns N, size - N bytes starting
>>>>    * at to must become equal to the bytes fetched from the corresponding area
>>>>    * starting at from.  All data past to + size - N must be left unmodified.
>>>>    *
>>>>    * If copying succeeds, the return value must be 0.  If some data cannot be
>>>>    * fetched, it is permitted to copy less than had been fetched; the only
>>>>    * hard requirement is that not storing anything at all (i.e. returning size)
>>>>    * should happen only when nothing could be copied.  In other words, you don't
>>>>    * have to squeeze as much as possible - it is allowed, but not necessary.
>>>>
>>>> arm64 instances violate the aforementioned hard requirement.
>>>
>>> After reading the above a few more times, I think I get it. The key
>>> sentence is: not storing anything at all should happen only when nothing
>>> could be copied. In the MTE case, something can still be copied.
>>>
>>>> Please, fix
>>>> it there; it's not hard.  All you need is an exception handler in .Ltiny15
>>>> that would fall back to (short) byte-by-byte copy if the faulting address
>>>> happened to be unaligned.  Or just do one-byte copy, not that it had been
>>>> considerably cheaper than a loop.  Will be cheaper than propagating that extra
>>>> information up the call chain, let alone paying for extra ->write_begin()
>>>> and ->write_end() for single byte in generic_perform_write().
>>>
>>> Yeah, it's definitely fixable in the arch code. I misread the above
>>> requirements and thought it could be fixed in the core code.
>>>
>>> Quick hack, though I think in the actual exception handling path in .S
>>> more sense (and it needs the copy_to_user for symmetry):
>>
>> Hmm, if anything the asm version might be even more straightforward; I think
>> it's pretty much just this (untested):
> 
> That's what I thought but it was too late in the day to think in asm.
> 
>> diff --git a/arch/arm64/lib/copy_to_user.S b/arch/arm64/lib/copy_to_user.S
>> index 043da90f5dd7..632bf1f9540d 100644
>> --- a/arch/arm64/lib/copy_to_user.S
>> +++ b/arch/arm64/lib/copy_to_user.S
>> @@ -62,6 +62,9 @@ EXPORT_SYMBOL(__arch_copy_to_user)
>>
>>          .section .fixup,"ax"
>>          .align  2
>> -9998:  sub     x0, end, dst                    // bytes not copied
>> +9998:  ldrb    w7, [x1]
>> +USER(9997f,    sttrb   w7, [x0])
>> +       add     x0, x0, #1
>> +9997:  sub     x0, end, dst                    // bytes not copied
>>          ret
>>          .previous
>>
>> If we can get away without trying to finish the whole copy bytewise, (i.e.
>> we don't cause any faults of our own by knowingly over-reading in the
>> routine itself), I'm more than happy with that.
> 
> I don't think we over-read/write in the routine itself as this is based
> on the user memcpy() which can't handle faults. And since we got a fault
> before the end of the copy, we have at least one byte left in the
> buffer (which may or may not trigger a fault).
> 
> I wonder whether we should skip the extra byte copy if something was
> copied, i.e. start the exception handler with:
> 
> 	cmp	dstin, dst
> 	b.ne	9997f
> 
> That said, the fall-back to bytewise copying may have some advantage. I
> think we still have the issue where we copy some data to user but report
> less (STP failing on the second 8-byte when the first had been already
> written first 8). A byte copy loop would solve this, unless we pass the
> fault address to the exception handler (I thought you had some patch for
> this at some point).

OK, this is the quick-fix patch I've ended up with - I'll send it
properly at rc1. It makes both copy_from_user and copy_to_user behave
reasonably in Catalin's MTE-based test-case, so I figure copy_in_user
must be OK too (I keep hoping it might have gone away by now...)

Robin.

----->8-----
From: Robin Murphy <robin.murphy@arm.com>
Subject: [PATCH] arm64: Avoid premature usercopy failure

Al reminds us that the usercopy API must only return complete failure
if absolutely nothing could be copied. Currently, if userspace does
something silly like giving us an unaligned pointer to Device memory,
or a size which overruns MTE tag bounds, we may fail to honour that
requirement when faulting on a multi-byte access even though a smaller
access could have succeeded.

Add a mitigation to the fixup routines to fall back to a single-byte
copy if we faulted on a larger access before anything has been written
to the destination, to guarantee making *some* forward progress. We
needn't be too concerned about the overall performance since this should
only occur when callers are doing something a bit dodgy in the first
place. Particularly broken userspace might still be able to trick
generic_perform_write() into an infinite loop by targeting write() at
an mmap() of some read-only device register where the fault-in load
succeeds but any store synchronously aborts such that copy_to_user() is
genuinely unable to make progress, but, well, don't do that...

Reported-by: Chen Huang <chenhuang5@huawei.com>
Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
  arch/arm64/lib/copy_from_user.S | 14 +++++++++++---
  arch/arm64/lib/copy_in_user.S   | 21 ++++++++++++++-------
  arch/arm64/lib/copy_to_user.S   | 14 +++++++++++---
  3 files changed, 36 insertions(+), 13 deletions(-)

diff --git a/arch/arm64/lib/copy_from_user.S b/arch/arm64/lib/copy_from_user.S
index 95cd62d67371..5b720a29a242 100644
--- a/arch/arm64/lib/copy_from_user.S
+++ b/arch/arm64/lib/copy_from_user.S
@@ -29,7 +29,7 @@
  	.endm
  
  	.macro ldrh1 reg, ptr, val
-	user_ldst 9998f, ldtrh, \reg, \ptr, \val
+	user_ldst 9997f, ldtrh, \reg, \ptr, \val
  	.endm
  
  	.macro strh1 reg, ptr, val
@@ -37,7 +37,7 @@
  	.endm
  
  	.macro ldr1 reg, ptr, val
-	user_ldst 9998f, ldtr, \reg, \ptr, \val
+	user_ldst 9997f, ldtr, \reg, \ptr, \val
  	.endm
  
  	.macro str1 reg, ptr, val
@@ -45,7 +45,7 @@
  	.endm
  
  	.macro ldp1 reg1, reg2, ptr, val
-	user_ldp 9998f, \reg1, \reg2, \ptr, \val
+	user_ldp 9997f, \reg1, \reg2, \ptr, \val
  	.endm
  
  	.macro stp1 reg1, reg2, ptr, val
@@ -53,8 +53,10 @@
  	.endm
  
  end	.req	x5
+srcin	.req	x15
  SYM_FUNC_START(__arch_copy_from_user)
  	add	end, x0, x2
+	mov	srcin, x1
  #include "copy_template.S"
  	mov	x0, #0				// Nothing to copy
  	ret
@@ -63,6 +65,12 @@ EXPORT_SYMBOL(__arch_copy_from_user)
  
  	.section .fixup,"ax"
  	.align	2
+9997:	cmp	dst, dstin
+	b.ne	9998f
+	// Before being absolutely sure we couldn't copy anything, try harder
+USER(9998f, ldtrb tmp1w, [srcin])
+	strb	tmp1w, [dstin]
+	add	dst, dstin, #1
  9998:	sub	x0, end, dst			// bytes not copied
  	ret
  	.previous
diff --git a/arch/arm64/lib/copy_in_user.S b/arch/arm64/lib/copy_in_user.S
index 1f61cd0df062..0abb24a01781 100644
--- a/arch/arm64/lib/copy_in_user.S
+++ b/arch/arm64/lib/copy_in_user.S
@@ -30,33 +30,34 @@
  	.endm
  
  	.macro ldrh1 reg, ptr, val
-	user_ldst 9998f, ldtrh, \reg, \ptr, \val
+	user_ldst 9997f, ldtrh, \reg, \ptr, \val
  	.endm
  
  	.macro strh1 reg, ptr, val
-	user_ldst 9998f, sttrh, \reg, \ptr, \val
+	user_ldst 9997f, sttrh, \reg, \ptr, \val
  	.endm
  
  	.macro ldr1 reg, ptr, val
-	user_ldst 9998f, ldtr, \reg, \ptr, \val
+	user_ldst 9997f, ldtr, \reg, \ptr, \val
  	.endm
  
  	.macro str1 reg, ptr, val
-	user_ldst 9998f, sttr, \reg, \ptr, \val
+	user_ldst 9997f, sttr, \reg, \ptr, \val
  	.endm
  
  	.macro ldp1 reg1, reg2, ptr, val
-	user_ldp 9998f, \reg1, \reg2, \ptr, \val
+	user_ldp 9997f, \reg1, \reg2, \ptr, \val
  	.endm
  
  	.macro stp1 reg1, reg2, ptr, val
-	user_stp 9998f, \reg1, \reg2, \ptr, \val
+	user_stp 9997f, \reg1, \reg2, \ptr, \val
  	.endm
  
  end	.req	x5
-
+srcin	.req	x15
  SYM_FUNC_START(__arch_copy_in_user)
  	add	end, x0, x2
+	mov	srcin, x1
  #include "copy_template.S"
  	mov	x0, #0
  	ret
@@ -65,6 +66,12 @@ EXPORT_SYMBOL(__arch_copy_in_user)
  
  	.section .fixup,"ax"
  	.align	2
+9997:	cmp	dst, dstin
+	b.ne	9998f
+	// Before being absolutely sure we couldn't copy anything, try harder
+USER(9998f, ldtrb tmp1w, [srcin])
+USER(9998f, sttrb tmp1w, [dstin])
+	add	dst, dstin, #1
  9998:	sub	x0, end, dst			// bytes not copied
  	ret
  	.previous
diff --git a/arch/arm64/lib/copy_to_user.S b/arch/arm64/lib/copy_to_user.S
index 043da90f5dd7..cfb598ae4812 100644
--- a/arch/arm64/lib/copy_to_user.S
+++ b/arch/arm64/lib/copy_to_user.S
@@ -32,7 +32,7 @@
  	.endm
  
  	.macro strh1 reg, ptr, val
-	user_ldst 9998f, sttrh, \reg, \ptr, \val
+	user_ldst 9997f, sttrh, \reg, \ptr, \val
  	.endm
  
  	.macro ldr1 reg, ptr, val
@@ -40,7 +40,7 @@
  	.endm
  
  	.macro str1 reg, ptr, val
-	user_ldst 9998f, sttr, \reg, \ptr, \val
+	user_ldst 9997f, sttr, \reg, \ptr, \val
  	.endm
  
  	.macro ldp1 reg1, reg2, ptr, val
@@ -48,12 +48,14 @@
  	.endm
  
  	.macro stp1 reg1, reg2, ptr, val
-	user_stp 9998f, \reg1, \reg2, \ptr, \val
+	user_stp 9997f, \reg1, \reg2, \ptr, \val
  	.endm
  
  end	.req	x5
+srcin	.req	x15
  SYM_FUNC_START(__arch_copy_to_user)
  	add	end, x0, x2
+	mov	srcin, x1
  #include "copy_template.S"
  	mov	x0, #0
  	ret
@@ -62,6 +64,12 @@ EXPORT_SYMBOL(__arch_copy_to_user)
  
  	.section .fixup,"ax"
  	.align	2
+9997:	cmp	dst, dstin
+	b.ne	9998f
+	// Before being absolutely sure we couldn't copy anything, try harder
+	ldrb	tmp1w, [srcin]
+USER(9998f, sttrb tmp1w, [dstin])
+	add	dst, dstin, #1
  9998:	sub	x0, end, dst			// bytes not copied
  	ret
  	.previous
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Catalin Marinas]

On Mon, Jun 28, 2021 at 05:22:30PM +0100, Robin Murphy wrote:
> From: Robin Murphy <robin.murphy@arm.com>
> Subject: [PATCH] arm64: Avoid premature usercopy failure
> 
> Al reminds us that the usercopy API must only return complete failure
> if absolutely nothing could be copied. Currently, if userspace does
> something silly like giving us an unaligned pointer to Device memory,
> or a size which overruns MTE tag bounds, we may fail to honour that
> requirement when faulting on a multi-byte access even though a smaller
> access could have succeeded.
> 
> Add a mitigation to the fixup routines to fall back to a single-byte
> copy if we faulted on a larger access before anything has been written
> to the destination, to guarantee making *some* forward progress. We
> needn't be too concerned about the overall performance since this should
> only occur when callers are doing something a bit dodgy in the first
> place. Particularly broken userspace might still be able to trick
> generic_perform_write() into an infinite loop by targeting write() at
> an mmap() of some read-only device register where the fault-in load
> succeeds but any store synchronously aborts such that copy_to_user() is
> genuinely unable to make progress, but, well, don't do that...
> 
> Reported-by: Chen Huang <chenhuang5@huawei.com>
> Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>

Thanks Robin for putting this together. I'll write some MTE kselftests
to check for regressions in the future.

> diff --git a/arch/arm64/lib/copy_from_user.S b/arch/arm64/lib/copy_from_user.S
> index 95cd62d67371..5b720a29a242 100644
> --- a/arch/arm64/lib/copy_from_user.S
> +++ b/arch/arm64/lib/copy_from_user.S
> @@ -29,7 +29,7 @@
>  	.endm
>  	.macro ldrh1 reg, ptr, val
> -	user_ldst 9998f, ldtrh, \reg, \ptr, \val
> +	user_ldst 9997f, ldtrh, \reg, \ptr, \val
>  	.endm
>  	.macro strh1 reg, ptr, val
> @@ -37,7 +37,7 @@
>  	.endm
>  	.macro ldr1 reg, ptr, val
> -	user_ldst 9998f, ldtr, \reg, \ptr, \val
> +	user_ldst 9997f, ldtr, \reg, \ptr, \val
>  	.endm
>  	.macro str1 reg, ptr, val
> @@ -45,7 +45,7 @@
>  	.endm
>  	.macro ldp1 reg1, reg2, ptr, val
> -	user_ldp 9998f, \reg1, \reg2, \ptr, \val
> +	user_ldp 9997f, \reg1, \reg2, \ptr, \val
>  	.endm
>  	.macro stp1 reg1, reg2, ptr, val
> @@ -53,8 +53,10 @@
>  	.endm
>  end	.req	x5
> +srcin	.req	x15
>  SYM_FUNC_START(__arch_copy_from_user)
>  	add	end, x0, x2
> +	mov	srcin, x1
>  #include "copy_template.S"
>  	mov	x0, #0				// Nothing to copy
>  	ret
> @@ -63,6 +65,12 @@ EXPORT_SYMBOL(__arch_copy_from_user)
>  	.section .fixup,"ax"
>  	.align	2
> +9997:	cmp	dst, dstin
> +	b.ne	9998f
> +	// Before being absolutely sure we couldn't copy anything, try harder
> +USER(9998f, ldtrb tmp1w, [srcin])
> +	strb	tmp1w, [dstin]
> +	add	dst, dstin, #1

Nitpick: can we do just strb tmb1w, [dst], #1? It matches the strb1
macro in this file.

Either way, it looks fine to me.

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Catalin Marinas]

On Mon, Jun 28, 2021 at 05:22:30PM +0100, Robin Murphy wrote:
> From: Robin Murphy <robin.murphy@arm.com>
> Subject: [PATCH] arm64: Avoid premature usercopy failure
> 
> Al reminds us that the usercopy API must only return complete failure
> if absolutely nothing could be copied. Currently, if userspace does
> something silly like giving us an unaligned pointer to Device memory,
> or a size which overruns MTE tag bounds, we may fail to honour that
> requirement when faulting on a multi-byte access even though a smaller
> access could have succeeded.
> 
> Add a mitigation to the fixup routines to fall back to a single-byte
> copy if we faulted on a larger access before anything has been written
> to the destination, to guarantee making *some* forward progress. We
> needn't be too concerned about the overall performance since this should
> only occur when callers are doing something a bit dodgy in the first
> place. Particularly broken userspace might still be able to trick
> generic_perform_write() into an infinite loop by targeting write() at
> an mmap() of some read-only device register where the fault-in load
> succeeds but any store synchronously aborts such that copy_to_user() is
> genuinely unable to make progress, but, well, don't do that...
> 
> Reported-by: Chen Huang <chenhuang5@huawei.com>
> Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>

Thanks Robin for putting this together. I'll write some MTE kselftests
to check for regressions in the future.

> diff --git a/arch/arm64/lib/copy_from_user.S b/arch/arm64/lib/copy_from_user.S
> index 95cd62d67371..5b720a29a242 100644
> --- a/arch/arm64/lib/copy_from_user.S
> +++ b/arch/arm64/lib/copy_from_user.S
> @@ -29,7 +29,7 @@
>  	.endm
>  	.macro ldrh1 reg, ptr, val
> -	user_ldst 9998f, ldtrh, \reg, \ptr, \val
> +	user_ldst 9997f, ldtrh, \reg, \ptr, \val
>  	.endm
>  	.macro strh1 reg, ptr, val
> @@ -37,7 +37,7 @@
>  	.endm
>  	.macro ldr1 reg, ptr, val
> -	user_ldst 9998f, ldtr, \reg, \ptr, \val
> +	user_ldst 9997f, ldtr, \reg, \ptr, \val
>  	.endm
>  	.macro str1 reg, ptr, val
> @@ -45,7 +45,7 @@
>  	.endm
>  	.macro ldp1 reg1, reg2, ptr, val
> -	user_ldp 9998f, \reg1, \reg2, \ptr, \val
> +	user_ldp 9997f, \reg1, \reg2, \ptr, \val
>  	.endm
>  	.macro stp1 reg1, reg2, ptr, val
> @@ -53,8 +53,10 @@
>  	.endm
>  end	.req	x5
> +srcin	.req	x15
>  SYM_FUNC_START(__arch_copy_from_user)
>  	add	end, x0, x2
> +	mov	srcin, x1
>  #include "copy_template.S"
>  	mov	x0, #0				// Nothing to copy
>  	ret
> @@ -63,6 +65,12 @@ EXPORT_SYMBOL(__arch_copy_from_user)
>  	.section .fixup,"ax"
>  	.align	2
> +9997:	cmp	dst, dstin
> +	b.ne	9998f
> +	// Before being absolutely sure we couldn't copy anything, try harder
> +USER(9998f, ldtrb tmp1w, [srcin])
> +	strb	tmp1w, [dstin]
> +	add	dst, dstin, #1

Nitpick: can we do just strb tmb1w, [dst], #1? It matches the strb1
macro in this file.

Either way, it looks fine to me.

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Robin Murphy]

On 2021-06-29 09:30, Catalin Marinas wrote:
> On Mon, Jun 28, 2021 at 05:22:30PM +0100, Robin Murphy wrote:
>> From: Robin Murphy <robin.murphy@arm.com>
>> Subject: [PATCH] arm64: Avoid premature usercopy failure
>>
>> Al reminds us that the usercopy API must only return complete failure
>> if absolutely nothing could be copied. Currently, if userspace does
>> something silly like giving us an unaligned pointer to Device memory,
>> or a size which overruns MTE tag bounds, we may fail to honour that
>> requirement when faulting on a multi-byte access even though a smaller
>> access could have succeeded.
>>
>> Add a mitigation to the fixup routines to fall back to a single-byte
>> copy if we faulted on a larger access before anything has been written
>> to the destination, to guarantee making *some* forward progress. We
>> needn't be too concerned about the overall performance since this should
>> only occur when callers are doing something a bit dodgy in the first
>> place. Particularly broken userspace might still be able to trick
>> generic_perform_write() into an infinite loop by targeting write() at
>> an mmap() of some read-only device register where the fault-in load
>> succeeds but any store synchronously aborts such that copy_to_user() is
>> genuinely unable to make progress, but, well, don't do that...
>>
>> Reported-by: Chen Huang <chenhuang5@huawei.com>
>> Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> 
> Thanks Robin for putting this together. I'll write some MTE kselftests
> to check for regressions in the future.
> 
>> diff --git a/arch/arm64/lib/copy_from_user.S b/arch/arm64/lib/copy_from_user.S
>> index 95cd62d67371..5b720a29a242 100644
>> --- a/arch/arm64/lib/copy_from_user.S
>> +++ b/arch/arm64/lib/copy_from_user.S
>> @@ -29,7 +29,7 @@
>>   	.endm
>>   	.macro ldrh1 reg, ptr, val
>> -	user_ldst 9998f, ldtrh, \reg, \ptr, \val
>> +	user_ldst 9997f, ldtrh, \reg, \ptr, \val
>>   	.endm
>>   	.macro strh1 reg, ptr, val
>> @@ -37,7 +37,7 @@
>>   	.endm
>>   	.macro ldr1 reg, ptr, val
>> -	user_ldst 9998f, ldtr, \reg, \ptr, \val
>> +	user_ldst 9997f, ldtr, \reg, \ptr, \val
>>   	.endm
>>   	.macro str1 reg, ptr, val
>> @@ -45,7 +45,7 @@
>>   	.endm
>>   	.macro ldp1 reg1, reg2, ptr, val
>> -	user_ldp 9998f, \reg1, \reg2, \ptr, \val
>> +	user_ldp 9997f, \reg1, \reg2, \ptr, \val
>>   	.endm
>>   	.macro stp1 reg1, reg2, ptr, val
>> @@ -53,8 +53,10 @@
>>   	.endm
>>   end	.req	x5
>> +srcin	.req	x15
>>   SYM_FUNC_START(__arch_copy_from_user)
>>   	add	end, x0, x2
>> +	mov	srcin, x1
>>   #include "copy_template.S"
>>   	mov	x0, #0				// Nothing to copy
>>   	ret
>> @@ -63,6 +65,12 @@ EXPORT_SYMBOL(__arch_copy_from_user)
>>   	.section .fixup,"ax"
>>   	.align	2
>> +9997:	cmp	dst, dstin
>> +	b.ne	9998f
>> +	// Before being absolutely sure we couldn't copy anything, try harder
>> +USER(9998f, ldtrb tmp1w, [srcin])
>> +	strb	tmp1w, [dstin]
>> +	add	dst, dstin, #1
> 
> Nitpick: can we do just strb tmb1w, [dst], #1? It matches the strb1
> macro in this file.

Oh, of course; I think I befuddled myself there and failed to consider 
that by this point we've already mandated that dstin == dst (so that we 
can use srcin because src may have advanced already), so in fact we 
*can* use dst as the base register to avoid the shuffling, and 
post-index this one. I'll clean that up.

> Either way, it looks fine to me.
> 
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

Thanks!

Robin.

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Robin Murphy]

On 2021-06-29 09:30, Catalin Marinas wrote:
> On Mon, Jun 28, 2021 at 05:22:30PM +0100, Robin Murphy wrote:
>> From: Robin Murphy <robin.murphy@arm.com>
>> Subject: [PATCH] arm64: Avoid premature usercopy failure
>>
>> Al reminds us that the usercopy API must only return complete failure
>> if absolutely nothing could be copied. Currently, if userspace does
>> something silly like giving us an unaligned pointer to Device memory,
>> or a size which overruns MTE tag bounds, we may fail to honour that
>> requirement when faulting on a multi-byte access even though a smaller
>> access could have succeeded.
>>
>> Add a mitigation to the fixup routines to fall back to a single-byte
>> copy if we faulted on a larger access before anything has been written
>> to the destination, to guarantee making *some* forward progress. We
>> needn't be too concerned about the overall performance since this should
>> only occur when callers are doing something a bit dodgy in the first
>> place. Particularly broken userspace might still be able to trick
>> generic_perform_write() into an infinite loop by targeting write() at
>> an mmap() of some read-only device register where the fault-in load
>> succeeds but any store synchronously aborts such that copy_to_user() is
>> genuinely unable to make progress, but, well, don't do that...
>>
>> Reported-by: Chen Huang <chenhuang5@huawei.com>
>> Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> 
> Thanks Robin for putting this together. I'll write some MTE kselftests
> to check for regressions in the future.
> 
>> diff --git a/arch/arm64/lib/copy_from_user.S b/arch/arm64/lib/copy_from_user.S
>> index 95cd62d67371..5b720a29a242 100644
>> --- a/arch/arm64/lib/copy_from_user.S
>> +++ b/arch/arm64/lib/copy_from_user.S
>> @@ -29,7 +29,7 @@
>>   	.endm
>>   	.macro ldrh1 reg, ptr, val
>> -	user_ldst 9998f, ldtrh, \reg, \ptr, \val
>> +	user_ldst 9997f, ldtrh, \reg, \ptr, \val
>>   	.endm
>>   	.macro strh1 reg, ptr, val
>> @@ -37,7 +37,7 @@
>>   	.endm
>>   	.macro ldr1 reg, ptr, val
>> -	user_ldst 9998f, ldtr, \reg, \ptr, \val
>> +	user_ldst 9997f, ldtr, \reg, \ptr, \val
>>   	.endm
>>   	.macro str1 reg, ptr, val
>> @@ -45,7 +45,7 @@
>>   	.endm
>>   	.macro ldp1 reg1, reg2, ptr, val
>> -	user_ldp 9998f, \reg1, \reg2, \ptr, \val
>> +	user_ldp 9997f, \reg1, \reg2, \ptr, \val
>>   	.endm
>>   	.macro stp1 reg1, reg2, ptr, val
>> @@ -53,8 +53,10 @@
>>   	.endm
>>   end	.req	x5
>> +srcin	.req	x15
>>   SYM_FUNC_START(__arch_copy_from_user)
>>   	add	end, x0, x2
>> +	mov	srcin, x1
>>   #include "copy_template.S"
>>   	mov	x0, #0				// Nothing to copy
>>   	ret
>> @@ -63,6 +65,12 @@ EXPORT_SYMBOL(__arch_copy_from_user)
>>   	.section .fixup,"ax"
>>   	.align	2
>> +9997:	cmp	dst, dstin
>> +	b.ne	9998f
>> +	// Before being absolutely sure we couldn't copy anything, try harder
>> +USER(9998f, ldtrb tmp1w, [srcin])
>> +	strb	tmp1w, [dstin]
>> +	add	dst, dstin, #1
> 
> Nitpick: can we do just strb tmb1w, [dst], #1? It matches the strb1
> macro in this file.

Oh, of course; I think I befuddled myself there and failed to consider 
that by this point we've already mandated that dstin == dst (so that we 
can use srcin because src may have advanced already), so in fact we 
*can* use dst as the base register to avoid the shuffling, and 
post-index this one. I'll clean that up.

> Either way, it looks fine to me.
> 
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

Thanks!

Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Catalin Marinas]

On Mon, Jun 28, 2021 at 05:22:30PM +0100, Robin Murphy wrote:
> diff --git a/arch/arm64/lib/copy_to_user.S b/arch/arm64/lib/copy_to_user.S
> index 043da90f5dd7..cfb598ae4812 100644
> --- a/arch/arm64/lib/copy_to_user.S
> +++ b/arch/arm64/lib/copy_to_user.S
> @@ -32,7 +32,7 @@
>  	.endm
>  	.macro strh1 reg, ptr, val
> -	user_ldst 9998f, sttrh, \reg, \ptr, \val
> +	user_ldst 9997f, sttrh, \reg, \ptr, \val
>  	.endm
>  	.macro ldr1 reg, ptr, val
> @@ -40,7 +40,7 @@
>  	.endm
>  	.macro str1 reg, ptr, val
> -	user_ldst 9998f, sttr, \reg, \ptr, \val
> +	user_ldst 9997f, sttr, \reg, \ptr, \val
>  	.endm
>  	.macro ldp1 reg1, reg2, ptr, val
> @@ -48,12 +48,14 @@
>  	.endm
>  	.macro stp1 reg1, reg2, ptr, val
> -	user_stp 9998f, \reg1, \reg2, \ptr, \val
> +	user_stp 9997f, \reg1, \reg2, \ptr, \val
>  	.endm
>  end	.req	x5
> +srcin	.req	x15
>  SYM_FUNC_START(__arch_copy_to_user)
>  	add	end, x0, x2
> +	mov	srcin, x1
>  #include "copy_template.S"
>  	mov	x0, #0
>  	ret
> @@ -62,6 +64,12 @@ EXPORT_SYMBOL(__arch_copy_to_user)
>  	.section .fixup,"ax"
>  	.align	2
> +9997:	cmp	dst, dstin
> +	b.ne	9998f
> +	// Before being absolutely sure we couldn't copy anything, try harder
> +	ldrb	tmp1w, [srcin]
> +USER(9998f, sttrb tmp1w, [dstin])
> +	add	dst, dstin, #1
>  9998:	sub	x0, end, dst			// bytes not copied
>  	ret
>  	.previous

I think it's worth doing the copy_to_user() fallback in a loop until it
faults or hits the end of the buffer. This would solve the problem we
currently have with writing more bytes than actually reported. The
copy_from_user() is not necessary, a byte would suffice.

-- 
Catalin

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Catalin Marinas]

On Mon, Jun 28, 2021 at 05:22:30PM +0100, Robin Murphy wrote:
> diff --git a/arch/arm64/lib/copy_to_user.S b/arch/arm64/lib/copy_to_user.S
> index 043da90f5dd7..cfb598ae4812 100644
> --- a/arch/arm64/lib/copy_to_user.S
> +++ b/arch/arm64/lib/copy_to_user.S
> @@ -32,7 +32,7 @@
>  	.endm
>  	.macro strh1 reg, ptr, val
> -	user_ldst 9998f, sttrh, \reg, \ptr, \val
> +	user_ldst 9997f, sttrh, \reg, \ptr, \val
>  	.endm
>  	.macro ldr1 reg, ptr, val
> @@ -40,7 +40,7 @@
>  	.endm
>  	.macro str1 reg, ptr, val
> -	user_ldst 9998f, sttr, \reg, \ptr, \val
> +	user_ldst 9997f, sttr, \reg, \ptr, \val
>  	.endm
>  	.macro ldp1 reg1, reg2, ptr, val
> @@ -48,12 +48,14 @@
>  	.endm
>  	.macro stp1 reg1, reg2, ptr, val
> -	user_stp 9998f, \reg1, \reg2, \ptr, \val
> +	user_stp 9997f, \reg1, \reg2, \ptr, \val
>  	.endm
>  end	.req	x5
> +srcin	.req	x15
>  SYM_FUNC_START(__arch_copy_to_user)
>  	add	end, x0, x2
> +	mov	srcin, x1
>  #include "copy_template.S"
>  	mov	x0, #0
>  	ret
> @@ -62,6 +64,12 @@ EXPORT_SYMBOL(__arch_copy_to_user)
>  	.section .fixup,"ax"
>  	.align	2
> +9997:	cmp	dst, dstin
> +	b.ne	9998f
> +	// Before being absolutely sure we couldn't copy anything, try harder
> +	ldrb	tmp1w, [srcin]
> +USER(9998f, sttrb tmp1w, [dstin])
> +	add	dst, dstin, #1
>  9998:	sub	x0, end, dst			// bytes not copied
>  	ret
>  	.previous

I think it's worth doing the copy_to_user() fallback in a loop until it
faults or hits the end of the buffer. This would solve the problem we
currently have with writing more bytes than actually reported. The
copy_from_user() is not necessary, a byte would suffice.

-- 
Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Robin Murphy]

On 2021-07-06 18:50, Catalin Marinas wrote:
> On Mon, Jun 28, 2021 at 05:22:30PM +0100, Robin Murphy wrote:
>> diff --git a/arch/arm64/lib/copy_to_user.S b/arch/arm64/lib/copy_to_user.S
>> index 043da90f5dd7..cfb598ae4812 100644
>> --- a/arch/arm64/lib/copy_to_user.S
>> +++ b/arch/arm64/lib/copy_to_user.S
>> @@ -32,7 +32,7 @@
>>   	.endm
>>   	.macro strh1 reg, ptr, val
>> -	user_ldst 9998f, sttrh, \reg, \ptr, \val
>> +	user_ldst 9997f, sttrh, \reg, \ptr, \val
>>   	.endm
>>   	.macro ldr1 reg, ptr, val
>> @@ -40,7 +40,7 @@
>>   	.endm
>>   	.macro str1 reg, ptr, val
>> -	user_ldst 9998f, sttr, \reg, \ptr, \val
>> +	user_ldst 9997f, sttr, \reg, \ptr, \val
>>   	.endm
>>   	.macro ldp1 reg1, reg2, ptr, val
>> @@ -48,12 +48,14 @@
>>   	.endm
>>   	.macro stp1 reg1, reg2, ptr, val
>> -	user_stp 9998f, \reg1, \reg2, \ptr, \val
>> +	user_stp 9997f, \reg1, \reg2, \ptr, \val
>>   	.endm
>>   end	.req	x5
>> +srcin	.req	x15
>>   SYM_FUNC_START(__arch_copy_to_user)
>>   	add	end, x0, x2
>> +	mov	srcin, x1
>>   #include "copy_template.S"
>>   	mov	x0, #0
>>   	ret
>> @@ -62,6 +64,12 @@ EXPORT_SYMBOL(__arch_copy_to_user)
>>   	.section .fixup,"ax"
>>   	.align	2
>> +9997:	cmp	dst, dstin
>> +	b.ne	9998f
>> +	// Before being absolutely sure we couldn't copy anything, try harder
>> +	ldrb	tmp1w, [srcin]
>> +USER(9998f, sttrb tmp1w, [dstin])
>> +	add	dst, dstin, #1
>>   9998:	sub	x0, end, dst			// bytes not copied
>>   	ret
>>   	.previous
> 
> I think it's worth doing the copy_to_user() fallback in a loop until it
> faults or hits the end of the buffer. This would solve the problem we
> currently have with writing more bytes than actually reported. The
> copy_from_user() is not necessary, a byte would suffice.

The thing is, we don't really have that problem since the set_fs cleanup 
removed IMP-DEF STP behaviour from the picture - even with the current 
mess we could perfectly well know which of the two STTRs faulted if we 
just put a little more effort in. Even, at worst, simply this:

diff --git a/arch/arm64/include/asm/asm-uaccess.h 
b/arch/arm64/include/asm/asm-uaccess.h
index ccedf548dac9..7513758bab3a 100644
--- a/arch/arm64/include/asm/asm-uaccess.h
+++ b/arch/arm64/include/asm/asm-uaccess.h
@@ -74,8 +74,9 @@ alternative_else_nop_endif

         .macro user_stp l, reg1, reg2, addr, post_inc
  8888:          sttr    \reg1, [\addr];
-8889:          sttr    \reg2, [\addr, #8];
-               add     \addr, \addr, \post_inc;
+               add     \addr, \addr, \post_inc / 2;
+8889:          sttr    \reg2, [\addr];
+               add     \addr, \addr, \post_inc / 2;

                 _asm_extable    8888b,\l;
                 _asm_extable    8889b,\l;

But yuck... If you think the potential under-reporting is worth fixing 
right now, rather than just letting it disappear in a future rewrite, 
then I'd still rather do it by passing the actual fault address to the 
current copy_to_user fixup. A retry loop could still technically 
under-report if the page disappears (or tag changes) between faulting on 
the second word of a pair and retrying from the first, so we'd want to 
pin the initial fault down to a single access anyway. All the loop would 
achieve after that is potentially fill in an extra 1-7 bytes right up to 
the offending page/tag boundary for the sake of being nice, which I 
remain unconvinced is worth the bother :)

Cheers,
Robin.

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Robin Murphy]

On 2021-07-06 18:50, Catalin Marinas wrote:
> On Mon, Jun 28, 2021 at 05:22:30PM +0100, Robin Murphy wrote:
>> diff --git a/arch/arm64/lib/copy_to_user.S b/arch/arm64/lib/copy_to_user.S
>> index 043da90f5dd7..cfb598ae4812 100644
>> --- a/arch/arm64/lib/copy_to_user.S
>> +++ b/arch/arm64/lib/copy_to_user.S
>> @@ -32,7 +32,7 @@
>>   	.endm
>>   	.macro strh1 reg, ptr, val
>> -	user_ldst 9998f, sttrh, \reg, \ptr, \val
>> +	user_ldst 9997f, sttrh, \reg, \ptr, \val
>>   	.endm
>>   	.macro ldr1 reg, ptr, val
>> @@ -40,7 +40,7 @@
>>   	.endm
>>   	.macro str1 reg, ptr, val
>> -	user_ldst 9998f, sttr, \reg, \ptr, \val
>> +	user_ldst 9997f, sttr, \reg, \ptr, \val
>>   	.endm
>>   	.macro ldp1 reg1, reg2, ptr, val
>> @@ -48,12 +48,14 @@
>>   	.endm
>>   	.macro stp1 reg1, reg2, ptr, val
>> -	user_stp 9998f, \reg1, \reg2, \ptr, \val
>> +	user_stp 9997f, \reg1, \reg2, \ptr, \val
>>   	.endm
>>   end	.req	x5
>> +srcin	.req	x15
>>   SYM_FUNC_START(__arch_copy_to_user)
>>   	add	end, x0, x2
>> +	mov	srcin, x1
>>   #include "copy_template.S"
>>   	mov	x0, #0
>>   	ret
>> @@ -62,6 +64,12 @@ EXPORT_SYMBOL(__arch_copy_to_user)
>>   	.section .fixup,"ax"
>>   	.align	2
>> +9997:	cmp	dst, dstin
>> +	b.ne	9998f
>> +	// Before being absolutely sure we couldn't copy anything, try harder
>> +	ldrb	tmp1w, [srcin]
>> +USER(9998f, sttrb tmp1w, [dstin])
>> +	add	dst, dstin, #1
>>   9998:	sub	x0, end, dst			// bytes not copied
>>   	ret
>>   	.previous
> 
> I think it's worth doing the copy_to_user() fallback in a loop until it
> faults or hits the end of the buffer. This would solve the problem we
> currently have with writing more bytes than actually reported. The
> copy_from_user() is not necessary, a byte would suffice.

The thing is, we don't really have that problem since the set_fs cleanup 
removed IMP-DEF STP behaviour from the picture - even with the current 
mess we could perfectly well know which of the two STTRs faulted if we 
just put a little more effort in. Even, at worst, simply this:

diff --git a/arch/arm64/include/asm/asm-uaccess.h 
b/arch/arm64/include/asm/asm-uaccess.h
index ccedf548dac9..7513758bab3a 100644
--- a/arch/arm64/include/asm/asm-uaccess.h
+++ b/arch/arm64/include/asm/asm-uaccess.h
@@ -74,8 +74,9 @@ alternative_else_nop_endif

         .macro user_stp l, reg1, reg2, addr, post_inc
  8888:          sttr    \reg1, [\addr];
-8889:          sttr    \reg2, [\addr, #8];
-               add     \addr, \addr, \post_inc;
+               add     \addr, \addr, \post_inc / 2;
+8889:          sttr    \reg2, [\addr];
+               add     \addr, \addr, \post_inc / 2;

                 _asm_extable    8888b,\l;
                 _asm_extable    8889b,\l;

But yuck... If you think the potential under-reporting is worth fixing 
right now, rather than just letting it disappear in a future rewrite, 
then I'd still rather do it by passing the actual fault address to the 
current copy_to_user fixup. A retry loop could still technically 
under-report if the page disappears (or tag changes) between faulting on 
the second word of a pair and retrying from the first, so we'd want to 
pin the initial fault down to a single access anyway. All the loop would 
achieve after that is potentially fill in an extra 1-7 bytes right up to 
the offending page/tag boundary for the sake of being nice, which I 
remain unconvinced is worth the bother :)

Cheers,
Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [BUG] arm64: an infinite loop in generic_perform_write()

[David Laight]

> > I think it's worth doing the copy_to_user() fallback in a loop until it
> > faults or hits the end of the buffer. This would solve the problem we
> > currently have with writing more bytes than actually reported. The
> > copy_from_user() is not necessary, a byte would suffice.
> 
> The thing is, we don't really have that problem since the set_fs cleanup
> removed IMP-DEF STP behaviour from the picture - even with the current
> mess we could perfectly well know which of the two STTRs faulted if we
...

There is a much more interesting case though.
It is possible for userspace to have supplied a misaligned
buffer that is mmapped to an IO address that doesn't support
misaligned accesses even though normal memory does support them.

So the 'byte retry' loop would work for the entire buffer.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [BUG] arm64: an infinite loop in generic_perform_write()

[David Laight]

> > I think it's worth doing the copy_to_user() fallback in a loop until it
> > faults or hits the end of the buffer. This would solve the problem we
> > currently have with writing more bytes than actually reported. The
> > copy_from_user() is not necessary, a byte would suffice.
> 
> The thing is, we don't really have that problem since the set_fs cleanup
> removed IMP-DEF STP behaviour from the picture - even with the current
> mess we could perfectly well know which of the two STTRs faulted if we
...

There is a much more interesting case though.
It is possible for userspace to have supplied a misaligned
buffer that is mmapped to an IO address that doesn't support
misaligned accesses even though normal memory does support them.

So the 'byte retry' loop would work for the entire buffer.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Robin Murphy]

On 2021-07-07 10:55, David Laight wrote:
>>> I think it's worth doing the copy_to_user() fallback in a loop until it
>>> faults or hits the end of the buffer. This would solve the problem we
>>> currently have with writing more bytes than actually reported. The
>>> copy_from_user() is not necessary, a byte would suffice.
>>
>> The thing is, we don't really have that problem since the set_fs cleanup
>> removed IMP-DEF STP behaviour from the picture - even with the current
>> mess we could perfectly well know which of the two STTRs faulted if we
> ...
> 
> There is a much more interesting case though.
> It is possible for userspace to have supplied a misaligned
> buffer that is mmapped to an IO address that doesn't support
> misaligned accesses even though normal memory does support them.

Er, yes, that's where this whole thing started - don't worry, I haven't 
forgotten.

> So the 'byte retry' loop would work for the entire buffer.

Indeed it might in certain cases, but is that (unlikely) possibility 
worth our while? What it boils down to is maintaining complexity in the 
kernel purely to humour broken userspace doing a nonsensical thing, when 
it's equally valid to just return a short read/write and let said broken 
userspace take responsibility for retrying the remainder of said 
nonsensical thing by itself. If userspace has managed to get its hands 
on an mmap of something without Normal memory semantics, I would expect 
it to know what it's doing...

Thanks,
Robin.

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Robin Murphy]

On 2021-07-07 10:55, David Laight wrote:
>>> I think it's worth doing the copy_to_user() fallback in a loop until it
>>> faults or hits the end of the buffer. This would solve the problem we
>>> currently have with writing more bytes than actually reported. The
>>> copy_from_user() is not necessary, a byte would suffice.
>>
>> The thing is, we don't really have that problem since the set_fs cleanup
>> removed IMP-DEF STP behaviour from the picture - even with the current
>> mess we could perfectly well know which of the two STTRs faulted if we
> ...
> 
> There is a much more interesting case though.
> It is possible for userspace to have supplied a misaligned
> buffer that is mmapped to an IO address that doesn't support
> misaligned accesses even though normal memory does support them.

Er, yes, that's where this whole thing started - don't worry, I haven't 
forgotten.

> So the 'byte retry' loop would work for the entire buffer.

Indeed it might in certain cases, but is that (unlikely) possibility 
worth our while? What it boils down to is maintaining complexity in the 
kernel purely to humour broken userspace doing a nonsensical thing, when 
it's equally valid to just return a short read/write and let said broken 
userspace take responsibility for retrying the remainder of said 
nonsensical thing by itself. If userspace has managed to get its hands 
on an mmap of something without Normal memory semantics, I would expect 
it to know what it's doing...

Thanks,
Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Catalin Marinas]

On Tue, Jul 06, 2021 at 08:15:47PM +0100, Robin Murphy wrote:
> On 2021-07-06 18:50, Catalin Marinas wrote:
> > On Mon, Jun 28, 2021 at 05:22:30PM +0100, Robin Murphy wrote:
> > > @@ -62,6 +64,12 @@ EXPORT_SYMBOL(__arch_copy_to_user)
> > >   	.section .fixup,"ax"
> > >   	.align	2
> > > +9997:	cmp	dst, dstin
> > > +	b.ne	9998f
> > > +	// Before being absolutely sure we couldn't copy anything, try harder
> > > +	ldrb	tmp1w, [srcin]
> > > +USER(9998f, sttrb tmp1w, [dstin])
> > > +	add	dst, dstin, #1
> > >   9998:	sub	x0, end, dst			// bytes not copied
> > >   	ret
> > >   	.previous
> > 
> > I think it's worth doing the copy_to_user() fallback in a loop until it
> > faults or hits the end of the buffer. This would solve the problem we
> > currently have with writing more bytes than actually reported. The
> > copy_from_user() is not necessary, a byte would suffice.
> 
> The thing is, we don't really have that problem since the set_fs cleanup
> removed IMP-DEF STP behaviour from the picture - even with the current mess
> we could perfectly well know which of the two STTRs faulted if we just put a
> little more effort in.

I think there are some corner cases: STTR across a page boundary,
faulting on the second page. The architecture allows some data to be
written (or not) in the first page, so we'd under-report if we use the
destination update. If we use the fault address it's even worse as we
may over-report in case the instruction did not write anything.

> But yuck... If you think the potential under-reporting is worth fixing right
> now, rather than just letting it disappear in a future rewrite, then I'd
> still rather do it by passing the actual fault address to the current
> copy_to_user fixup.

After some more digging in the ARM ARM, I don't think that's fixable by
using the actual fault address. B2.2.1 and D1.13.5 in version G.a
(thanks to Will for digging them out) mean that for an interrupted store
(exception, interrupt), any bytes stored by the instruction become
UNKNOWN. In practice, this means left unchanged or written.

So I think a byte-wise write loop is the only chance we have at a
more precise reporting, unless we change the loops to align the writes.

> A retry loop could still technically under-report if the
> page disappears (or tag changes) between faulting on the second word of a
> pair and retrying from the first, so we'd want to pin the initial fault down
> to a single access anyway. All the loop would achieve after that is
> potentially fill in an extra 1-7 bytes right up to the offending page/tag
> boundary for the sake of being nice, which I remain unconvinced is worth the
> bother :)

There is indeed the risk of a race but we can blame the user for
concurrently changing the permissions or tag. The kernel wouldn't
normally do this.

-- 
Catalin

Re: [BUG] arm64: an infinite loop in generic_perform_write()

[Catalin Marinas]

On Tue, Jul 06, 2021 at 08:15:47PM +0100, Robin Murphy wrote:
> On 2021-07-06 18:50, Catalin Marinas wrote:
> > On Mon, Jun 28, 2021 at 05:22:30PM +0100, Robin Murphy wrote:
> > > @@ -62,6 +64,12 @@ EXPORT_SYMBOL(__arch_copy_to_user)
> > >   	.section .fixup,"ax"
> > >   	.align	2
> > > +9997:	cmp	dst, dstin
> > > +	b.ne	9998f
> > > +	// Before being absolutely sure we couldn't copy anything, try harder
> > > +	ldrb	tmp1w, [srcin]
> > > +USER(9998f, sttrb tmp1w, [dstin])
> > > +	add	dst, dstin, #1
> > >   9998:	sub	x0, end, dst			// bytes not copied
> > >   	ret
> > >   	.previous
> > 
> > I think it's worth doing the copy_to_user() fallback in a loop until it
> > faults or hits the end of the buffer. This would solve the problem we
> > currently have with writing more bytes than actually reported. The
> > copy_from_user() is not necessary, a byte would suffice.
> 
> The thing is, we don't really have that problem since the set_fs cleanup
> removed IMP-DEF STP behaviour from the picture - even with the current mess
> we could perfectly well know which of the two STTRs faulted if we just put a
> little more effort in.

I think there are some corner cases: STTR across a page boundary,
faulting on the second page. The architecture allows some data to be
written (or not) in the first page, so we'd under-report if we use the
destination update. If we use the fault address it's even worse as we
may over-report in case the instruction did not write anything.

> But yuck... If you think the potential under-reporting is worth fixing right
> now, rather than just letting it disappear in a future rewrite, then I'd
> still rather do it by passing the actual fault address to the current
> copy_to_user fixup.

After some more digging in the ARM ARM, I don't think that's fixable by
using the actual fault address. B2.2.1 and D1.13.5 in version G.a
(thanks to Will for digging them out) mean that for an interrupted store
(exception, interrupt), any bytes stored by the instruction become
UNKNOWN. In practice, this means left unchanged or written.

So I think a byte-wise write loop is the only chance we have at a
more precise reporting, unless we change the loops to align the writes.

> A retry loop could still technically under-report if the
> page disappears (or tag changes) between faulting on the second word of a
> pair and retrying from the first, so we'd want to pin the initial fault down
> to a single access anyway. All the loop would achieve after that is
> potentially fill in an extra 1-7 bytes right up to the offending page/tag
> boundary for the sake of being nice, which I remain unconvinced is worth the
> bother :)

There is indeed the risk of a race but we can blame the user for
concurrently changing the permissions or tag. The kernel wouldn't
normally do this.

-- 
Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel