From: Dave A. <ai...@gm...> - 2009-10-28 01:22:32
|
So for drm KMS we wrote a bunch of ioctls that were 64-bit clean in theory. They used uint64_t to represent userspace pointers and userspace casted into those and the kernel casts back out and passes it to copy_*_user Now I thought cool I don't need to worry about compat ioctl hackery I can run 32 on 64 bit apps fine and it'll all just work. Now Dave Miller points out that I'm obivously deluded and we really need to add compat ioctls so that the kernel can truncate correctly 32-bit address in case userspace shoves garbage into the top 32bits of the u64. Is there really no way to avoid compat ioctls? was I delusional in thinking there was? Dave. |
From: David M. <da...@da...> - 2009-10-28 02:25:48
|
From: Dave Airlie <ai...@gm...> Date: Wed, 28 Oct 2009 11:22:18 +1000 > Is there really no way to avoid compat ioctls? was I delusional in > thinking there was? If you use pointers in your interfaces in any way, no. And for this drm_radeon_info thing the pointer is "pointless", you're just returning 32-bit values to the user, just use a u32 inside of the drm_radeon_info structure for the kernel to place the result in. |
From: Dave A. <ai...@gm...> - 2009-10-28 03:01:48
|
On Wed, Oct 28, 2009 at 12:25 PM, David Miller <da...@da...> wrote: > From: Dave Airlie <ai...@gm...> > Date: Wed, 28 Oct 2009 11:22:18 +1000 > >> Is there really no way to avoid compat ioctls? was I delusional in >> thinking there was? > > If you use pointers in your interfaces in any way, no. > > And for this drm_radeon_info thing the pointer is "pointless", > you're just returning 32-bit values to the user, just use > a u32 inside of the drm_radeon_info structure for the kernel > to place the result in. The plan for that was to expand it later, for now 32-bit was all we used. Dave. |
From: Dave A. <ai...@gm...> - 2009-10-28 03:05:20
|
On Wed, Oct 28, 2009 at 12:53 PM, Andi Kleen <an...@fi...> wrote: > Dave Airlie <ai...@gm...> writes: > >> They used uint64_t to represent userspace pointers and userspace >> casted into those and the kernel casts back out and passes it to copy_*_user > > uint64_t is actually dangerous due to different alignment on x86-32 vs 64, > better use compat_u64/s64 We've designed that into a/c also, we pad all 64-bit values to 64-bit alignment on all the ioctls we've added to the drm in the past couple of years. Just because of this particular insanity. > >> Now I thought cool I don't need to worry about compat ioctl hackery I can >> run 32 on 64 bit apps fine and it'll all just work. >> >> Now Dave Miller points out that I'm obivously deluded and we really need >> to add compat ioctls so that the kernel can truncate correctly 32-bit address >> in case userspace shoves garbage into the top 32bits of the u64. > > When the user space sees a u64 field it should never shove garbage here. > You just have to cast on 32bit for this, which is a bit ugly. > > However some architectures need special operations on compat pointers > (s390 iirc), but if you don't support those it might be reasonable > to not support that. > >> Is there really no way to avoid compat ioctls? was I delusional in >> thinking there was? > > Experience shows that people make mistakes and you sooner or > later need them anyways to work around them. > Assume no mistakes are made, new ioctls designed from scratch and reviewed to do 32/64-bit properly. The s390 was something I didn't know about but KMS on s390 is probably never going to be something that sees the light of day. I'm just amazed that compat_ioctl should be required for all new code. DrNick on irc suggested just doing: if (is_compat_task()) ptr &= 0x00000000FFFFFFFF; Is there a one liner I can just do in the actual ioctls instead of adding 20 compat ones? Dave. |
From: Dave A. <ai...@gm...> - 2009-10-28 03:28:20
|
On Wed, Oct 28, 2009 at 1:19 PM, Andi Kleen <an...@fi...> wrote: > On Wed, Oct 28, 2009 at 01:05:08PM +1000, Dave Airlie wrote: >> We've designed that into a/c also, we pad all 64-bit values to 64-bit >> alignment on all the >> ioctls we've added to the drm in the past couple of years. Just because of >> this particular insanity. > > That's actually not needed, just use compat_*64. >> >> Assume no mistakes are made, new ioctls designed from scratch > > That seems like a bad assumption. It sounds like you already > made some. You mean its impossible to design a 32/64-bit safe ioctl no matter what? and we should live with having compat ioctls? This isn't something that was very clearly stated or documented, the advice we had previously was that compat ioctls were only required for old ioctls or ioctls with design problems. compat_*64 didn't exist when this code we designed, and we worked around that, but it was in no way mistaken, manually aligning 64-bit values is a perfectly good solution, not sure why you imply it isn't. > >> and reviewed to do 32/64-bit properly. The s390 was something I didn't >> know about but KMS on s390 is probably never going to be something >> that sees the light of day. > > Well in theory there might be more architectures in the future > which rely on compat_ptr > Well this was what I was trying to gather, so maybe I just need to write something up to state that compat_ioctl is always required for new ioctls that pass pointers or 64-bit values hiding pointers, so more people don't make this mistake going forward. I can say when we inquired about this 2 or so years ago when designing kms I didn't get this answer, which is a pity. Dave. |
From: David M. <da...@da...> - 2009-10-28 03:41:45
|
From: Dave Airlie <ai...@gm...> Date: Wed, 28 Oct 2009 13:28:10 +1000 > On Wed, Oct 28, 2009 at 1:19 PM, Andi Kleen <an...@fi...> wrote: >> On Wed, Oct 28, 2009 at 01:05:08PM +1000, Dave Airlie wrote: >>> We've designed that into a/c also, we pad all 64-bit values to 64-bit >>> alignment on all the >>> ioctls we've added to the drm in the past couple of years. Just because of >>> this particular insanity. >> >> That's actually not needed, just use compat_*64. >>> >>> Assume no mistakes are made, new ioctls designed from scratch >> >> That seems like a bad assumption. It sounds like you already >> made some. > > You mean its impossible to design a 32/64-bit safe ioctl no matter what? If you use pointers at all, yes. We've said this several times. > and we should live with having compat ioctls? This isn't something that was very > clearly stated or documented, the advice we had previously was that > compat ioctls > were only required for old ioctls or ioctls with design problems. compat_*64 > didn't exist when this code we designed, and we worked around that, but it was > in no way mistaken, manually aligning 64-bit values is a perfectly > good solution, > not sure why you imply it isn't. Manually "aligning" the way you have solves only one side of the compat problem on x86 with 64-bit types. It may handle the layout properly bit it doesn't change the alignment requirements of the containing structure and that's a similarly important the issue. If you want to solve both aspects, use the "aligned_u64" type. But once you introduce pointers, you must have compat layer code, and this is a hard requirement. And there is nothing baroque or wrong about it. |
From: Dave A. <ai...@li...> - 2009-10-28 03:43:22
|
> > > DrNick on irc suggested just doing: > > if (is_compat_task()) ptr &= 0x00000000FFFFFFFF; > > > > Is there a one liner I can just do in the actual ioctls instead of > > adding 20 compat > > ones? > > Just do the right thing and pass all userland compat pointers > through the correct compat_*() macros. I wondered why the other ioctls worked, (uint32_t __user *)(unsigned long)card_res->fb_id_ptr; we already opencoded this (probably before it was macroisied or we just pasted it), so the radeon one is buggy, I should just go and compat_* all of these then and we should be all happy? Dave. |
From: Dave A. <ai...@li...> - 2009-10-28 03:54:51
|
> > > we already opencoded this (probably before it was macroisied or we just > > pasted it), so the radeon one is buggy, I should just go and compat_* all > > of these then and we should be all happy? > > It should be, it's only working because: > > 1) A malicious userland hasn't put garbage in the upper bits for > you yet. > > 2) Nobody has tested s390 yet :-) > So will an inline like this work? static inline void *__user convert_user_ptr(uint64_t ioctl_ptr) { #ifdef CONFIG_COMPAT if (is_compat_task()) return compat_ptr((compat_uptr_t)ioctl_ptr); else #endif return (void __user *)(unsigned long)ioctl_ptr; } then I can convert all the code to just use that instead of explicity casts or brokenness. Dave. |
From: Dave A. <ai...@li...> - 2009-10-28 05:42:43
|
> Please don't do this. > > This is exactly what I feared people would do when is_compat_task() > was added. is_compat_task() is for situations where there is > otherwise no other way to handle the compat situation properly. > > It's not that much work for you to hook up the compat ioctls properly, > and if you are clever you can do it in such a way that you'll get > warnings if someone accidently adds a new ioctl but forgets the > compat bits :-) There are close to 15 ioctls needing this, so I can add 15 functions to unpack and repack stuff or I can add that function, sorry if I'm leaning towards a lighter weight solution. I'll add this to my TODO for before the next merge window as its definitely more than I can manage now. Dave. |
From: David M. <da...@da...> - 2009-10-28 05:28:07
|
From: Dave Airlie <ai...@li...> Date: Wed, 28 Oct 2009 03:54:34 +0000 (GMT) >> >> > we already opencoded this (probably before it was macroisied or we just >> > pasted it), so the radeon one is buggy, I should just go and compat_* all >> > of these then and we should be all happy? >> >> It should be, it's only working because: >> >> 1) A malicious userland hasn't put garbage in the upper bits for >> you yet. >> >> 2) Nobody has tested s390 yet :-) >> > > So will an inline like this work? > > static inline void *__user convert_user_ptr(uint64_t ioctl_ptr) > { Please don't do this. This is exactly what I feared people would do when is_compat_task() was added. is_compat_task() is for situations where there is otherwise no other way to handle the compat situation properly. It's not that much work for you to hook up the compat ioctls properly, and if you are clever you can do it in such a way that you'll get warnings if someone accidently adds a new ioctl but forgets the compat bits :-) |
From: David M. <da...@da...> - 2009-10-28 06:04:45
|
From: Dave Airlie <ai...@li...> Date: Wed, 28 Oct 2009 05:42:27 +0000 (GMT) > I'll add this to my TODO for before the next merge window as its > definitely more than I can manage now. I'll do it. |
From: Andi K. <an...@fi...> - 2009-10-28 07:59:23
|
> } > - chunk_array_ptr = (uint64_t *)(unsigned long)(cs->chunks); > +#ifdef CONFIG_COMPAT > + if (is_compat_task()) Are the COMPAT ifdefs really needed? The compiler should optimize that away anyways on non compat aware architectures, shouldn't it? -Andi |
From: Andi K. <an...@fi...> - 2009-10-28 08:19:29
|
On Wed, Oct 28, 2009 at 01:11:41AM -0700, David Miller wrote: > From: Andi Kleen <an...@fi...> > Date: Wed, 28 Oct 2009 08:59:08 +0100 > > >> } > >> - chunk_array_ptr = (uint64_t *)(unsigned long)(cs->chunks); > >> +#ifdef CONFIG_COMPAT > >> + if (is_compat_task()) > > > > Are the COMPAT ifdefs really needed? The compiler should optimize that > > away anyways on non compat aware architectures, shouldn't it? > > There are no non-compat is_compat_task() definitions, nor are there > non-compat build definitions of compat_uptr_t and the assosciated > interfaces. That seems wrong then, better fix that too? It would be certainly better than adding a lot of ifdefs. -Andi -- ak...@li... -- Speaking for myself only. |
From: David M. <da...@da...> - 2009-10-28 08:28:09
|
From: Andi Kleen <an...@fi...> Date: Wed, 28 Oct 2009 09:19:09 +0100 > On Wed, Oct 28, 2009 at 01:11:41AM -0700, David Miller wrote: >> From: Andi Kleen <an...@fi...> >> Date: Wed, 28 Oct 2009 08:59:08 +0100 >> >> >> } >> >> - chunk_array_ptr = (uint64_t *)(unsigned long)(cs->chunks); >> >> +#ifdef CONFIG_COMPAT >> >> + if (is_compat_task()) >> > >> > Are the COMPAT ifdefs really needed? The compiler should optimize that >> > away anyways on non compat aware architectures, shouldn't it? >> >> There are no non-compat is_compat_task() definitions, nor are there >> non-compat build definitions of compat_uptr_t and the assosciated >> interfaces. > > That seems wrong then, better fix that too? It would be certainly better > than adding a lot of ifdefs. That's usually done by seperating the compat code into a seperate file and "obj-$(CONFIG_COMPAT) += foo.o" in the Makefile. That's not really possible here. Sure, longer term we can provide those kinds of definitions to avoid the ifdefs, but that's not what my change is about. :-) |
From: David M. <da...@da...> - 2009-10-28 08:11:34
|
From: Andi Kleen <an...@fi...> Date: Wed, 28 Oct 2009 08:59:08 +0100 >> } >> - chunk_array_ptr = (uint64_t *)(unsigned long)(cs->chunks); >> +#ifdef CONFIG_COMPAT >> + if (is_compat_task()) > > Are the COMPAT ifdefs really needed? The compiler should optimize that > away anyways on non compat aware architectures, shouldn't it? There are no non-compat is_compat_task() definitions, nor are there non-compat build definitions of compat_uptr_t and the assosciated interfaces. |
From: David M. <da...@da...> - 2009-10-28 12:32:09
|
From: Arnd Bergmann <arn...@go...> Date: Wed, 28 Oct 2009 13:13:32 +0100 > The ioctl argument actually needs a compat_ptr() conversion as well. > For the s390 case, we can't do that in common code, because some > ioctl methods put a 32 bit integer into the argument. Not sure if we > want to fix that everywhere, the problem is very common and the > impact is minimal. What does s390 do with the 'arg' argument to sys_ioctl()? That assumption that you can cast this to a pointer is everywhere. If someone wants to fix this up, feel free to do an audit and go over that seperately from my work :-) |
From: Arnd B. <arn...@go...> - 2009-10-28 15:40:36
|
On Wednesday 28 October 2009, David Miller wrote: > > The ioctl argument actually needs a compat_ptr() conversion as well. > > For the s390 case, we can't do that in common code, because some > > ioctl methods put a 32 bit integer into the argument. Not sure if we > > want to fix that everywhere, the problem is very common and the > > impact is minimal. > > What does s390 do with the 'arg' argument to sys_ioctl()? It clears the top 32 bits, but not bit 31, because that is significant for a few ioctl handlers passing data directly instead of a pointer. > That assumption that you can cast this to a pointer is everywhere. Yes, I know :( > If someone wants to fix this up, feel free to do an audit and go > over that seperately from my work :-) Cc'ing Heiko and Martin, since I'm not working on s390 any more. I'm pretty sure it was ok when we started adding the compat_ioctl handlers years ago. I think most people just ignored these for the majority of drivers that can't possibly run on s390. Even on s390, gcc will always do the right thing if you call call ioctl with a pointer to a normal object in the .data section, heap or stack, but hand-written assembly or other compilers may not. Arnd <>< |
From: David M. <da...@da...> - 2009-10-28 07:53:41
|
From: David Miller <da...@da...> Date: Tue, 27 Oct 2009 23:04:50 -0700 (PDT) > From: Dave Airlie <ai...@li...> > Date: Wed, 28 Oct 2009 05:42:27 +0000 (GMT) > >> I'll add this to my TODO for before the next merge window as its >> definitely more than I can manage now. > > I'll do it. Ok, everything was straightforward except for radeon_cs, which has three levels of indirection for the tables of data and relocation chunks userland can pass into the DRM :-/ There is no way to isolate the compat handling without parsing and bringing the pointer array and the chunk headers into the kernel twice. So I guess I'm willing to capitulate with is_compat_task() checks in radeon_cs_init(), but if you want to improve this interface I see two ways: 1) Eliminate one level of indirection so there is just a straight scatter-gather list at one level. 2) Use a base pointer and a set of offsets to communicate at least one level of indirection. Both schemes should satisfy the buffering flexibility these interfaces seem to be geared towards giving to userland. Anyways the following patch is tested and seems to work well. Signed-off-by: David S. Miller <da...@da...> diff --git a/drivers/gpu/drm/radeon/radeon_cs.c b/drivers/gpu/drm/radeon/radeon_cs.c index 5ab2cf9..ba44eba 100644 --- a/drivers/gpu/drm/radeon/radeon_cs.c +++ b/drivers/gpu/drm/radeon/radeon_cs.c @@ -24,6 +24,8 @@ * Authors: * Jerome Glisse <gl...@fr...> */ +#include <linux/compat.h> + #include "drmP.h" #include "radeon_drm.h" #include "radeon_reg.h" @@ -107,7 +109,12 @@ int radeon_cs_parser_init(struct radeon_cs_parser *p, void *data) if (p->chunks_array == NULL) { return -ENOMEM; } - chunk_array_ptr = (uint64_t *)(unsigned long)(cs->chunks); +#ifdef CONFIG_COMPAT + if (is_compat_task()) + chunk_array_ptr = compat_ptr((compat_uptr_t) cs->chunks); + else +#endif + chunk_array_ptr = (uint64_t *)(unsigned long)(cs->chunks); if (DRM_COPY_FROM_USER(p->chunks_array, chunk_array_ptr, sizeof(uint64_t)*cs->num_chunks)) { return -EFAULT; @@ -120,9 +127,15 @@ int radeon_cs_parser_init(struct radeon_cs_parser *p, void *data) for (i = 0; i < p->nchunks; i++) { struct drm_radeon_cs_chunk __user **chunk_ptr = NULL; struct drm_radeon_cs_chunk user_chunk; - uint32_t __user *cdata; - chunk_ptr = (void __user*)(unsigned long)p->chunks_array[i]; +#ifdef CONFIG_COMPAT + if (is_compat_task()) + chunk_ptr = compat_ptr((compat_uptr_t) + p->chunks_array[i]); + else +#endif + chunk_ptr = (void __user*) (unsigned long) + p->chunks_array[i]; if (DRM_COPY_FROM_USER(&user_chunk, chunk_ptr, sizeof(struct drm_radeon_cs_chunk))) { return -EFAULT; @@ -142,9 +155,16 @@ int radeon_cs_parser_init(struct radeon_cs_parser *p, void *data) } p->chunks[i].length_dw = user_chunk.length_dw; - p->chunks[i].user_ptr = (void __user *)(unsigned long)user_chunk.chunk_data; +#ifdef CONFIG_COMPAT + if (is_compat_task()) + p->chunks[i].user_ptr = + compat_ptr((compat_uptr_t) + user_chunk.chunk_data); + else +#endif + p->chunks[i].user_ptr = (void __user *) (unsigned long) + user_chunk.chunk_data; - cdata = (uint32_t *)(unsigned long)user_chunk.chunk_data; if (p->chunks[i].chunk_id != RADEON_CHUNK_ID_IB) { size = p->chunks[i].length_dw * sizeof(uint32_t); p->chunks[i].kdata = kmalloc(size, GFP_KERNEL); diff --git a/drivers/gpu/drm/radeon/radeon_ioc32.c b/drivers/gpu/drm/radeon/radeon_ioc32.c index a1bf11d..3c4dfa2 100644 --- a/drivers/gpu/drm/radeon/radeon_ioc32.c +++ b/drivers/gpu/drm/radeon/radeon_ioc32.c @@ -156,7 +156,7 @@ static int compat_radeon_cp_stipple(struct file *file, unsigned int cmd, typedef struct drm_radeon_tex_image32 { unsigned int x, y; /* Blit coordinates */ unsigned int width, height; - u32 data; + compat_uptr_t data; } drm_radeon_tex_image32_t; typedef struct drm_radeon_texture32 { @@ -165,7 +165,7 @@ typedef struct drm_radeon_texture32 { int format; int width; /* Texture image coordinates */ int height; - u32 image; + compat_uptr_t image; } drm_radeon_texture32_t; static int compat_radeon_cp_texture(struct file *file, unsigned int cmd, @@ -180,8 +180,7 @@ static int compat_radeon_cp_texture(struct file *file, unsigned int cmd, return -EFAULT; if (req32.image == 0) return -EINVAL; - if (copy_from_user(&img32, (void __user *)(unsigned long)req32.image, - sizeof(img32))) + if (copy_from_user(&img32, compat_ptr(req32.image), sizeof(img32))) return -EFAULT; request = compat_alloc_user_space(sizeof(*request) + sizeof(*image)); @@ -200,8 +199,7 @@ static int compat_radeon_cp_texture(struct file *file, unsigned int cmd, || __put_user(img32.y, &image->y) || __put_user(img32.width, &image->width) || __put_user(img32.height, &image->height) - || __put_user((const void __user *)(unsigned long)img32.data, - &image->data)) + || __put_user(compat_ptr(img32.data), &image->data)) return -EFAULT; return drm_ioctl(file->f_path.dentry->d_inode, file, @@ -212,9 +210,9 @@ typedef struct drm_radeon_vertex2_32 { int idx; /* Index of vertex buffer */ int discard; /* Client finished with buffer? */ int nr_states; - u32 state; + compat_uptr_t state; int nr_prims; - u32 prim; + compat_uptr_t prim; } drm_radeon_vertex2_32_t; static int compat_radeon_cp_vertex2(struct file *file, unsigned int cmd, @@ -231,11 +229,9 @@ static int compat_radeon_cp_vertex2(struct file *file, unsigned int cmd, || __put_user(req32.idx, &request->idx) || __put_user(req32.discard, &request->discard) || __put_user(req32.nr_states, &request->nr_states) - || __put_user((void __user *)(unsigned long)req32.state, - &request->state) + || __put_user(compat_ptr(req32.state), &request->state) || __put_user(req32.nr_prims, &request->nr_prims) - || __put_user((void __user *)(unsigned long)req32.prim, - &request->prim)) + || __put_user(compat_ptr(req32.prim), &request->prim)) return -EFAULT; return drm_ioctl(file->f_path.dentry->d_inode, file, @@ -244,9 +240,9 @@ static int compat_radeon_cp_vertex2(struct file *file, unsigned int cmd, typedef struct drm_radeon_cmd_buffer32 { int bufsz; - u32 buf; + compat_uptr_t buf; int nbox; - u32 boxes; + compat_uptr_t boxes; } drm_radeon_cmd_buffer32_t; static int compat_radeon_cp_cmdbuf(struct file *file, unsigned int cmd, @@ -261,11 +257,9 @@ static int compat_radeon_cp_cmdbuf(struct file *file, unsigned int cmd, request = compat_alloc_user_space(sizeof(*request)); if (!access_ok(VERIFY_WRITE, request, sizeof(*request)) || __put_user(req32.bufsz, &request->bufsz) - || __put_user((void __user *)(unsigned long)req32.buf, - &request->buf) + || __put_user(compat_ptr(req32.buf), &request->buf) || __put_user(req32.nbox, &request->nbox) - || __put_user((void __user *)(unsigned long)req32.boxes, - &request->boxes)) + || __put_user(compat_ptr(req32.boxes), &request->boxes)) return -EFAULT; return drm_ioctl(file->f_path.dentry->d_inode, file, @@ -274,7 +268,7 @@ static int compat_radeon_cp_cmdbuf(struct file *file, unsigned int cmd, typedef struct drm_radeon_getparam32 { int param; - u32 value; + compat_uptr_t value; } drm_radeon_getparam32_t; static int compat_radeon_cp_getparam(struct file *file, unsigned int cmd, @@ -289,8 +283,7 @@ static int compat_radeon_cp_getparam(struct file *file, unsigned int cmd, request = compat_alloc_user_space(sizeof(*request)); if (!access_ok(VERIFY_WRITE, request, sizeof(*request)) || __put_user(req32.param, &request->param) - || __put_user((void __user *)(unsigned long)req32.value, - &request->value)) + || __put_user(compat_ptr(req32.value), &request->value)) return -EFAULT; return drm_ioctl(file->f_path.dentry->d_inode, file, @@ -301,7 +294,7 @@ typedef struct drm_radeon_mem_alloc32 { int region; int alignment; int size; - u32 region_offset; /* offset from start of fb or GART */ + compat_uptr_t region_offset; /* offset from start of fb or GART */ } drm_radeon_mem_alloc32_t; static int compat_radeon_mem_alloc(struct file *file, unsigned int cmd, @@ -318,7 +311,7 @@ static int compat_radeon_mem_alloc(struct file *file, unsigned int cmd, || __put_user(req32.region, &request->region) || __put_user(req32.alignment, &request->alignment) || __put_user(req32.size, &request->size) - || __put_user((int __user *)(unsigned long)req32.region_offset, + || __put_user(compat_ptr(req32.region_offset), &request->region_offset)) return -EFAULT; @@ -327,7 +320,7 @@ static int compat_radeon_mem_alloc(struct file *file, unsigned int cmd, } typedef struct drm_radeon_irq_emit32 { - u32 irq_seq; + compat_uptr_t irq_seq; } drm_radeon_irq_emit32_t; static int compat_radeon_irq_emit(struct file *file, unsigned int cmd, @@ -341,43 +334,42 @@ static int compat_radeon_irq_emit(struct file *file, unsigned int cmd, request = compat_alloc_user_space(sizeof(*request)); if (!access_ok(VERIFY_WRITE, request, sizeof(*request)) - || __put_user((int __user *)(unsigned long)req32.irq_seq, - &request->irq_seq)) + || __put_user(compat_ptr(req32.irq_seq), &request->irq_seq)) return -EFAULT; return drm_ioctl(file->f_path.dentry->d_inode, file, DRM_IOCTL_RADEON_IRQ_EMIT, (unsigned long)request); } -/* The two 64-bit arches where alignof(u64)==4 in 32-bit code */ #if defined (CONFIG_X86_64) || defined(CONFIG_IA64) typedef struct drm_radeon_setparam32 { int param; u64 value; } __attribute__((packed)) drm_radeon_setparam32_t; +#else +#define drm_radeon_setparam32_t drm_radeon_setparam_t +#endif static int compat_radeon_cp_setparam(struct file *file, unsigned int cmd, unsigned long arg) { drm_radeon_setparam32_t req32; drm_radeon_setparam_t __user *request; + compat_uptr_t uptr; if (copy_from_user(&req32, (void __user *) arg, sizeof(req32))) return -EFAULT; request = compat_alloc_user_space(sizeof(*request)); + uptr = req32.value; if (!access_ok(VERIFY_WRITE, request, sizeof(*request)) || __put_user(req32.param, &request->param) - || __put_user((void __user *)(unsigned long)req32.value, - &request->value)) + || __put_user(compat_ptr(uptr), &request->value)) return -EFAULT; return drm_ioctl(file->f_dentry->d_inode, file, DRM_IOCTL_RADEON_SETPARAM, (unsigned long) request); } -#else -#define compat_radeon_cp_setparam NULL -#endif /* X86_64 || IA64 */ drm_ioctl_compat_t *radeon_compat_ioctls[] = { [DRM_RADEON_CP_INIT] = compat_radeon_cp_init, @@ -392,6 +384,95 @@ drm_ioctl_compat_t *radeon_compat_ioctls[] = { [DRM_RADEON_IRQ_EMIT] = compat_radeon_irq_emit, }; +static int compat_radeon_info_ioctl(struct file *file, unsigned int cmd, + unsigned long arg) +{ + struct drm_radeon_info __user *uinfo; + struct drm_radeon_info kinfo; + compat_uptr_t uaddr; + void *uptr; + + if (copy_from_user(&kinfo, (void __user *) arg, sizeof(kinfo))) + return -EFAULT; + + uaddr = kinfo.value; + uptr = compat_ptr(uaddr); + if (kinfo.value == (uint64_t) uptr) + return drm_ioctl(file->f_dentry->d_inode, file, + DRM_IOCTL_RADEON_INFO, arg); + + kinfo.value = (uint64_t) uptr; + + uinfo = compat_alloc_user_space(sizeof(*uinfo)); + if (copy_to_user(uinfo, &kinfo, sizeof(kinfo))) + return -EFAULT; + + return drm_ioctl(file->f_dentry->d_inode, file, + DRM_IOCTL_RADEON_INFO, (unsigned long) uinfo); +} + +static int compat_radeon_gem_pread_ioctl(struct file *file, unsigned int cmd, + unsigned long arg) +{ + struct drm_radeon_gem_pread __user *upread; + struct drm_radeon_gem_pread kpread; + compat_uptr_t uaddr; + void *uptr; + + if (copy_from_user(&kpread, (void __user *) arg, sizeof(kpread))) + return -EFAULT; + + uaddr = kpread.data_ptr; + uptr = compat_ptr(uaddr); + + if (kpread.data_ptr == (uint64_t) uptr) + return drm_ioctl(file->f_dentry->d_inode, file, + DRM_IOCTL_RADEON_GEM_PREAD, arg); + + kpread.data_ptr = (uint64_t) uptr; + + upread = compat_alloc_user_space(sizeof(*upread)); + if (copy_to_user(upread, &kpread, sizeof(kpread))) + return -EFAULT; + + return drm_ioctl(file->f_dentry->d_inode, file, + DRM_IOCTL_RADEON_GEM_PREAD, (unsigned long) upread); +} + +static int compat_radeon_gem_pwrite_ioctl(struct file *file, unsigned int cmd, + unsigned long arg) +{ + struct drm_radeon_gem_pwrite __user *upwrite; + struct drm_radeon_gem_pwrite kpwrite; + compat_uptr_t uaddr; + void *uptr; + + if (copy_from_user(&kpwrite, (void __user *) arg, sizeof(kpwrite))) + return -EFAULT; + + uaddr = kpwrite.data_ptr; + uptr = compat_ptr(uaddr); + + if (kpwrite.data_ptr == (uint64_t) uptr) + return drm_ioctl(file->f_dentry->d_inode, file, + DRM_IOCTL_RADEON_GEM_PWRITE, arg); + + kpwrite.data_ptr = (uint64_t) uptr; + + upwrite = compat_alloc_user_space(sizeof(*upwrite)); + if (copy_to_user(upwrite, &kpwrite, sizeof(kpwrite))) + return -EFAULT; + + return drm_ioctl(file->f_dentry->d_inode, file, + DRM_IOCTL_RADEON_GEM_PREAD, (unsigned long) upwrite); +} + +static drm_ioctl_compat_t *radeon_compat_kms_ioctls[] = { + [DRM_RADEON_INFO] = compat_radeon_info_ioctl, + [DRM_RADEON_GEM_PREAD] = compat_radeon_gem_pread_ioctl, + [DRM_RADEON_GEM_PWRITE] = compat_radeon_gem_pwrite_ioctl, +}; + /** * Called whenever a 32-bit process running under a 64-bit kernel * performs an ioctl on /dev/dri/card<n>. @@ -426,13 +507,20 @@ long radeon_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) long radeon_kms_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) { unsigned int nr = DRM_IOCTL_NR(cmd); + drm_ioctl_compat_t *fn = NULL; int ret; if (nr < DRM_COMMAND_BASE) return drm_compat_ioctl(filp, cmd, arg); + if (nr < DRM_COMMAND_BASE + DRM_ARRAY_SIZE(radeon_compat_kms_ioctls)) + fn = radeon_compat_kms_ioctls[nr - DRM_COMMAND_BASE]; + lock_kernel(); /* XXX for now */ - ret = drm_ioctl(filp->f_path.dentry->d_inode, filp, cmd, arg); + if (fn != NULL) + ret = (*fn) (filp, cmd, arg); + else + ret = drm_ioctl(filp->f_path.dentry->d_inode, filp, cmd, arg); unlock_kernel(); return ret; |
From: Arnd B. <arn...@go...> - 2009-10-28 12:41:36
|
On Wednesday 28 October 2009, David Miller wrote: > -/* The two 64-bit arches where alignof(u64)==4 in 32-bit code */ > #if defined (CONFIG_X86_64) || defined(CONFIG_IA64) > typedef struct drm_radeon_setparam32 { > int param; > u64 value; > } __attribute__((packed)) drm_radeon_setparam32_t; > +#else > +#define drm_radeon_setparam32_t drm_radeon_setparam_t > +#endif I guess a cleaner way to put this would be typedef struct drm_radeon_setparam32 { int param; compat_u64 value; } drm_radeon_setparam32_t; > static int compat_radeon_cp_setparam(struct file *file, unsigned int cmd, > unsigned long arg) > { > drm_radeon_setparam32_t req32; > drm_radeon_setparam_t __user *request; > + compat_uptr_t uptr; > > if (copy_from_user(&req32, (void __user *) arg, sizeof(req32))) > return -EFAULT; The ioctl argument actually needs a compat_ptr() conversion as well. For the s390 case, we can't do that in common code, because some ioctl methods put a 32 bit integer into the argument. Not sure if we want to fix that everywhere, the problem is very common and the impact is minimal. > +static int compat_radeon_info_ioctl(struct file *file, unsigned int cmd, > + unsigned long arg) > +{ > + struct drm_radeon_info __user *uinfo; > + struct drm_radeon_info kinfo; > + compat_uptr_t uaddr; > + void *uptr; > + > + if (copy_from_user(&kinfo, (void __user *) arg, sizeof(kinfo))) > + return -EFAULT; > + > + uaddr = kinfo.value; > + uptr = compat_ptr(uaddr); > + if (kinfo.value == (uint64_t) uptr) > + return drm_ioctl(file->f_dentry->d_inode, file, > + DRM_IOCTL_RADEON_INFO, arg); > + > + kinfo.value = (uint64_t) uptr; > + > + uinfo = compat_alloc_user_space(sizeof(*uinfo)); > + if (copy_to_user(uinfo, &kinfo, sizeof(kinfo))) > + return -EFAULT; > + > + return drm_ioctl(file->f_dentry->d_inode, file, > + DRM_IOCTL_RADEON_INFO, (unsigned long) uinfo); > +} IMHO a better way to handle the radeon specific ioctls would be to avoid the compat_alloc_user_space and just define the common function taking a kernel pointer, with two implementations of the copy operation: static int __radeon_info_ioctl(struct file *file, struct drm_radeon_info *info); static int radeon_info_ioctl(struct file *file, unsigned int cmd, unsigned long arg) { struct drm_radeon_info kinfo; if (copy_from_user(&kinfo, (void __user *)arg, sizeof(kinfo))) return -EFAULT; return __radeon_info_ioctl(file, cmd, &kinfo); } static int radeon_info_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg) { struct compat_drm_radeon_info kinfo; if (copy_from_user(&kinfo, compat_ptr(arg), sizeof(kinfo))) return -EFAULT; kinfo.value = (u64)compat_ptr(kinfo.value); return __radeon_info_ioctl(file, cmd, &kinfo); } > @@ -426,13 +507,20 @@ long radeon_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) > long radeon_kms_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) > { > unsigned int nr = DRM_IOCTL_NR(cmd); > + drm_ioctl_compat_t *fn = NULL; > int ret; > > if (nr < DRM_COMMAND_BASE) > return drm_compat_ioctl(filp, cmd, arg); > > + if (nr < DRM_COMMAND_BASE + DRM_ARRAY_SIZE(radeon_compat_kms_ioctls)) > + fn = radeon_compat_kms_ioctls[nr - DRM_COMMAND_BASE]; > + > lock_kernel(); /* XXX for now */ > - ret = drm_ioctl(filp->f_path.dentry->d_inode, filp, cmd, arg); > + if (fn != NULL) > + ret = (*fn) (filp, cmd, arg); > + else > + ret = drm_ioctl(filp->f_path.dentry->d_inode, filp, cmd, arg); > unlock_kernel(); This could consequently become if (nr < DRM_COMMAND_BASE) return drm_compat_ioctl(filp, cmd, arg); + switch (cmd) { + case DRM_IOCTL_RADEON_INFO: + return radeon_info_compat_ioctl(filp, cmd, arg); + case ... + } + lock_kernel(); /* XXX for now */ ret = drm_ioctl(filp->f_path.dentry->d_inode, filp, cmd, arg); unlock_kernel(); The other changes in your patch look good to me. Arnd <>< |
From: David M. <da...@da...> - 2009-10-28 12:32:14
|
From: Arnd Bergmann <arn...@go...> Date: Wed, 28 Oct 2009 13:13:32 +0100 > IMHO a better way to handle the radeon specific ioctls would be to > avoid the compat_alloc_user_space and just define the common > function taking a kernel pointer, with two implementations of the > copy operation: Agreed, that would be a great follow-on patch to mine. |
From: David M. <da...@da...> - 2009-10-29 05:41:51
|
From: Arnd Bergmann <arn...@go...> Date: Wed, 28 Oct 2009 16:40:18 +0100 > I'm pretty sure it was ok when we started adding the compat_ioctl > handlers years ago. I think most people just ignored these for > the majority of drivers that can't possibly run on s390. Even > on s390, gcc will always do the right thing if you call call ioctl > with a pointer to a normal object in the .data section, heap or stack, > but hand-written assembly or other compilers may not. Arnd, even compat_sys_ioctl() itself has constructs like: case FS_IOC_RESVSP: case FS_IOC_RESVSP64: error = ioctl_preallocate(filp, (void __user *)arg); goto out_fput; That's why I asked about the 'arg' argument to sys_ioctl on s390 :-) |
From: Arnd B. <ar...@ar...> - 2009-10-29 08:30:24
|
On Thursday 29 October 2009, David Miller wrote: > Arnd, even compat_sys_ioctl() itself has constructs like: > > case FS_IOC_RESVSP: > case FS_IOC_RESVSP64: > error = ioctl_preallocate(filp, (void __user *)arg); > goto out_fput; Right. This one is pretty recent and I did not notice it doing this unfortunately. There are a few others added to fs/compat_ioctl.c over the years also doing it. Arnd <>< |
From: Heiko C. <hei...@de...> - 2009-10-29 08:51:46
|
On Wed, Oct 28, 2009 at 10:41:57PM -0700, David Miller wrote: > From: Arnd Bergmann <arn...@go...> > Date: Wed, 28 Oct 2009 16:40:18 +0100 > > > I'm pretty sure it was ok when we started adding the compat_ioctl > > handlers years ago. I think most people just ignored these for > > the majority of drivers that can't possibly run on s390. Even > > on s390, gcc will always do the right thing if you call call ioctl > > with a pointer to a normal object in the .data section, heap or stack, > > but hand-written assembly or other compilers may not. > > Arnd, even compat_sys_ioctl() itself has constructs like: > > case FS_IOC_RESVSP: > case FS_IOC_RESVSP64: > error = ioctl_preallocate(filp, (void __user *)arg); > goto out_fput; That's broken, but it's quite new code. In general it looks like we don't have many compat ioctl problems on s390. At least I don't remember when we faced the last bug. We did have some compat syscall issues when SLES11 testing started. The lack of bug reports is probably just a lack of 32 bit userspace ;) This should fix at least the bug above: Subject: [PATCH] fs: add missing compat_ptr handling for FS_IOC_RESVSP ioctl From: Heiko Carstens <hei...@de...> For FS_IOC_RESVSP and FS_IOC_RESVSP64 compat_sys_ioctl() uses its arg argument as a pointer to userspace. However it is missing a a call to compat_ptr() which will do a proper pointer conversion. This was introduced with 3e63cbb1 "fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls". Cc: Ankit Jain <me...@an...> Cc: Christoph Hellwig <hc...@ls...> Cc: Al Viro <vi...@ze...> Cc: Arnd Bergmann <arn...@go...> Reported-by: David Miller <da...@da...> Signed-off-by: Heiko Carstens <hei...@de...> --- fs/compat_ioctl.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c index f91fd51..d84e705 100644 --- a/fs/compat_ioctl.c +++ b/fs/compat_ioctl.c @@ -1800,7 +1800,7 @@ struct space_resv_32 { /* just account for different alignment */ static int compat_ioctl_preallocate(struct file *file, unsigned long arg) { - struct space_resv_32 __user *p32 = (void __user *)arg; + struct space_resv_32 __user *p32 = compat_ptr(arg); struct space_resv __user *p = compat_alloc_user_space(sizeof(*p)); if (copy_in_user(&p->l_type, &p32->l_type, sizeof(s16)) || @@ -2802,7 +2802,7 @@ asmlinkage long compat_sys_ioctl(unsigned int fd, unsigned int cmd, #else case FS_IOC_RESVSP: case FS_IOC_RESVSP64: - error = ioctl_preallocate(filp, (void __user *)arg); + error = ioctl_preallocate(filp, compat_ptr(arg)); goto out_fput; #endif |
From: David M. <da...@da...> - 2009-10-29 08:39:08
|
From: Heiko Carstens <hei...@de...> Date: Thu, 29 Oct 2009 09:34:16 +0100 > Subject: [PATCH] fs: add missing compat_ptr handling for FS_IOC_RESVSP ioctl > > From: Heiko Carstens <hei...@de...> > > For FS_IOC_RESVSP and FS_IOC_RESVSP64 compat_sys_ioctl() uses its > arg argument as a pointer to userspace. However it is missing a > a call to compat_ptr() which will do a proper pointer conversion. > > This was introduced with 3e63cbb1 "fs: Add new pre-allocation ioctls > to vfs for compatibility with legacy xfs ioctls". > > Cc: Ankit Jain <me...@an...> > Cc: Christoph Hellwig <hc...@ls...> > Cc: Al Viro <vi...@ze...> > Cc: Arnd Bergmann <arn...@go...> > Reported-by: David Miller <da...@da...> > Signed-off-by: Heiko Carstens <hei...@de...> Acked-by: David S. Miller <da...@da...> |
From: Dave A. <ai...@gm...> - 2009-10-30 01:13:22
|
On Wed, Oct 28, 2009 at 5:53 PM, David Miller <da...@da...> wrote: > From: David Miller <da...@da...> > Date: Tue, 27 Oct 2009 23:04:50 -0700 (PDT) > >> From: Dave Airlie <ai...@li...> >> Date: Wed, 28 Oct 2009 05:42:27 +0000 (GMT) >> >>> I'll add this to my TODO for before the next merge window as its >>> definitely more than I can manage now. >> >> I'll do it. Btw when I mentioned ioctls I meant more than radeon, all the KMS ioctls in the common drm_crtc.c file suffer from this problem as well. Hence why I still believe either my drm specific inline or something more generic (granted I can see why a generic solution would be ugly). You patch below does suffer from a lot of #ifdefs and cut-n-paste that is a lot better suited to doing in an inline or macro. We can then comment that inline saying if anyone else does this we will be most unhappy. Dave. |