The error path in write_ldt() frees the already installed LDT memory
instead of the newly allocated which cannot be installed.
Fixes: f55f0501cbf6 ("x86/pti: Put the LDT in its own PGD if PTI is on")
Reported-by: Mathieu Desnoyers <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
---
arch/x86/kernel/ldt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- a/arch/x86/kernel/ldt.c
+++ b/arch/x86/kernel/ldt.c
@@ -421,7 +421,7 @@ static int write_ldt(void __user *ptr, u
*/
error = map_ldt_struct(mm, new_ldt, old_ldt ? !old_ldt->slot : 0);
if (error) {
- free_ldt_struct(old_ldt);
+ free_ldt_struct(new_ldt);
goto out_unlock;
}
* Thomas Gleixner <[email protected]> wrote:
> The error path in write_ldt() frees the already installed LDT memory
> instead of the newly allocated which cannot be installed.
s/newly allocated
/newly allocated one
>
> Fixes: f55f0501cbf6 ("x86/pti: Put the LDT in its own PGD if PTI is on")
> Reported-by: Mathieu Desnoyers <[email protected]>
> Signed-off-by: Thomas Gleixner <[email protected]>
> ---
> arch/x86/kernel/ldt.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> --- a/arch/x86/kernel/ldt.c
> +++ b/arch/x86/kernel/ldt.c
> @@ -421,7 +421,7 @@ static int write_ldt(void __user *ptr, u
> */
> error = map_ldt_struct(mm, new_ldt, old_ldt ? !old_ldt->slot : 0);
> if (error) {
> - free_ldt_struct(old_ldt);
> + free_ldt_struct(new_ldt);
> goto out_unlock;
> }
>
This bug kind of scares me ...
Reviewed-by: Ingo Molnar <[email protected]>
Thanks,
Ingo
The error path in write_ldt() tries to free old_ldt instead of the newly
allocated new_ldt resulting in a memory leak. It also misses to clean up a
half populated LDT pagetable, which is not a leak as it gets cleaned up
when the process exits.
Free both the potentially half populated LDT pagetable and the newly
allocated LDT struct. This can be done unconditionally because once a LDT
is mapped subsequent maps will succeed because the PTE page is already
populated and the two LDTs fit into that single page.
Fixes: f55f0501cbf6 ("x86/pti: Put the LDT in its own PGD if PTI is on")
Reported-by: Mathieu Desnoyers <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
---
arch/x86/kernel/ldt.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
--- a/arch/x86/kernel/ldt.c
+++ b/arch/x86/kernel/ldt.c
@@ -421,7 +421,13 @@ static int write_ldt(void __user *ptr, u
*/
error = map_ldt_struct(mm, new_ldt, old_ldt ? !old_ldt->slot : 0);
if (error) {
- free_ldt_struct(old_ldt);
+ /*
+ * This only can fail for the first LDT setup. If a LDT is
+ * already installed then the PTE page is already
+ * populated. Mop up a half populated page table.
+ */
+ free_ldt_pgtables(mm);
+ free_ldt_struct(new_ldt);
goto out_unlock;
}
> On Dec 31, 2017, at 2:24 AM, Thomas Gleixner <[email protected]> wrote:
>
> The error path in write_ldt() tries to free old_ldt instead of the newly
> allocated new_ldt resulting in a memory leak. It also misses to clean up a
> half populated LDT pagetable, which is not a leak as it gets cleaned up
> when the process exits.
>
> Free both the potentially half populated LDT pagetable and the newly
> allocated LDT struct. This can be done unconditionally because once a LDT
> is mapped subsequent maps will succeed because the PTE page is already
> populated and the two LDTs fit into that single page.
>
> Fixes: f55f0501cbf6 ("x86/pti: Put the LDT in its own PGD if PTI is on")
> Reported-by: Mathieu Desnoyers <[email protected]>
> Signed-off-by: Thomas Gleixner <[email protected]>
> ---
> arch/x86/kernel/ldt.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> --- a/arch/x86/kernel/ldt.c
> +++ b/arch/x86/kernel/ldt.c
> @@ -421,7 +421,13 @@ static int write_ldt(void __user *ptr, u
> */
> error = map_ldt_struct(mm, new_ldt, old_ldt ? !old_ldt->slot : 0);
> if (error) {
> - free_ldt_struct(old_ldt);
> + /*
> + * This only can fail for the first LDT setup. If a LDT is
> + * already installed then the PTE page is already
> + * populated. Mop up a half populated page table.
> + */
I liked it better with the conditional. If this ever fails due to fault injection, some silly accounting issue, a paravirt glitch, or whatever, then we'll oops.
> + free_ldt_pgtables(mm);
> + free_ldt_struct(new_ldt);
> goto out_unlock;
> }
>