Re: [PATCH v3 1/1] x86: allow to enable/disable modify_ldt at run time

[Willy Tarreau]

On Wed, Aug 05, 2015 at 10:26:16AM +0200, Ingo Molnar wrote:
> > +modify_ldt: (X86 only)
> > +
> > +Enables (1) or disables (0) the modify_ldt syscall. Modifying the LDT
> > +(Local Descriptor Table) may be needed to run a 16-bit or segmented code
> 
> s/run a/run

good catch, thanks.

> > +such as Dosemu or Wine. This is done via a system call which is not needed
> 
> s/Dosemu/DOSEMU

I didn't know. Fixed.

> > +to run portable applications, and which can sometimes be abused to exploit
> > +some weaknesses of the architecture, opening new vulnerabilities.
> 
> So that's pretty vague IMHO, and a bit FUD-ish in character. How about:
> 
>   ... , and which system call exposes complex, rarely used legacy hardware 
>   features and semantics that had suffered vulnerabilities in the past.

OK fine for me, fixed.

> > +	if (!sysctl_modify_ldt) {
> > +		printk_ratelimited(KERN_INFO
> > +			"Denied a call to modify_ldt() from %s[%d] (uid: %d)."
> > +			" Adjust the modify_ldt sysctl if this was not an"
> 
> Would it really be so difficult to write this as:
> 
>   Set "sys.kernel.modify_ldt = 1" in /etc/sysctl.conf if this was not an exploit attempt.

It's just a matter of taste. Normally I consider the kernel distro-agnostic
so I don't like to suggest one way to adjust sysctls nor to reference config
files. Here we're in a case where only standard distro users may hit the
issue, and users of embedded distros will not face this message or will
easily translate it into their respective configuration scheme. So OK for
this one.

> 99% of the users seeing this message will see it right after an app of theirs 
> ended up not working. Let's not add to the annoyance factor!

That's exactly why I wrote the message in the first place!

> > +			" exploit attempt.\n",
> > +			current->comm, task_pid_nr(current),
> > +			from_kuid_munged(current_user_ns(), current_uid()));
> 
> Also generally please don't break message lines in the source code while they are 
> a single line in the syslog, to make it easier to grep for and to expose kernel 
> hackers to the form of message they are emitting. Ignore checkpatch.

I'm fine with this rule as well, it's only in the kenrel that I tend to
care about the 80-col limit, in my own code I easily ignore it for the
same reason.

> > @@ -960,6 +963,17 @@ static struct ctl_table kern_table[] = {
> >  		.mode		= 0644,
> >  		.proc_handler	= proc_dointvec,
> >  	},
> > +#ifdef CONFIG_MODIFY_LDT_SYSCALL
> > +	{
> > +		.procname	= "modify_ldt",
> > +		.data		= &sysctl_modify_ldt,
> > +		.maxlen		= sizeof(int),
> > +		.mode		= 0644,
> > +		.proc_handler   = proc_dointvec_minmax,
> > +		.extra1         = &zero,
> > +		.extra2         = &one,
> > +	},
> > +#endif
> 
> So I'd actually make the permissions 0600: to make it a tiny bit harder for 
> exploits to silently query the current value to figure out whether they can safely 
> attempt the syscall or not ...

That's a good point. If we later make other syscalls configurable, it might
be different because some users might want to contact their admin to ask for
a specific one. But here, there's usually no admin so I'm fine with hardening
it.

> (Sadly /etc/sysctl.conf is world-readable on most distros.)

Yes, just like most executables are readable while not strictly needed,
especially setuid ones which allow the code to be studied in place or
easily duplicated to script some exploits. But we could discuss hours
about basic system hardening!

I've updated the patch with your suggestions.

Thanks,
Willy