Re: [PATCH v8 07/10] x86, mpx: decode MPX instruction to get bound violation information

[H. Peter Anvin]

On 09/12/2014 06:10 AM, Thomas Gleixner wrote:
>>
>> I'm not wedded to that concept, by the way, but using the generic parser had a
>> whole bunch of its own problems, including the fact that you're getting bytes
>> from user space.
> 
> Errm. The instruction decoder does not even know about user space.
> 
>       u8 buf[MAX_INSN_SIZE];
> 
>       memset(buf, 0, MAX_INSN_SIZE);
>       if (copy_from_user(buf, addr, MAX_INSN_SIZE))
>       	    return 0;
> 
>       insn_init(insn, buf, is_64bit(current));
> 
>       /* Process the entire instruction */
>       insn_get_length(insn);
> 
>       /* Decode the faulting address */
>       return mpx_get_addr(insn, regs);
> 
> I really can't see why that should not work. insn_get_length()
> retrieves exactly the information which is required to call
> mpx_get_addr().
> 
> Sure it might be a bit slower because the generic decoder does a bit
> more than the mpx private sauce, but this happens in the context of a
> bounds violation and it really does not matter at all whether SIGSEGV
> is delivered 5 microseconds later or not.
> 
> The only difference is the insn->limit handling in the MPX
> decoder. The existing decoder has a limit check of:
> 
> #define MAX_INSN_SIZE       16
> 
> and MPX private one makes that
> 
> #define MAX_MPX_INSN_SIZE   15
> 
> and limits it runtime further to:
> 
>     MAX_MPX_INSN_SIZE - bytes_not_copied_from_user_space;
> 
> This is beyond silly, really. If we cannot copy 16 bytes from user
> space, why bother in dealing with a partial copy at all.
> 

The correct limit is 15 bytes, not anything else, so this is a bug in
the existing decoder.  A sequence of bytes longer than 15 bytes will
#UD, regardless of being "otherwise valid".

Keep in mind the instruction may not be aligned, and you could fit an
instruction plus a jump and still overrun a page in 15 bytes.

> Aside of that the existing decoder handles the 32bit app on a 64bit
> kernel already correctly while the extra magic MPX decoder does
> not. It just adds some magically optimized and different copy of the
> existing decoder for exactly ZERO value.
> 
>> It might be worthwhile to compare the older patchset which did use the generic
>> parser to make sure that it actually made sense.
> 
> I can't find such a thing. The first version I found contains an even
> more convoluted private parser. Intelnal mail perhaps?

Yes, I suspect so.

	-hpa

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/