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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
