linux-kernel - Re: [PATCH] ARM: add get_user() support for 8 byte types

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAF6AEGvoXR3YJomjV1YOeNvjr46aYJ2mVxS+jZfH+41+v1nAvw@mail.gmail.com>
Date:	Mon, 12 Nov 2012 18:31:50 -0600
From:	Rob Clark <rob.clark@...aro.org>
To:	Russell King - ARM Linux <linux@....linux.org.uk>
Cc:	linux-arm-kernel@...ts.infradead.org, patches@...aro.org,
	linux-kernel@...r.kernel.org, linux-omap@...r.kernel.org,
	dri-devel@...ts.freedesktop.org, Arnd Bergmann <arnd@...db.de>
Subject: Re: [PATCH] ARM: add get_user() support for 8 byte types

On Mon, Nov 12, 2012 at 5:53 PM, Russell King - ARM Linux
<linux@....linux.org.uk> wrote:
> On Mon, Nov 12, 2012 at 05:33:41PM -0600, Rob Clark wrote:
>> I'm sort of thinking maybe we want to change 'switch (sizeof(*(__p)))'
>> with 'switch (sizeof(typeof(x)))' in case someone ignores the compiler
>> warning when they try something like:
>
> Definitely not.  Ttype of access is controlled by the pointer, not by the
> size of what it's being assigned to.  Switching that around is likely to
> break stuff hugely.  Consider this:
>
>         unsigned char __user *p;
>         int val;
>
>         get_user(val, p);
>
> If the pointer type is used to determine the access size, a char will be
> accessed.  This is legal - because we end up assigning an unsigned character
> to an int.
>
> If the size of the destination was used, we'd access an int instead, which
> is larger than the pointer, and probably the wrong thing to do anyway.
>
> Think of get_user(a, b) as being a special accessor having the ultimate
> semantics of: "a = *b;" but done in a safe way with error checking.
>
>>    uint32_t x;
>>    uint64_t *p = ...;
>>    get_user(x, p);
>>
>> that was my one concern about 'register typeof(x) __r2 ...', but I
>> think just changing the switch condition is enough.  But maybe good to
>> have some eyes on in case there is something else I'm not thinking of.
>
> And what should happen in the above is exactly the same as what happens
> if you do:
>
>         x = *p;
>
> with those types.  For ARM, that would be a 64-bit access (if the
> compiler decides not to optimize away the upper 32-bit access) followed
> by a narrowing cast down to 32-bit.  With get_user() of course, there's
> no option not to optimize it away.
>
> However, this _does_ reveal a bug with your approach.  With sizeof(*p)
> being 8, and the type of __r2 being a 32-bit quantity, the compiler will
> choose the 64-bit accessor, which will corrupt r3 - and the compiler
> won't know that r3 has been corrupted.

right, that is what I was worried about..  but what about something
along the lines of:

		case 8: {						\
			if (sizeof(x) < 8)				\
				__get_user_x(__r2, __p, __e, __l, 4);	\
			else						\
				__get_user_x(__r2, __p, __e, __l, 8);	\
			break;						\
		}							\

maybe we need a special variant of __get_user_8() instead to get the
right 32bits on big vs little endian systems, but I think something
roughly along these lines could work.

Or maybe in sizeof(x)<8 case, we just __get_user_bad()..  I'm not 100%
sure on whether this is supposed to be treated as an error case at
compile time or not.

BR,
-R

> That's too unsafe.
>
> I just tried a variant of your approach, but got lots of warnings again:
>         register unsigned long long __r2 asm("r2");
>         __get_user_x(__r2, __p, __e, 8, "lr");
>         x = (typeof(*(__p))) ((typeof(x))__r2);
> because typeof(x) in the test_ptr() case ends up being a pointer itself.
>
> So, back to the drawing board, and I think back to the original position
> of "we don't support this".
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/