Re: [PATCH v3 7/7] x86/ldt,ptrace: provide regset access to the LDT

2018-06-22 Thread Andy Lutomirski 
On Fri, Jun 22, 2018 at 8:06 AM  wrote:
>
> On June 22, 2018 7:49:13 AM PDT, Andy Lutomirski  wrote:
> >On Thu, Jun 21, 2018 at 2:18 PM H. Peter Anvin, Intel
> > wrote:
> >>
> >> From: "H. Peter Anvin" 
> >>
> >> Provide ptrace/regset access to the LDT, if one exists.  This
> >> interface provides both read and write access. The write code is
> >> unified with modify_ldt(); the read code doesn't have enough
> >> similarity so it has been kept made separate.
> >
> >For this and for the GDT, you've chosen to use struct user_desc as
> >your format instead of using a native hardware descriptor format.  Any
> >particular reason why?  If nothing else, it will bloat core files a
> >bit more than needed.
>
> I did because REGSET_TLS was implemented that way, and that is simply a 
> subset of the GDT (which made the same code trivially applicable to both.) 
> modify_ldt() does it *both* ways for extra fun (one for reading, and one for 
> writing.)
>
> ->active is defined as "beyond this point the regset contains only the 
> default value", which seemed appropriate in this case.

I saw that definition too.  But I'm still very unclear as to what, if
anything, the code actually does :)

--Andy

Re: [PATCH v3 7/7] x86/ldt,ptrace: provide regset access to the LDT

2018-06-22 Thread hpa 
On June 22, 2018 7:49:13 AM PDT, Andy Lutomirski  wrote:
>On Thu, Jun 21, 2018 at 2:18 PM H. Peter Anvin, Intel
> wrote:
>>
>> From: "H. Peter Anvin" 
>>
>> Provide ptrace/regset access to the LDT, if one exists.  This
>> interface provides both read and write access. The write code is
>> unified with modify_ldt(); the read code doesn't have enough
>> similarity so it has been kept made separate.
>
>For this and for the GDT, you've chosen to use struct user_desc as
>your format instead of using a native hardware descriptor format.  Any
>particular reason why?  If nothing else, it will bloat core files a
>bit more than needed.

I did because REGSET_TLS was implemented that way, and that is simply a subset 
of the GDT (which made the same code trivially applicable to both.) 
modify_ldt() does it *both* ways for extra fun (one for reading, and one for 
writing.)

->active is defined as "beyond this point the regset contains only the default 
value", which seemed appropriate in this case.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH v3 7/7] x86/ldt,ptrace: provide regset access to the LDT

2018-06-22 Thread Andy Lutomirski 
On Thu, Jun 21, 2018 at 2:18 PM H. Peter Anvin, Intel
 wrote:
>
> From: "H. Peter Anvin" 
>
> Provide ptrace/regset access to the LDT, if one exists.  This
> interface provides both read and write access. The write code is
> unified with modify_ldt(); the read code doesn't have enough
> similarity so it has been kept made separate.

For this and for the GDT, you've chosen to use struct user_desc as
your format instead of using a native hardware descriptor format.  Any
particular reason why?  If nothing else, it will bloat core files a
bit more than needed.

[PATCH v3 7/7] x86/ldt,ptrace: provide regset access to the LDT

2018-06-21 Thread H. Peter Anvin, Intel 
From: "H. Peter Anvin" 

Provide ptrace/regset access to the LDT, if one exists.  This
interface provides both read and write access. The write code is
unified with modify_ldt(); the read code doesn't have enough
similarity so it has been kept made separate.

Signed-off-by: H. Peter Anvin (Intel) 
Cc: Ingo Molnar 
Cc: Thomas Gleixner 
Cc: Andy Lutomirski 
Cc: Chang S. Bae 
Cc: Markus T. Metzger 
---
 arch/x86/include/asm/desc.h |   2 +-
 arch/x86/include/asm/ldt.h  |  16 
 arch/x86/kernel/ldt.c   | 209 +++-
 arch/x86/kernel/ptrace.c|  20 +
 include/uapi/linux/elf.h|   1 +
 5 files changed, 204 insertions(+), 44 deletions(-)
 create mode 100644 arch/x86/include/asm/ldt.h

diff --git a/arch/x86/include/asm/desc.h b/arch/x86/include/asm/desc.h
index 5e69f993c9ff..12a759d76314 100644
--- a/arch/x86/include/asm/desc.h
+++ b/arch/x86/include/asm/desc.h
@@ -3,7 +3,7 @@
 #define _ASM_X86_DESC_H
 
 #include 
-#include 
+#include 
 #include 
 #include 
 #include 
diff --git a/arch/x86/include/asm/ldt.h b/arch/x86/include/asm/ldt.h
new file mode 100644
index ..302ec2d6d45d
--- /dev/null
+++ b/arch/x86/include/asm/ldt.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Ptrace interface to the LDT
+ *
+ */
+
+#ifndef _ARCH_X86_INCLUDE_ASM_LDT_H
+
+#include 
+#include 
+
+extern user_regset_active_fn regset_ldt_active;
+extern user_regset_get_fn regset_ldt_get;
+extern user_regset_set_fn regset_ldt_set;
+
+#endif /* _ARCH_X86_INCLUDE_ASM_LDT_H */
diff --git a/arch/x86/kernel/ldt.c b/arch/x86/kernel/ldt.c
index 601d24268a99..e80dfde1f82f 100644
--- a/arch/x86/kernel/ldt.c
+++ b/arch/x86/kernel/ldt.c
@@ -392,53 +392,39 @@ static int read_default_ldt(void __user *ptr, unsigned 
long bytecount)
return bytecount;
 }
 
-static int write_ldt(void __user *ptr, unsigned long bytecount, int oldmode)
+
+static int do_write_ldt(const struct task_struct *target,
+   const void *kbuf, const void __user *ubuf,
+   unsigned long bytecount, unsigned int index,
+   bool oldmode)
 {
-   struct mm_struct *mm = current->mm;
+   struct mm_struct *mm = target->mm;
struct ldt_struct *new_ldt, *old_ldt;
-   unsigned int old_nr_entries, new_nr_entries;
+   unsigned int count, old_nr_entries, new_nr_entries;
struct user_desc ldt_info;
+   const struct user_desc *kptr = kbuf;
+   const struct user_desc __user *uptr = ubuf;
struct desc_struct ldt;
+   unsigned short first_index, last_index;
int error;
 
-   error = -EINVAL;
-   if (bytecount != sizeof(ldt_info))
-   goto out;
-   error = -EFAULT;
-   if (copy_from_user(&ldt_info, ptr, sizeof(ldt_info)))
-   goto out;
+   if (bytecount % sizeof(ldt_info) ||
+   bytecount >= LDT_ENTRY_SIZE*LDT_ENTRIES)
+   return -EINVAL;
 
-   error = -EINVAL;
-   if (ldt_info.entry_number >= LDT_ENTRIES)
-   goto out;
-   if (ldt_info.contents == 3) {
-   if (oldmode)
-   goto out;
-   if (ldt_info.seg_not_present == 0)
-   goto out;
-   }
+   count = bytecount/sizeof(ldt_info);
+   if (index >= LDT_ENTRIES || index + count > LDT_ENTRIES)
+   return -EINVAL;
 
-   if ((oldmode && !ldt_info.base_addr && !ldt_info.limit) ||
-   LDT_empty(&ldt_info)) {
-   /* The user wants to clear the entry. */
-   memset(&ldt, 0, sizeof(ldt));
-   } else {
-   if (!IS_ENABLED(CONFIG_X86_16BIT) && !ldt_info.seg_32bit) {
-   error = -EINVAL;
-   goto out;
-   }
-
-   fill_ldt(&ldt, &ldt_info);
-   if (oldmode)
-   ldt.avl = 0;
-   }
+   first_index = index;
+   last_index = index + count - 1;
 
if (down_write_killable(&mm->context.ldt_usr_sem))
return -EINTR;
 
-   old_ldt   = mm->context.ldt;
+   old_ldt= mm->context.ldt;
old_nr_entries = old_ldt ? old_ldt->nr_entries : 0;
-   new_nr_entries = max(ldt_info.entry_number + 1, old_nr_entries);
+   new_nr_entries = max(index + count, old_nr_entries);
 
error = -ENOMEM;
new_ldt = alloc_ldt_struct(new_nr_entries);
@@ -446,9 +432,46 @@ static int write_ldt(void __user *ptr, unsigned long 
bytecount, int oldmode)
goto out_unlock;
 
if (old_ldt)
-   memcpy(new_ldt->entries, old_ldt->entries, old_nr_entries * 
LDT_ENTRY_SIZE);
+   memcpy(new_ldt->entries, old_ldt->entries,
+  old_nr_entries * LDT_ENTRY_SIZE);
+
+   while (count--) {
+   error = -EFAULT;
+   if (kptr) {
+   memcpy(&ldt_info, kptr++, sizeof(ldt_info));
+   } else {
+