Re: [PATCH] x86/ia32: Do not modify the DPL bits for a null selector

[Eric W. Biederman]

"Li, Xin3" <xin3.li@xxxxxxxxx> writes:

>> Perhaps something like patch below to make it clear that we are
>> normalizing the segment values and forcing that normalization.
>
> "Normalizing" sounds a good term.
>
>> I am a bit confused why this code is not the same for ia32 and
>> ia32_emulation.  I would think the normalization at least should apply
>> to the 32bit case as well.
>> 
>> Eric
>> 
>> diff --git a/arch/x86/kernel/signal_32.c b/arch/x86/kernel/signal_32.c
>> index 9027fc088f97..e5f3978388fd 100644
>> --- a/arch/x86/kernel/signal_32.c
>> +++ b/arch/x86/kernel/signal_32.c
>> @@ -36,22 +36,47 @@
>>  #ifdef CONFIG_IA32_EMULATION
>>  #include <asm/ia32_unistd.h>
>> 
>> +static inline unsigned int normalize_seg_index(unsigned int index)
>
> normalize_usrseg_index?

Perhaps useg?  I think that is the abbreviation I have seen used
elsewhere.  I was trying to get things as close as I could to the
x86 documentation so people could figure out what is going on by
reading the documentation.

>> +{
>> +	/*
>> +	 * Convert the segment index into normalized form.
>> +	 *
>> +	 * For the indexes 0,1,2,3 always use the value of 0, as IRET
>> +	 * forces this form for the nul segment.
>> +	 *
>> +	 * Otherwise set both DPL bits to force it to be a userspace
>> +	 * ring 3 segment index.
>> +	 */
>> +	return (index < 3) ? 0 : index | 3;
>
> s/</<=
>
> no?

Yes.  My typo.

The patch was very much a suggestion on how we can perhaps write the
code to make it clearer what is happening.  Normalizing the segment
index values seems like what we have been intending to do all along.

In fact it might make sense for clarity to simply use the existing
"index | 3" logic in what I called normal_seg_index, and then just
update the normalization to add the null pointer case.

I was just spending a little time trying to make it so that the code
is readable.

> With this patch it looks that 1,2,3 are no longer valid selector values
> in Linux, which seems the right thing to do but I don't know if there is
> any side effect.

You have said that IRET always changes 1,2,3 to 0.  So I don't expect
the selector values of 1,2,3 will last very long.

If that is not the case I misunderstood, and we should consider doing
something else.

It bothers me that the existing code, and your code as well only
handles the normalization on x86_64 for ia32 mode.  Shouldn't
the same normalization logic apply in a 32bit kernel as well?
Scope creep I know but the fact the code does not match
seems concerning.

Eric


>> +}
>> +
>>  static inline void reload_segments(struct sigcontext_32 *sc)
>>  {
>> -	unsigned int cur;
>> +	unsigned int new, cur;
>> 
>> +	new = normalize_seg_index(sc->gs);
>>  	savesegment(gs, cur);
>> -	if ((sc->gs | 0x03) != cur)
>> -		load_gs_index(sc->gs | 0x03);
>> +	cur = normalize_seg_index(cur);
>> +	if (new != cur)
>> +		load_gs_index(new);
>> +
>> +	new = normalize_seg_index(sc->fs);
>>  	savesegment(fs, cur);
>> -	if ((sc->fs | 0x03) != cur)
>> -		loadsegment(fs, sc->fs | 0x03);
>> +	cur = normalize_seg_index(cur);
>> +	if (new != cur)
>> +		loadsegment(fs, new);
>> +
>> +	new = normalize_seg_index(sc->ds);
>>  	savesegment(ds, cur);
>> -	if ((sc->ds | 0x03) != cur)
>> -		loadsegment(ds, sc->ds | 0x03);
>> +	cur = normalize_seg_index(cur);
>> +	if (new != cur)
>> +		loadsegment(ds, new);
>> +
>> +	new = normalize_seg_index(sc->es);
>>  	savesegment(es, cur);
>> -	if ((sc->es | 0x03) != cur)
>> -		loadsegment(es, sc->es | 0x03);
>> +	cur = normalize_seg_index(cur);
>> +	if (new != cur)
>> +		loadsegment(es, new);
>>  }
>> 
>>  #define sigset32_t			compat_sigset_t