| Message ID | 20210622051204.3682580-1-pcc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] arm64: allow TCR_EL1.TBID0 to be configured | expand |
Hi Peter, On Mon, Jun 21, 2021 at 10:12:04PM -0700, Peter Collingbourne wrote: > Introduce a command line flag that controls whether TCR_EL1.TBID0 > is set at boot time. Since this is a change to the userspace ABI the > option defaults to off for now, although it seems likely that we'll > be able to change the default at some future point. > > Setting TCR_EL1.TBID0 increases the number of signature bits used by > the pointer authentication instructions for instruction addresses by 8, > which improves the security of pointer authentication, but it also has > the consequence of changing the operation of the branch instructions > so that they no longer ignore the top byte of the target address but > instead fault if they are non-zero. I'm a bit uneasy about the ABI change and not so keen on constraining the ABI through the kernel command line. Ideally we should make this an opt-in per application (prctl()) but that has some aspects to address first: (a) this bit is permitted to be cached in the TLB so we'd need some TLBI when setting it (and a clarification in the specs that it is tagged by ASID/VMID, probably fine) and (b) we'd need to context-switch TCR_EL1, with a small performance penalty (I don't think it's significant but worth testing). Unfortunately, we can't turn TBID0 off dynamically when we detect a tagged PC since this would break authentication of already encoded pointers. Prior to hwasan and MTE, I doubt anyone would have noticed this change but once malloc() and friends started returning tagged pointers, programs executing code from malloc()'ed regions would fall apart with TBID0. I think it's a bit of stretch to argue that it's hwasan and MTE causing the application breakage rather than a user-kernel ABI change, since that's already working currently (though such programs should be re-written). Longer term, I'd like the TBID0 to be the default but transitioning without breaking the user is tricky, hence my first option would be per-application with an opt-in. > diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c > index 871c82ab0a30..9ee32afe121c 100644 > --- a/arch/arm64/mm/fault.c > +++ b/arch/arm64/mm/fault.c > @@ -529,11 +529,23 @@ static int __kprobes do_page_fault(unsigned long far, unsigned int esr, > vm_fault_t fault; > unsigned long vm_flags; > unsigned int mm_flags = FAULT_FLAG_DEFAULT; > - unsigned long addr = untagged_addr(far); > + unsigned long addr; > > if (kprobe_page_fault(regs, esr)) > return 0; > > + /* > + * If TBID0 is set then we may get an IABT with a tagged address here as > + * a result of branching to a tagged address. In this case we want to > + * avoid untagging the address, let the VMA lookup fail and get a > + * SIGSEGV. Leaving the address as is will also work if TBID0 is clear > + * or unsupported because the tag bits of FAR_EL1 will be clear. > + */ > + if (is_el0_instruction_abort(esr)) > + addr = far; > + else > + addr = untagged_addr(far); Should this also check for tcr_tbid0_enabled() before deciding not to untag the address?
On Tue, Jul 27, 2021 at 9:51 AM Catalin Marinas <catalin.marinas@arm.com> wrote: > > Hi Peter, > > On Mon, Jun 21, 2021 at 10:12:04PM -0700, Peter Collingbourne wrote: > > Introduce a command line flag that controls whether TCR_EL1.TBID0 > > is set at boot time. Since this is a change to the userspace ABI the > > option defaults to off for now, although it seems likely that we'll > > be able to change the default at some future point. > > > > Setting TCR_EL1.TBID0 increases the number of signature bits used by > > the pointer authentication instructions for instruction addresses by 8, > > which improves the security of pointer authentication, but it also has > > the consequence of changing the operation of the branch instructions > > so that they no longer ignore the top byte of the target address but > > instead fault if they are non-zero. > > I'm a bit uneasy about the ABI change and not so keen on constraining > the ABI through the kernel command line. Ideally we should make this an > opt-in per application (prctl()) but that has some aspects to address This doesn't necessarily need to be the end state, we can enhance this based on need. For example, we could choose to take this patch now and later implement the per-process opt-in where the default is controlled by the command line. Or just implement the software-only per-process TBID0 almost-disablement which would be much simpler than doing it in hardware, if that is enough to satisfy future needs. Otherwise we risk adding "unused" complexity to the kernel that we can never remove due to API stability guarantees. > first: (a) this bit is permitted to be cached in the TLB so we'd need > some TLBI when setting it (and a clarification in the specs that it is > tagged by ASID/VMID, probably fine) and (b) we'd need to context-switch > TCR_EL1, with a small performance penalty (I don't think it's > significant but worth testing). So TLBI all of the CPUs on prctl() and context-switch TCR_EL1? I thought there would be DOS concerns with the first part of that? I hope it would be a straightforward spec clarification. Maybe Cortex tags TBID0 by ASID/VMID but what about Apple? If we can resolve these concerns I suppose that would work. But as mentioned above I'm not sure we should do it straight away. > Unfortunately, we can't turn TBID0 off dynamically when we detect a > tagged PC since this would break authentication of already encoded > pointers. > > Prior to hwasan and MTE, I doubt anyone would have noticed this change > but once malloc() and friends started returning tagged pointers, > programs executing code from malloc()'ed regions would fall apart with > TBID0. I think it's a bit of stretch to argue that it's hwasan and MTE > causing the application breakage rather than a user-kernel ABI change, > since that's already working currently (though such programs should be > re-written). > > Longer term, I'd like the TBID0 to be the default but transitioning > without breaking the user is tricky, hence my first option would be > per-application with an opt-in. If we want to transition to TBID0 being the default, it seems better to not let it be controlled by the application, if we can get away with it. Otherwise applications may start implicitly disabling it, or assuming it to be disabled by default. For example if we made it a bit in tagged_addr_ctrl then a call such as: prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE, 0, 0, 0); will disable TBID0 even if the default is for it to be enabled. (So it's a bit unfortunate that we didn't adopt the PR_PAC_SET_ENABLED_KEYS scheme for PR_SET_TAGGED_ADDR_CTRL.) > > diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c > > index 871c82ab0a30..9ee32afe121c 100644 > > --- a/arch/arm64/mm/fault.c > > +++ b/arch/arm64/mm/fault.c > > @@ -529,11 +529,23 @@ static int __kprobes do_page_fault(unsigned long far, unsigned int esr, > > vm_fault_t fault; > > unsigned long vm_flags; > > unsigned int mm_flags = FAULT_FLAG_DEFAULT; > > - unsigned long addr = untagged_addr(far); > > + unsigned long addr; > > > > if (kprobe_page_fault(regs, esr)) > > return 0; > > > > + /* > > + * If TBID0 is set then we may get an IABT with a tagged address here as > > + * a result of branching to a tagged address. In this case we want to > > + * avoid untagging the address, let the VMA lookup fail and get a > > + * SIGSEGV. Leaving the address as is will also work if TBID0 is clear > > + * or unsupported because the tag bits of FAR_EL1 will be clear. > > + */ > > + if (is_el0_instruction_abort(esr)) > > + addr = far; > > + else > > + addr = untagged_addr(far); > > Should this also check for tcr_tbid0_enabled() before deciding not to > untag the address? No, if TBID0 is disabled then by this point the hardware would have untagged the PC for us. Peter
On Tue, Jul 27, 2021 at 03:00:10PM -0700, Peter Collingbourne wrote: > On Tue, Jul 27, 2021 at 9:51 AM Catalin Marinas <catalin.marinas@arm.com> wrote: > > On Mon, Jun 21, 2021 at 10:12:04PM -0700, Peter Collingbourne wrote: > > > Introduce a command line flag that controls whether TCR_EL1.TBID0 > > > is set at boot time. Since this is a change to the userspace ABI the > > > option defaults to off for now, although it seems likely that we'll > > > be able to change the default at some future point. > > > > > > Setting TCR_EL1.TBID0 increases the number of signature bits used by > > > the pointer authentication instructions for instruction addresses by 8, > > > which improves the security of pointer authentication, but it also has > > > the consequence of changing the operation of the branch instructions > > > so that they no longer ignore the top byte of the target address but > > > instead fault if they are non-zero. > > > > I'm a bit uneasy about the ABI change and not so keen on constraining > > the ABI through the kernel command line. Ideally we should make this an > > opt-in per application (prctl()) but that has some aspects to address > > This doesn't necessarily need to be the end state, we can enhance this > based on need. For example, we could choose to take this patch now and > later implement the per-process opt-in where the default is controlled > by the command line. What's the risk of an application becoming reliant on the new mode (TBID0) and breaking with the old one? Probably very small but I haven't figured out if it's zero. Depending on whether we have PAC or PAC2 (the latter came with 8.6 but optional in 8.3) with TBID0, there are some differences on how the PAC/AUT instructions work and the code generated (XOR with the top bits). > Or just implement the software-only per-process > TBID0 almost-disablement which would be much simpler than doing it in > hardware, if that is enough to satisfy future needs. I don't entirely follow this. > Otherwise we risk adding "unused" complexity to the kernel that we can > never remove due to API stability guarantees. We've had other debates over the years and, in general, if a kernel change causes apps to break, we'd have to keep the original behaviour. Are there any plans to fix the JITs tools you discovered? Talking to Will about this he was wondering whether we could make TBID0 on by default and clear the tag in PC if we take a fault (on tagged PC), restarting the context. PAC shouldn't be affected since we would only branch to an authenticated (PAC code removed) pointer. If this works, we'd only affect performance slightly on such apps but don't completely break them. > > first: (a) this bit is permitted to be cached in the TLB so we'd need > > some TLBI when setting it (and a clarification in the specs that it is > > tagged by ASID/VMID, probably fine) and (b) we'd need to context-switch > > TCR_EL1, with a small performance penalty (I don't think it's > > significant but worth testing). > > So TLBI all of the CPUs on prctl() and context-switch TCR_EL1? I > thought there would be DOS concerns with the first part of that? The DoS problem appears if we need to issue an IPI to all CPUs (like stop_machine). The TLBI with broadcast handled in hardware should be OK as it's targeted to a specific ASID. But this would have to be issued before any app threads are started, otherwise we'd need to synchronise TCR_EL1. Given that TBID0 toggling affects PAC, this can only be done safely very early in the application before return addresses get a PAC code. > > Unfortunately, we can't turn TBID0 off dynamically when we detect a > > tagged PC since this would break authentication of already encoded > > pointers. > > > > Prior to hwasan and MTE, I doubt anyone would have noticed this change > > but once malloc() and friends started returning tagged pointers, > > programs executing code from malloc()'ed regions would fall apart with > > TBID0. I think it's a bit of stretch to argue that it's hwasan and MTE > > causing the application breakage rather than a user-kernel ABI change, > > since that's already working currently (though such programs should be > > re-written). > > > > Longer term, I'd like the TBID0 to be the default but transitioning > > without breaking the user is tricky, hence my first option would be > > per-application with an opt-in. > > If we want to transition to TBID0 being the default, it seems better > to not let it be controlled by the application, if we can get away > with it. Otherwise applications may start implicitly disabling it, or > assuming it to be disabled by default. For example if we made it a bit > in tagged_addr_ctrl then a call such as: > > prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE, 0, 0, 0); > > will disable TBID0 even if the default is for it to be enabled. (So > it's a bit unfortunate that we didn't adopt the > PR_PAC_SET_ENABLED_KEYS scheme for PR_SET_TAGGED_ADDR_CTRL.) At the time, we didn't see a need for it ;). But if it's an opt-in, the default should be off anyway. > > > diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c > > > index 871c82ab0a30..9ee32afe121c 100644 > > > --- a/arch/arm64/mm/fault.c > > > +++ b/arch/arm64/mm/fault.c > > > @@ -529,11 +529,23 @@ static int __kprobes do_page_fault(unsigned long far, unsigned int esr, > > > vm_fault_t fault; > > > unsigned long vm_flags; > > > unsigned int mm_flags = FAULT_FLAG_DEFAULT; > > > - unsigned long addr = untagged_addr(far); > > > + unsigned long addr; > > > > > > if (kprobe_page_fault(regs, esr)) > > > return 0; > > > > > > + /* > > > + * If TBID0 is set then we may get an IABT with a tagged address here as > > > + * a result of branching to a tagged address. In this case we want to > > > + * avoid untagging the address, let the VMA lookup fail and get a > > > + * SIGSEGV. Leaving the address as is will also work if TBID0 is clear > > > + * or unsupported because the tag bits of FAR_EL1 will be clear. > > > + */ > > > + if (is_el0_instruction_abort(esr)) > > > + addr = far; > > > + else > > > + addr = untagged_addr(far); > > > > Should this also check for tcr_tbid0_enabled() before deciding not to > > untag the address? > > No, if TBID0 is disabled then by this point the hardware would have > untagged the PC for us. Ah, ok, didn't realise that the tag is removed by the hardware.
On Wed, Jul 28, 2021 at 9:42 AM Catalin Marinas <catalin.marinas@arm.com> wrote: > > On Tue, Jul 27, 2021 at 03:00:10PM -0700, Peter Collingbourne wrote: > > On Tue, Jul 27, 2021 at 9:51 AM Catalin Marinas <catalin.marinas@arm.com> wrote: > > > On Mon, Jun 21, 2021 at 10:12:04PM -0700, Peter Collingbourne wrote: > > > > Introduce a command line flag that controls whether TCR_EL1.TBID0 > > > > is set at boot time. Since this is a change to the userspace ABI the > > > > option defaults to off for now, although it seems likely that we'll > > > > be able to change the default at some future point. > > > > > > > > Setting TCR_EL1.TBID0 increases the number of signature bits used by > > > > the pointer authentication instructions for instruction addresses by 8, > > > > which improves the security of pointer authentication, but it also has > > > > the consequence of changing the operation of the branch instructions > > > > so that they no longer ignore the top byte of the target address but > > > > instead fault if they are non-zero. > > > > > > I'm a bit uneasy about the ABI change and not so keen on constraining > > > the ABI through the kernel command line. Ideally we should make this an > > > opt-in per application (prctl()) but that has some aspects to address > > > > This doesn't necessarily need to be the end state, we can enhance this > > based on need. For example, we could choose to take this patch now and > > later implement the per-process opt-in where the default is controlled > > by the command line. > > What's the risk of an application becoming reliant on the new mode > (TBID0) and breaking with the old one? Probably very small but I haven't > figured out if it's zero. Depending on whether we have PAC or PAC2 (the > latter came with 8.6 but optional in 8.3) with TBID0, there are some > differences on how the PAC/AUT instructions work and the code generated > (XOR with the top bits). I think it would be quite small. On Android, at least to begin with there would be a mixture of devices with different TBID0 settings (devices without PAC support and devices with older kernels would all have this disabled), so I think it would be difficult for an application to depend on it being enabled. > > Or just implement the software-only per-process > > TBID0 almost-disablement which would be much simpler than doing it in > > hardware, if that is enough to satisfy future needs. > > I don't entirely follow this. Sorry, I was referring to my earlier proposal for recovering from tagged PC in the kernel by clearing the tag bits: https://lore.kernel.org/linux-arm-kernel/CAMn1gO7+_JhTUw2gS6jnyRV+TCqprmpuCAfee3RCAhNjoVyy9w@mail.gmail.com/ > > Otherwise we risk adding "unused" complexity to the kernel that we can > > never remove due to API stability guarantees. > > We've had other debates over the years and, in general, if a kernel > change causes apps to break, we'd have to keep the original behaviour. > Are there any plans to fix the JITs tools you discovered? Yes, we would definitely want to fix the JIT issue in the Android platform before rolling out a forward PAC ABI. This would be separate from fixing apps, which would need to opt into MTE (or address tagging via the target API level) anyway. But if it turns out that there are too many apps with these JITs that use MTE or address tagging, I think we would need to come back to the kernel to figure out some way to let these programs run. > Talking to Will about this he was wondering whether we could make TBID0 > on by default and clear the tag in PC if we take a fault (on tagged PC), > restarting the context. PAC shouldn't be affected since we would only > branch to an authenticated (PAC code removed) pointer. If this works, > we'd only affect performance slightly on such apps but don't completely > break them. Right, this sounds exactly like my earlier proposal. > > > first: (a) this bit is permitted to be cached in the TLB so we'd need > > > some TLBI when setting it (and a clarification in the specs that it is > > > tagged by ASID/VMID, probably fine) and (b) we'd need to context-switch > > > TCR_EL1, with a small performance penalty (I don't think it's > > > significant but worth testing). > > > > So TLBI all of the CPUs on prctl() and context-switch TCR_EL1? I > > thought there would be DOS concerns with the first part of that? > > The DoS problem appears if we need to issue an IPI to all CPUs (like > stop_machine). The TLBI with broadcast handled in hardware should be OK > as it's targeted to a specific ASID. But this would have to be issued I see -- I hadn't realised that this instruction is implemented as a broadcast. So we would just need to issue the instruction from any CPU and we should be good. > before any app threads are started, otherwise we'd need to synchronise > TCR_EL1. Given that TBID0 toggling affects PAC, this can only be done Right, so this would be different from everything currently in tagged_addr_ctrl because it would be per-process rather than per-thread. So if this were a true TBID0 control we may even want it as a separate prctl() since there are certainly use cases for changing the other bits of tagged_addr_ctrl while the other threads are running. > safely very early in the application before return addresses get a PAC > code. Right, so maybe the "almost-disablement" would work better since all of the signatures would then still be valid, and you could even have different settings for different threads if you wanted to (e.g. if you arranged to run legacy code only on a specific thread). Peter
On Wed, Jul 28, 2021 at 04:50:07PM -0700, Peter Collingbourne wrote: > On Wed, Jul 28, 2021 at 9:42 AM Catalin Marinas <catalin.marinas@arm.com> wrote: > > On Tue, Jul 27, 2021 at 03:00:10PM -0700, Peter Collingbourne wrote: > > > On Tue, Jul 27, 2021 at 9:51 AM Catalin Marinas <catalin.marinas@arm.com> wrote: > > > > On Mon, Jun 21, 2021 at 10:12:04PM -0700, Peter Collingbourne wrote: > > > > > Introduce a command line flag that controls whether TCR_EL1.TBID0 > > > > > is set at boot time. Since this is a change to the userspace ABI the > > > > > option defaults to off for now, although it seems likely that we'll > > > > > be able to change the default at some future point. > > > > > > > > > > Setting TCR_EL1.TBID0 increases the number of signature bits used by > > > > > the pointer authentication instructions for instruction addresses by 8, > > > > > which improves the security of pointer authentication, but it also has > > > > > the consequence of changing the operation of the branch instructions > > > > > so that they no longer ignore the top byte of the target address but > > > > > instead fault if they are non-zero. > > > > > > > > I'm a bit uneasy about the ABI change and not so keen on constraining > > > > the ABI through the kernel command line. Ideally we should make this an > > > > opt-in per application (prctl()) but that has some aspects to address > > > > > > This doesn't necessarily need to be the end state, we can enhance this > > > based on need. For example, we could choose to take this patch now and > > > later implement the per-process opt-in where the default is controlled > > > by the command line. > > > > What's the risk of an application becoming reliant on the new mode > > (TBID0) and breaking with the old one? Probably very small but I haven't > > figured out if it's zero. Depending on whether we have PAC or PAC2 (the > > latter came with 8.6 but optional in 8.3) with TBID0, there are some > > differences on how the PAC/AUT instructions work and the code generated > > (XOR with the top bits). > > I think it would be quite small. On Android, at least to begin with > there would be a mixture of devices with different TBID0 settings > (devices without PAC support and devices with older kernels would all > have this disabled), so I think it would be difficult for an > application to depend on it being enabled. > > > > Or just implement the software-only per-process > > > TBID0 almost-disablement which would be much simpler than doing it in > > > hardware, if that is enough to satisfy future needs. > > > > I don't entirely follow this. > > Sorry, I was referring to my earlier proposal for recovering from > tagged PC in the kernel by clearing the tag bits: > https://lore.kernel.org/linux-arm-kernel/CAMn1gO7+_JhTUw2gS6jnyRV+TCqprmpuCAfee3RCAhNjoVyy9w@mail.gmail.com/ Ah, sorry, I missed this one (or just paged it out entirely over the holiday). > > > Otherwise we risk adding "unused" complexity to the kernel that we can > > > never remove due to API stability guarantees. > > > > We've had other debates over the years and, in general, if a kernel > > change causes apps to break, we'd have to keep the original behaviour. > > Are there any plans to fix the JITs tools you discovered? > > Yes, we would definitely want to fix the JIT issue in the Android > platform before rolling out a forward PAC ABI. This would be separate > from fixing apps, which would need to opt into MTE (or address tagging > via the target API level) anyway. But if it turns out that there are > too many apps with these JITs that use MTE or address tagging, I think > we would need to come back to the kernel to figure out some way to let > these programs run. OK, I guess we don't yet have a clear view on how many such apps are affected. > > Talking to Will about this he was wondering whether we could make TBID0 > > on by default and clear the tag in PC if we take a fault (on tagged PC), > > restarting the context. PAC shouldn't be affected since we would only > > branch to an authenticated (PAC code removed) pointer. If this works, > > we'd only affect performance slightly on such apps but don't completely > > break them. > > Right, this sounds exactly like my earlier proposal. Indeed. Something I haven't figured out yet is whether such handling in the kernel would weaken PAC. For example, following a failed AUT (the old style which does not trap), the resulting address would cause a translation fault. Would the kernel clearing the top byte result in a potentially valid address? > > > > first: (a) this bit is permitted to be cached in the TLB so we'd need > > > > some TLBI when setting it (and a clarification in the specs that it is > > > > tagged by ASID/VMID, probably fine) and (b) we'd need to context-switch > > > > TCR_EL1, with a small performance penalty (I don't think it's > > > > significant but worth testing). > > > > > > So TLBI all of the CPUs on prctl() and context-switch TCR_EL1? I > > > thought there would be DOS concerns with the first part of that? > > > > The DoS problem appears if we need to issue an IPI to all CPUs (like > > stop_machine). The TLBI with broadcast handled in hardware should be OK > > as it's targeted to a specific ASID. But this would have to be issued > > I see -- I hadn't realised that this instruction is implemented as a > broadcast. So we would just need to issue the instruction from any CPU > and we should be good. Well, as long as it's single-threaded when the prctl() is issued. If there are multiple threads, we'd have to synchronise all the CPUs. Such requirement is probably not a big deal anyway as it affects the return addresses, so it would have to be done early. > > before any app threads are started, otherwise we'd need to synchronise > > TCR_EL1. Given that TBID0 toggling affects PAC, this can only be done > > Right, so this would be different from everything currently in > tagged_addr_ctrl because it would be per-process rather than > per-thread. So if this were a true TBID0 control we may even want it > as a separate prctl() since there are certainly use cases for changing > the other bits of tagged_addr_ctrl while the other threads are > running. Since it's permitted to be cached in the TLB, switching it between threads would require TLBI on each switch, so not feasible. How early is the prctl(PR_TAGGED_ADDR_ENABLE) called? Anyway, it probably makes more sense to have a separate control since TBID0 is not necessarily linked to hwasan or MTE. We'd want more PAC bits even if hwasan or MTE are not deployed. > > safely very early in the application before return addresses get a PAC > > code. > > Right, so maybe the "almost-disablement" would work better since all > of the signatures would then still be valid, and you could even have > different settings for different threads if you wanted to (e.g. if you > arranged to run legacy code only on a specific thread). Yes, if (a) it doesn't weaken PAC and (b) there are no future plans to use code TBI as trapping would make it slower. I think we need to decide whether full TBI would be any useful going forward. One comment from Szabolcs on the previous version was that "data only tbi is a more complicated concept than plain tbi". Are there any tooling plans to benefit from code TBI? Or any use-cases that would be affected? For example, if we decide mmap() to return tagged pointers (with a new flag), would JITs want to benefit? If we need both data and code TBI to coexist, we should look into a per-process control.
On Thu, Jul 29, 2021 at 10:44 AM Catalin Marinas <catalin.marinas@arm.com> wrote: > > On Wed, Jul 28, 2021 at 04:50:07PM -0700, Peter Collingbourne wrote: > > On Wed, Jul 28, 2021 at 9:42 AM Catalin Marinas <catalin.marinas@arm.com> wrote: > > > On Tue, Jul 27, 2021 at 03:00:10PM -0700, Peter Collingbourne wrote: > > > > On Tue, Jul 27, 2021 at 9:51 AM Catalin Marinas <catalin.marinas@arm.com> wrote: > > > > > On Mon, Jun 21, 2021 at 10:12:04PM -0700, Peter Collingbourne wrote: > > > > > > Introduce a command line flag that controls whether TCR_EL1.TBID0 > > > > > > is set at boot time. Since this is a change to the userspace ABI the > > > > > > option defaults to off for now, although it seems likely that we'll > > > > > > be able to change the default at some future point. > > > > > > > > > > > > Setting TCR_EL1.TBID0 increases the number of signature bits used by > > > > > > the pointer authentication instructions for instruction addresses by 8, > > > > > > which improves the security of pointer authentication, but it also has > > > > > > the consequence of changing the operation of the branch instructions > > > > > > so that they no longer ignore the top byte of the target address but > > > > > > instead fault if they are non-zero. > > > > > > > > > > I'm a bit uneasy about the ABI change and not so keen on constraining > > > > > the ABI through the kernel command line. Ideally we should make this an > > > > > opt-in per application (prctl()) but that has some aspects to address > > > > > > > > This doesn't necessarily need to be the end state, we can enhance this > > > > based on need. For example, we could choose to take this patch now and > > > > later implement the per-process opt-in where the default is controlled > > > > by the command line. > > > > > > What's the risk of an application becoming reliant on the new mode > > > (TBID0) and breaking with the old one? Probably very small but I haven't > > > figured out if it's zero. Depending on whether we have PAC or PAC2 (the > > > latter came with 8.6 but optional in 8.3) with TBID0, there are some > > > differences on how the PAC/AUT instructions work and the code generated > > > (XOR with the top bits). > > > > I think it would be quite small. On Android, at least to begin with > > there would be a mixture of devices with different TBID0 settings > > (devices without PAC support and devices with older kernels would all > > have this disabled), so I think it would be difficult for an > > application to depend on it being enabled. > > > > > > Or just implement the software-only per-process > > > > TBID0 almost-disablement which would be much simpler than doing it in > > > > hardware, if that is enough to satisfy future needs. > > > > > > I don't entirely follow this. > > > > Sorry, I was referring to my earlier proposal for recovering from > > tagged PC in the kernel by clearing the tag bits: > > https://lore.kernel.org/linux-arm-kernel/CAMn1gO7+_JhTUw2gS6jnyRV+TCqprmpuCAfee3RCAhNjoVyy9w@mail.gmail.com/ > > Ah, sorry, I missed this one (or just paged it out entirely over the > holiday). > > > > > Otherwise we risk adding "unused" complexity to the kernel that we can > > > > never remove due to API stability guarantees. > > > > > > We've had other debates over the years and, in general, if a kernel > > > change causes apps to break, we'd have to keep the original behaviour. > > > Are there any plans to fix the JITs tools you discovered? > > > > Yes, we would definitely want to fix the JIT issue in the Android > > platform before rolling out a forward PAC ABI. This would be separate > > from fixing apps, which would need to opt into MTE (or address tagging > > via the target API level) anyway. But if it turns out that there are > > too many apps with these JITs that use MTE or address tagging, I think > > we would need to come back to the kernel to figure out some way to let > > these programs run. > > OK, I guess we don't yet have a clear view on how many such apps are > affected. > > > > Talking to Will about this he was wondering whether we could make TBID0 > > > on by default and clear the tag in PC if we take a fault (on tagged PC), > > > restarting the context. PAC shouldn't be affected since we would only > > > branch to an authenticated (PAC code removed) pointer. If this works, > > > we'd only affect performance slightly on such apps but don't completely > > > break them. > > > > Right, this sounds exactly like my earlier proposal. > > Indeed. Something I haven't figured out yet is whether such handling in > the kernel would weaken PAC. For example, following a failed AUT (the > old style which does not trap), the resulting address would cause a > translation fault. Would the kernel clearing the top byte result in a > potentially valid address? Yes, as I mentioned in my earlier message, without enhanced PAC2 the error code will go to bit 61 or 62, and those would be cleared by the kernel in the almost-disablement mode. So instruction PAC would be ineffective on CPUs without that feature. With enhanced PAC2 and without FPAC the computed PAC will be XORed with the pointer, so you still have a chance of faulting on a corrupted pointer (if the PAC bits below 55 do not match) -- basically the same chance as if TBID0 were disabled. > > > > > first: (a) this bit is permitted to be cached in the TLB so we'd need > > > > > some TLBI when setting it (and a clarification in the specs that it is > > > > > tagged by ASID/VMID, probably fine) and (b) we'd need to context-switch > > > > > TCR_EL1, with a small performance penalty (I don't think it's > > > > > significant but worth testing). > > > > > > > > So TLBI all of the CPUs on prctl() and context-switch TCR_EL1? I > > > > thought there would be DOS concerns with the first part of that? > > > > > > The DoS problem appears if we need to issue an IPI to all CPUs (like > > > stop_machine). The TLBI with broadcast handled in hardware should be OK > > > as it's targeted to a specific ASID. But this would have to be issued > > > > I see -- I hadn't realised that this instruction is implemented as a > > broadcast. So we would just need to issue the instruction from any CPU > > and we should be good. > > Well, as long as it's single-threaded when the prctl() is issued. If > there are multiple threads, we'd have to synchronise all the CPUs. Such > requirement is probably not a big deal anyway as it affects the return > addresses, so it would have to be done early. > > > > before any app threads are started, otherwise we'd need to synchronise > > > TCR_EL1. Given that TBID0 toggling affects PAC, this can only be done > > > > Right, so this would be different from everything currently in > > tagged_addr_ctrl because it would be per-process rather than > > per-thread. So if this were a true TBID0 control we may even want it > > as a separate prctl() since there are certainly use cases for changing > > the other bits of tagged_addr_ctrl while the other threads are > > running. > > Since it's permitted to be cached in the TLB, switching it between > threads would require TLBI on each switch, so not feasible. > > How early is the prctl(PR_TAGGED_ADDR_ENABLE) called? Anyway, it > probably makes more sense to have a separate control since TBID0 is not > necessarily linked to hwasan or MTE. We'd want more PAC bits even if > hwasan or MTE are not deployed. The prctl is called before any threads are created, so it would be feasible to turn on TBID0 there as well. Agreed about the separate control. > > > safely very early in the application before return addresses get a PAC > > > code. > > > > Right, so maybe the "almost-disablement" would work better since all > > of the signatures would then still be valid, and you could even have > > different settings for different threads if you wanted to (e.g. if you > > arranged to run legacy code only on a specific thread). > > Yes, if (a) it doesn't weaken PAC and (b) there are no future plans to > use code TBI as trapping would make it slower. As mentioned above it does weaken it, but only on certain CPUs. According to their technical reference manuals, Cortex-A78C implements enhanced PAC2, and Cortex-A710 implements both that and FPAC. So the only publicly known CPU where the almost-disablement would also weaken PAC is the Apple M1, which doesn't implement FPAC or enhanced PAC2. I wouldn't put much weight on the fact that PAC is weakened though anyway -- this mode would just be a way to get legacy applications running even if slower/less secure. > I think we need to decide whether full TBI would be any useful going > forward. One comment from Szabolcs on the previous version was that > "data only tbi is a more complicated concept than plain tbi". Are there > any tooling plans to benefit from code TBI? Or any use-cases that would > be affected? For example, if we decide mmap() to return tagged pointers > (with a new flag), would JITs want to benefit? At least on the Android side we have no plans to use those bits, not even for debugging tools like HWASan. Indeed we would much rather put those bits to use for PAC. I suppose that it's possible that JITs may want to use them, but that's not something that we would want to support on Android. And if such a JIT is portable to Apple platforms it would need an alternative code path to avoid using the bits anyway due to TBID0 being enabled on those platforms. > If we need both data and code TBI to coexist, we should look into a > per-process control. It doesn't seem necessary at this point IMHO. So can we start with the patch that makes this a boot-time control for now? Peter
diff --git a/arch/arm64/include/asm/compiler.h b/arch/arm64/include/asm/compiler.h index 6fb2e6bcc392..3c2c7a1a2abf 100644 --- a/arch/arm64/include/asm/compiler.h +++ b/arch/arm64/include/asm/compiler.h @@ -8,19 +8,26 @@ #define ARM64_ASM_PREAMBLE #endif +/* Open-code TCR_TBID0 value to avoid circular dependency. */ +#define tcr_tbid0_enabled() (init_tcr & (1UL << 51)) + /* * The EL0/EL1 pointer bits used by a pointer authentication code. * This is dependent on TBI0/TBI1 being enabled, or bits 63:56 would also apply. */ -#define ptrauth_user_pac_mask() GENMASK_ULL(54, vabits_actual) +#define ptrauth_user_insn_pac_mask() \ + (tcr_tbid0_enabled() ? GENMASK_ULL(63, vabits_actual) : \ + GENMASK_ULL(54, vabits_actual)) +#define ptrauth_user_data_pac_mask() GENMASK_ULL(54, vabits_actual) #define ptrauth_kernel_pac_mask() GENMASK_ULL(63, vabits_actual) /* Valid for EL0 TTBR0 and EL1 TTBR1 instruction pointers */ -#define ptrauth_clear_pac(ptr) \ - ((ptr & BIT_ULL(55)) ? (ptr | ptrauth_kernel_pac_mask()) : \ - (ptr & ~ptrauth_user_pac_mask())) +#define ptrauth_clear_insn_pac(ptr) \ + ((ptr & BIT_ULL(55)) ? (ptr | ptrauth_kernel_pac_mask()) : \ + (ptr & ~ptrauth_user_insn_pac_mask())) -#define __builtin_return_address(val) \ - (void *)(ptrauth_clear_pac((unsigned long)__builtin_return_address(val))) +#define __builtin_return_address(val) \ + ((void *)(ptrauth_clear_insn_pac( \ + (unsigned long)__builtin_return_address(val)))) #endif /* __ASM_COMPILER_H */ diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h index 87b90dc27a43..e0d8b8443ca6 100644 --- a/arch/arm64/include/asm/memory.h +++ b/arch/arm64/include/asm/memory.h @@ -191,6 +191,8 @@ extern u64 kimage_vaddr; /* the offset between the kernel virtual and physical mappings */ extern u64 kimage_voffset; +extern u64 init_tcr; + static inline unsigned long kaslr_offset(void) { return kimage_vaddr - KIMAGE_VADDR; diff --git a/arch/arm64/include/asm/pgtable-hwdef.h b/arch/arm64/include/asm/pgtable-hwdef.h index b82575a33f8b..31fc7c4b75d4 100644 --- a/arch/arm64/include/asm/pgtable-hwdef.h +++ b/arch/arm64/include/asm/pgtable-hwdef.h @@ -275,6 +275,7 @@ #define TCR_TBI1 (UL(1) << 38) #define TCR_HA (UL(1) << 39) #define TCR_HD (UL(1) << 40) +#define TCR_TBID0 (UL(1) << 51) #define TCR_TBID1 (UL(1) << 52) #define TCR_NFD0 (UL(1) << 53) #define TCR_NFD1 (UL(1) << 54) diff --git a/arch/arm64/include/asm/pointer_auth.h b/arch/arm64/include/asm/pointer_auth.h index d50416be99be..1bb1b022e5ee 100644 --- a/arch/arm64/include/asm/pointer_auth.h +++ b/arch/arm64/include/asm/pointer_auth.h @@ -92,7 +92,7 @@ extern int ptrauth_get_enabled_keys(struct task_struct *tsk); static inline unsigned long ptrauth_strip_insn_pac(unsigned long ptr) { - return ptrauth_clear_pac(ptr); + return ptrauth_clear_insn_pac(ptr); } static __always_inline void ptrauth_enable(void) diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h index 9df3feeee890..b2a575359a9c 100644 --- a/arch/arm64/include/asm/processor.h +++ b/arch/arm64/include/asm/processor.h @@ -253,6 +253,8 @@ unsigned long get_wchan(struct task_struct *p); void set_task_sctlr_el1(u64 sctlr); +void enable_tcr(u64 tcr); + /* Thread switching */ extern struct task_struct *cpu_switch_to(struct task_struct *prev, struct task_struct *next); diff --git a/arch/arm64/kernel/pointer_auth.c b/arch/arm64/kernel/pointer_auth.c index 60901ab0a7fe..9ac2fc2b4e46 100644 --- a/arch/arm64/kernel/pointer_auth.c +++ b/arch/arm64/kernel/pointer_auth.c @@ -109,3 +109,15 @@ int ptrauth_get_enabled_keys(struct task_struct *tsk) return retval; } + +static int __init tbi_data(char *arg) +{ + bool tbi_data; + + if (kstrtobool(arg, &tbi_data)) + return -EINVAL; + if (tbi_data) + enable_tcr(TCR_TBID0); + return 0; +} +early_param("tbi_data", tbi_data); diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c index b4bb67f17a2c..d7a2f6cb833e 100644 --- a/arch/arm64/kernel/process.c +++ b/arch/arm64/kernel/process.c @@ -644,6 +644,49 @@ void arch_setup_new_exec(void) } } +#ifdef CONFIG_ARM64_64K_PAGES +#define TCR_TG_FLAGS (TCR_TG0_64K | TCR_TG1_64K) +#elif defined(CONFIG_ARM64_16K_PAGES) +#define TCR_TG_FLAGS (TCR_TG0_16K | TCR_TG1_16K) +#else /* CONFIG_ARM64_4K_PAGES */ +#define TCR_TG_FLAGS (TCR_TG0_4K | TCR_TG1_4K) +#endif + +#ifdef CONFIG_RANDOMIZE_BASE +#define TCR_KASLR_FLAGS TCR_NFD1 +#else +#define TCR_KASLR_FLAGS 0 +#endif + +#define TCR_SMP_FLAGS TCR_SHARED + +/* PTWs cacheable, inner/outer WBWA */ +#define TCR_CACHE_FLAGS (TCR_IRGN_WBWA | TCR_ORGN_WBWA) + +#ifdef CONFIG_KASAN_SW_TAGS +#define TCR_KASAN_SW_FLAGS (TCR_TBI1 | TCR_TBID1) +#else +#define TCR_KASAN_SW_FLAGS 0 +#endif + +u64 __section(".mmuoff.data.read") init_tcr = + TCR_TxSZ(VA_BITS) | TCR_CACHE_FLAGS | TCR_SMP_FLAGS | TCR_TG_FLAGS + | TCR_KASLR_FLAGS | TCR_ASID16 | TCR_TBI0 | TCR_A1 | TCR_KASAN_SW_FLAGS; +EXPORT_SYMBOL(init_tcr); + +void __init enable_tcr(u64 tcr) +{ + u64 tmp; + + init_tcr |= tcr; + __asm__ __volatile__( + "mrs %0, tcr_el1\n" + "orr %0, %0, %1\n" + "msr tcr_el1, %0\n" + "tlbi vmalle1\n" + : "=&r"(tmp) : "r"(tcr)); +} + #ifdef CONFIG_ARM64_TAGGED_ADDR_ABI /* * Control the relaxed ABI allowing tagged user addresses into the kernel. diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c index eb2f73939b7b..4d86870ed348 100644 --- a/arch/arm64/kernel/ptrace.c +++ b/arch/arm64/kernel/ptrace.c @@ -894,13 +894,11 @@ static int pac_mask_get(struct task_struct *target, { /* * The PAC bits can differ across data and instruction pointers - * depending on TCR_EL1.TBID*, which we may make use of in future, so - * we expose separate masks. + * depending on TCR_EL1.TBID0, so we expose separate masks. */ - unsigned long mask = ptrauth_user_pac_mask(); struct user_pac_mask uregs = { - .data_mask = mask, - .insn_mask = mask, + .data_mask = ptrauth_user_data_pac_mask(), + .insn_mask = ptrauth_user_insn_pac_mask(), }; if (!system_supports_address_auth()) diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c index 871c82ab0a30..9ee32afe121c 100644 --- a/arch/arm64/mm/fault.c +++ b/arch/arm64/mm/fault.c @@ -529,11 +529,23 @@ static int __kprobes do_page_fault(unsigned long far, unsigned int esr, vm_fault_t fault; unsigned long vm_flags; unsigned int mm_flags = FAULT_FLAG_DEFAULT; - unsigned long addr = untagged_addr(far); + unsigned long addr; if (kprobe_page_fault(regs, esr)) return 0; + /* + * If TBID0 is set then we may get an IABT with a tagged address here as + * a result of branching to a tagged address. In this case we want to + * avoid untagging the address, let the VMA lookup fail and get a + * SIGSEGV. Leaving the address as is will also work if TBID0 is clear + * or unsupported because the tag bits of FAR_EL1 will be clear. + */ + if (is_el0_instruction_abort(esr)) + addr = far; + else + addr = untagged_addr(far); + /* * If we're in an interrupt or have no user context, we must not take * the fault. diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S index 97d7bcd8d4f2..bae9476e6c2a 100644 --- a/arch/arm64/mm/proc.S +++ b/arch/arm64/mm/proc.S @@ -20,31 +20,6 @@ #include <asm/smp.h> #include <asm/sysreg.h> -#ifdef CONFIG_ARM64_64K_PAGES -#define TCR_TG_FLAGS TCR_TG0_64K | TCR_TG1_64K -#elif defined(CONFIG_ARM64_16K_PAGES) -#define TCR_TG_FLAGS TCR_TG0_16K | TCR_TG1_16K -#else /* CONFIG_ARM64_4K_PAGES */ -#define TCR_TG_FLAGS TCR_TG0_4K | TCR_TG1_4K -#endif - -#ifdef CONFIG_RANDOMIZE_BASE -#define TCR_KASLR_FLAGS TCR_NFD1 -#else -#define TCR_KASLR_FLAGS 0 -#endif - -#define TCR_SMP_FLAGS TCR_SHARED - -/* PTWs cacheable, inner/outer WBWA */ -#define TCR_CACHE_FLAGS TCR_IRGN_WBWA | TCR_ORGN_WBWA - -#ifdef CONFIG_KASAN_SW_TAGS -#define TCR_KASAN_SW_FLAGS TCR_TBI1 | TCR_TBID1 -#else -#define TCR_KASAN_SW_FLAGS 0 -#endif - #ifdef CONFIG_KASAN_HW_TAGS #define TCR_KASAN_HW_FLAGS SYS_TCR_EL1_TCMA1 | TCR_TBI1 | TCR_TBID1 #else @@ -425,9 +400,7 @@ SYM_FUNC_START(__cpu_setup) mair .req x17 tcr .req x16 mov_q mair, MAIR_EL1_SET - mov_q tcr, TCR_TxSZ(VA_BITS) | TCR_CACHE_FLAGS | TCR_SMP_FLAGS | \ - TCR_TG_FLAGS | TCR_KASLR_FLAGS | TCR_ASID16 | \ - TCR_TBI0 | TCR_A1 | TCR_KASAN_SW_FLAGS + ldr_l tcr, init_tcr #ifdef CONFIG_ARM64_MTE /*
Introduce a command line flag that controls whether TCR_EL1.TBID0 is set at boot time. Since this is a change to the userspace ABI the option defaults to off for now, although it seems likely that we'll be able to change the default at some future point. Setting TCR_EL1.TBID0 increases the number of signature bits used by the pointer authentication instructions for instruction addresses by 8, which improves the security of pointer authentication, but it also has the consequence of changing the operation of the branch instructions so that they no longer ignore the top byte of the target address but instead fault if they are non-zero. Signed-off-by: Peter Collingbourne <pcc@google.com> Link: https://linux-review.googlesource.com/id/Ife724ad708142bc475f42e8c1d9609124994bbbd --- v2: - rebase to linux-next - make it a command line flag arch/arm64/include/asm/compiler.h | 19 ++++++++---- arch/arm64/include/asm/memory.h | 2 ++ arch/arm64/include/asm/pgtable-hwdef.h | 1 + arch/arm64/include/asm/pointer_auth.h | 2 +- arch/arm64/include/asm/processor.h | 2 ++ arch/arm64/kernel/pointer_auth.c | 12 +++++++ arch/arm64/kernel/process.c | 43 ++++++++++++++++++++++++++ arch/arm64/kernel/ptrace.c | 8 ++--- arch/arm64/mm/fault.c | 14 ++++++++- arch/arm64/mm/proc.S | 29 +---------------- 10 files changed, 91 insertions(+), 41 deletions(-)