From mboxrd@z Thu Jan 1 00:00:00 1970 From: James Hogan Subject: Re: [PATCH 1/2] metag/uaccess: Fix access_ok() Date: Tue, 2 May 2017 22:26:05 +0100 Message-ID: <20170502212605.GO1105@jhogan-linux.le.imgtec.org> References: <8a8b56638bcac4e64cccc88bf95a0f9f4b19a2fb.1493759289.git-series.james.hogan@imgtec.com> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="nrXiCraHbXeog9mY" Return-path: Content-Disposition: inline In-Reply-To: <8a8b56638bcac4e64cccc88bf95a0f9f4b19a2fb.1493759289.git-series.james.hogan-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> Sender: linux-metag-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-ID: To: Stephen Rothwell Cc: Al Viro , linux-metag-u79uwXL29TY76Z2rM5mHXA@public.gmane.org --nrXiCraHbXeog9mY Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi Stephen, On Tue, May 02, 2017 at 10:11:54PM +0100, James Hogan wrote: > The __user_bad() macro used by access_ok() has a few corner cases > noticed by Al Viro where it doesn't behave correctly: >=20 > - The kernel range check has off by 1 errors which permit access to the > first and last byte of the kernel mapped range. >=20 > - The kernel range check ends at LINCORE_BASE rather than > META_MEMORY_LIMIT, which is ineffective when the kernel is in global > space (an extremely uncommon configuration). >=20 > There are a couple of other shortcomings here too: >=20 > - Access to the whole of the other address space is permitted (i.e. the > global half of the address space when the kernel is in local space). > This isn't ideal as it could theoretically still contain privileged > mappings set up by the bootloader. >=20 > - The size argument is unused, permitting user copies which start on > valid pages at the end of the user address range and cross the > boundary into the kernel address space (e.g. addr =3D 0x3ffffff0, size > > 0x10). >=20 > It isn't very convenient to add size checks when disallowing certain > regions, and it seems far safer to be sure and explicit about what > userland is able to access, so invert the logic to allow certain regions > instead, and fix the off by 1 errors and missing size checks. This also > allows the get_fs() =3D=3D KERNEL_DS check to be more easily optimised in= to > the user address range case. >=20 > We now have 3 such allowed regions: >=20 > - The user address range (incorporating the get_fs() =3D=3D KERNEL_DS > check). >=20 > - NULL (some kernel code expects this to work, and we'll always catch > the fault anyway). >=20 > - The core code memory region. >=20 > Fixes: 373cd784d0fc ("metag: Memory handling") > Reported-by: Al Viro > Signed-off-by: James Hogan > Cc: linux-metag-u79uwXL29TY76Z2rM5mHXA@public.gmane.org > Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org > --- > arch/metag/include/asm/uaccess.h | 40 +++++++++++++++++++-------------- > 1 file changed, 24 insertions(+), 16 deletions(-) >=20 > diff --git a/arch/metag/include/asm/uaccess.h b/arch/metag/include/asm/ua= ccess.h > index 469a2f1393d3..1e5f26d2dce8 100644 > --- a/arch/metag/include/asm/uaccess.h > +++ b/arch/metag/include/asm/uaccess.h > @@ -28,24 +28,32 @@ > =20 > #define segment_eq(a, b) ((a).seg =3D=3D (b).seg) > =20 > -#define __kernel_ok (segment_eq(get_fs(), KERNEL_DS)) FYI this patch, commit 8a8b56638bca ("metag/uaccess: Fix access_ok()"), just pushed to metag/for-next, will conflict with Al's commit db68ce10c4f0 ("new helper: uaccess_kernel()") from his vfs/for-next branch. This change should supersede Al's metag change, i.e. __kernel_ok should be removed from this file. Cheers James > -/* > - * Explicitly allow NULL pointers here. Parts of the kernel such > - * as readv/writev use access_ok to validate pointers, but want > - * to allow NULL pointers for various reasons. NULL pointers are > - * safe to allow through because the first page is not mappable on > - * Meta. > - * > - * We also wish to avoid letting user code access the system area > - * and the kernel half of the address space. > - */ > -#define __user_bad(addr, size) (((addr) > 0 && (addr) < META_MEMORY_BASE= ) || \ > - ((addr) > PAGE_OFFSET && \ > - (addr) < LINCORE_BASE)) > - > static inline int __access_ok(unsigned long addr, unsigned long size) > { > - return __kernel_ok || !__user_bad(addr, size); > + /* > + * Allow access to the user mapped memory area, but not the system area > + * before it. The check extends to the top of the address space when > + * kernel access is allowed (there's no real reason to user copy to the > + * system area in any case). > + */ > + if (likely(addr >=3D META_MEMORY_BASE && addr < get_fs().seg && > + size <=3D get_fs().seg - addr)) > + return true; > + /* > + * Explicitly allow NULL pointers here. Parts of the kernel such > + * as readv/writev use access_ok to validate pointers, but want > + * to allow NULL pointers for various reasons. NULL pointers are > + * safe to allow through because the first page is not mappable on > + * Meta. > + */ > + if (!addr) > + return true; > + /* Allow access to core code memory area... */ > + if (addr >=3D LINCORE_CODE_BASE && addr <=3D LINCORE_CODE_LIMIT && > + size <=3D LINCORE_CODE_LIMIT + 1 - addr) > + return true; > + /* ... but no other areas. */ > + return false; > } > =20 > #define access_ok(type, addr, size) __access_ok((unsigned long)(addr), \ > --=20 > git-series 0.8.10 --nrXiCraHbXeog9mY Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEd80NauSabkiESfLYbAtpk944dnoFAlkI+WUACgkQbAtpk944 dnoCDA/9Hd3eMnYAbg3QNdyk4Yum+pks8b9L9cLkBN5HcFriNi/BXITpgy5qyxZk hyz2pzgJykSgA9AFKduwv5jxfMknWI8kWTmBpbKeqEHXISIteq5cXionCqRwWlhS KSDXGOm64ebzSIPn3mvm/8nlFJIKyNE3S+iMCrfzOsZ0u2/zQdQnuTdxWYnCfiZN z/vsS2jTc6e+ZpNcp6dv3k4gaTtDQiejfSj3uBYUTkQfoLwbdIpQDSOxUSvgPCD8 tlwIKcitAwyFYyDzEgeonl5j7jO8/9VY4KZftn4t6jw61hJFoDOrpNlRWPeP1lj/ HKgpgXjS4rZKAJoPsSDfHGqDnDTxBKwiqbH+fCleDG8OTNA1xyb0klZ1j1VxqcIA IR3rKg8djYlCupUpJt2KVoEes0HO9HEsxDSlrQYpYTlgz5f4lt7b3kj/ytPur3Lf jtD0CL5LiwqOSKB7ia/3fAl+340Bmj56Z/kSpEkv83ZlCbcVcHBq4vmziOP1TMIu yUE5/Co1yKXtSv4LfRxarW9/RuXQYMlwoD/0dpHMRrL3AOwAYbXqBNI0SlfxqDEb rzlLBrVNEK0r/70aAoGB05Z805DkMe6IVFqjxuzxjSOpX0NamgenSzqREbcdGvPU 4ZkCjLmH7E3XFIMFuRvQUZfieIZyGBIxliY9zWJ0YTwSFJs/QTE= =hGms -----END PGP SIGNATURE----- --nrXiCraHbXeog9mY--