ARMv6 - fix aborts for copyin/out kind of functions

[skra]

Summary

It happens that there was missing abort logic for use of unprivileged instructions in trap-v6.c. These instructions are utilized in copyin/out functions where user address space is accessed from kernel and any abort should be resolved considering that. This was not true if an user provided address from KVA space. Fortunatelly, according to circumstances, it could cause only three things: (1) Never ending loop of aborts if KVA was mapped, or (2) panic on no fault entry if KVA was not mapped, or (3) panic on pt1 or PT2MAP if KVA was from their ranges.

[• hselasky]

Don't forget to MFC :-)

[imp]

The mechanics of this look fine, but I've not read the docs to know if the handling is correct.

[skra]

sys/arm/arm/trap-v6.c

446

Just for the record. Each of mentioned instructions has the following two encoding:

		bit 27-20 

LDRT	A1	01 00 U 011
	A2	01 10 U 011
LDRHT	A1	00 00 U 111
	A2	00 00 U 011
LDRBT   A1	01 00 U 111
	A2	01 10 U 111
	
STRT	A1	01 00 U 010
	A2	01 10 U 010
STRHT	A1	00 00 U 110
	A2	00 00 U 010
STRBT	A1	01 00 U 110
	A2	01 10 U 110

The other bits and bit U in these encodings are arbitrary (conditions, registers, immediate values, ... ). Thus only 7 bits carry info about instruction type and it means 128 possible combinations. Bit 21 is always 1, bit 24 and 27 are always 0. Thus an encoding is masked by 0x09200000 and compared with 0x00200000. After that only 16 combinations remain. If bit 25 is 1, then bit 26 is never zero. Thus an encoding is masked by 0x06000000 and compared with 0x02000000. After that 4 combinations are eliminated and only 12 ones remain. It agrees with 12 encondings of these instructions.

Note that each of these instructions has one more T1 (thumb) encoding and then, instruction size is 16 bits. But it's another story.

[kib]

Why do we need the ldrt/strt on FreeBSD, which cause such complications ? The FreeBSD UVA/KVA layout is very simple, the user/kernel is split by VM_MAXUSER_ADDRESS. copyin/copyout should be only allowed to access a buffer which is fully located below or eq VM_MAXUSER_ADDRESS.

Other architectures check that start < start + length and start + length <= VM_MAXUSER_ADDRESS (<= is correct). Then normal ldr/str instructions can be used, and additional handling of faults is not needed.

[skra]

In D3617#77934, @kib wrote:

Why do we need the ldrt/strt on FreeBSD, which cause such complications ? The FreeBSD UVA/KVA layout is very simple, the user/kernel is split by VM_MAXUSER_ADDRESS. copyin/copyout should be only allowed to access a buffer which is fully located below or eq VM_MAXUSER_ADDRESS.

Other architectures check that start < start + length and start + length <= VM_MAXUSER_ADDRESS (<= is correct). Then normal ldr/str instructions can be used, and additional handling of faults is not needed.

In fact, I do not know. When the problem was pointed out, I looked at how it was solved before and make the solution same. Maybe someone who remebers the time when the copyin/out functions were created for ARM could tell us the reason.

IMO, testing user provided address for VM_MAXUSER_ADDRESS seems to me easier and more clear. Maybe, once in copyin/out function implementation, they could be made more ARMv6 like. I mean.,maybe, they could be optimised better.

[kib]

In D3617#78032, @onwahe-gmail-com wrote:

In fact, I do not know. When the problem was pointed out, I looked at how it was solved before and make the solution same. Maybe someone who remebers the time when the copyin/out functions were created for ARM could tell us the reason.

IMO, testing user provided address for VM_MAXUSER_ADDRESS seems to me easier and more clear. Maybe, once in copyin/out function implementation, they could be made more ARMv6 like. I mean.,maybe, they could be optimised better.

After thinking about this more, I now believe that both your change, and a change to check VM_MAXUSER_ADDRESS, should be made. Your change provides working STRT/LDRT in case it appear to be needed, and it is pity to lost the work done. On the other hand, VM_MAXUSER_ADDRESS would provide a faster and easier to reason about, way to validate accesses. Also, it avoids some undesirable actions in the copyin/out path, like disabling interrupts.

If you have no time to code VM_MAXUSER_ADDRESS change, I could do it.

sys/arm/arm/trap-v6.c

446

One more thing. This clarification should go into a comment.

[skra]

The unprivileged instruction encodings are described in code now. The evaluation was moved to separate function to keep abort_handler() well-arranged.

[skra]

In D3617#78154, @kib wrote:

In D3617#78032, @onwahe-gmail-com wrote:

In fact, I do not know. When the problem was pointed out, I looked at how it was solved before and make the solution same. Maybe someone who remebers the time when the copyin/out functions were created for ARM could tell us the reason.

IMO, testing user provided address for VM_MAXUSER_ADDRESS seems to me easier and more clear. Maybe, once in copyin/out function implementation, they could be made more ARMv6 like. I mean.,maybe, they could be optimised better.

After thinking about this more, I now believe that both your change, and a change to check VM_MAXUSER_ADDRESS, should be made. Your change provides working STRT/LDRT in case it appear to be needed, and it is pity to lost the work done. On the other hand, VM_MAXUSER_ADDRESS would provide a faster and easier to reason about, way to validate accesses. Also, it avoids some undesirable actions in the copyin/out path, like disabling interrupts.

If you have no time to code VM_MAXUSER_ADDRESS change, I could do it.

You are welcome to do it.

[skra]

In D3617#78699, @onwahe-gmail-com wrote:

The unprivileged instruction encodings are described in code now. The evaluation was moved to separate function to keep abort_handler() well-arranged.

I forgot that the evaluation of unprivileged instructions was fixed and it should be correct now.

[skra]

There is no need to check for __ARM_ARCH >= 6 in file dedicated for armv6 only. The check should be done in place where such file is included. It's not in the scope of this patch to fix it. However, as a side effect of this little change, this patch is build-able on not armv6 arm platforms too.

[skra]

Update the patch to r294822.

The patch is not needed now after r289372 as unprivileged instructions are only used in copyin()/copyout() like functions. However, I would like to keep it updated as situation may be changed in the future.

[kib]

So just commit this ?