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.
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Comment Actions
The mechanics of this look fine, but I've not read the docs to know if the handling is correct.
| sys/arm/arm/trap-v6.c | ||
|---|---|---|
| 353 | 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. | |
Comment Actions
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.
Comment Actions
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.
Comment Actions
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 | ||
|---|---|---|
| 353 | One more thing. This clarification should go into a comment. | |
Comment Actions
The unprivileged instruction encodings are described in code now. The evaluation was moved to separate function to keep abort_handler() well-arranged.
Comment Actions
I forgot that the evaluation of unprivileged instructions was fixed and it should be correct now.
Comment Actions
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.
Comment Actions
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.