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: Dave Airlie <[email protected]>
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.
Dave Airlie <[email protected]> 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
> 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.
-Andi
--
[email protected] -- Speaking for myself only.
On Wed, Oct 28, 2009 at 12:25 PM, David Miller <[email protected]> wrote:
> From: Dave Airlie <[email protected]>
> 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.
On Wed, Oct 28, 2009 at 12:53 PM, Andi Kleen <[email protected]> wrote:
> Dave Airlie <[email protected]> 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.
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.
> 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
>
> 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;
Such hacks often have problems on BE.
-Andi
--
[email protected] -- Speaking for myself only.
On Wed, Oct 28, 2009 at 1:19 PM, Andi Kleen <[email protected]> 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.
On Wed, Oct 28, 2009 at 01:28:10PM +1000, Dave Airlie wrote:
> You mean its impossible to design a 32/64-bit safe ioctl no matter what?
Not impossible, but there's always a rate of mistakes.
> 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.
It's true, just saying that even people who try to avoid creating
them often (but not always) need them anyways in the end.
>
> >
> >> 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.
Right now you could probably ignore it (if you document it), since
there are no non s390 architectures with this problem, just
prepare mentally that you might need to revisit this at some point.
-Andi
--
[email protected] -- Speaking for myself only.
From: Andi Kleen <[email protected]>
Date: Wed, 28 Oct 2009 03:53:17 +0100
> Dave Airlie <[email protected]> writes:
>
>> 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.
The problem is that it can if it wants to.
On sparc64, in order to make debugging easier, we trap any time
the kernel does a userspace access to a compat task and any
of the upper 32-bits are non-zero.
That's more than a reasonable assumption to make because user
pointers are either:
1) Zero chopped when passed in via register arguments to a syscall.
2) Go through the compat_uptr() interface which chops the upper bits.
So whether userland should or should not do something, we still have
to guard against it.
> You just have to cast on 32bit for this, which is a bit ugly.
Right, via the compat_*() interfaces.
> 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.
s390 has to sign extend all 32-bit compat process pointers when
processing them in the 64-bit s390 kernel. I think one other
architecture has this kind of situation too.
From: Dave Airlie <[email protected]>
Date: Wed, 28 Oct 2009 13:05:08 +1000
> 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.
From: Dave Airlie <[email protected]>
Date: Wed, 28 Oct 2009 13:28:10 +1000
> On Wed, Oct 28, 2009 at 1:19 PM, Andi Kleen <[email protected]> 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.
>
> > 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: Andi Kleen <[email protected]>
Date: Wed, 28 Oct 2009 04:34:55 +0100
> On Wed, Oct 28, 2009 at 01:28:10PM +1000, Dave Airlie wrote:
>> 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.
>
> Right now you could probably ignore it (if you document it), since
> there are no non s390 architectures with this problem, just
> prepare mentally that you might need to revisit this at some point.
You can't ignore it on sparc64, it already OOPS's, and I refuse to
live with that "if (is_compat_task())" masking hack, no way.
We designed portable interfaces for doing this stuff, please use it.
From: Dave Airlie <[email protected]>
Date: Wed, 28 Oct 2009 03:43:07 +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 :-)
Let's just not design non-portability into the code when we
have no strong reason to, ok? :-)
On Tue, Oct 27, 2009 at 08:45:30PM -0700, David Miller wrote:
> From: Dave Airlie <[email protected]>
> Date: Wed, 28 Oct 2009 03:43:07 +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.
The x86 compat_ptr wouldn't even help with that because it doesn't mask.
If they use *_user() or anything else with access_ok later that should
be caught properly. The user land could only put in pointers to unmapped
[32bit...kernel boundary] data, which is harmless.
-Andi
--
[email protected] -- Speaking for myself only.
>
> > 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.
On Tue, Oct 27, 2009 at 08:37:09PM -0700, David Miller wrote:
> On sparc64, in order to make debugging easier, we trap any time
> the kernel does a userspace access to a compat task and any
> of the upper 32-bits are non-zero.
Interesting. That definitely means Dave needs a special path.
> > 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.
>
> s390 has to sign extend all 32-bit compat process pointers when
> processing them in the 64-bit s390 kernel. I think one other
> architecture has this kind of situation too.
Which other architure? I reviewed all the definitions in tree
and don't see any other than s390 doing magic there.
-Andi
--
[email protected] -- Speaking for myself only.
From: Dave Airlie <[email protected]>
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: Andi Kleen <[email protected]>
Date: Wed, 28 Oct 2009 05:36:11 +0100
> On Tue, Oct 27, 2009 at 08:37:09PM -0700, David Miller wrote:
>> On sparc64, in order to make debugging easier, we trap any time
>> the kernel does a userspace access to a compat task and any
>> of the upper 32-bits are non-zero.
>
> Interesting. That definitely means Dave needs a special path.
I wouldn't call it special, the rule is that any userland pointer
has to either go through a syscall argument register or one
of the compat_*() accessor routines :-)
> 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: Dave Airlie <[email protected]>
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: David Miller <[email protected]>
Date: Tue, 27 Oct 2009 23:04:50 -0700 (PDT)
> From: Dave Airlie <[email protected]>
> 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 <[email protected]>
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 <[email protected]>
*/
+#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;
> }
> - 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 Kleen <[email protected]>
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.
On Wed, Oct 28, 2009 at 01:11:41AM -0700, David Miller wrote:
> From: Andi Kleen <[email protected]>
> 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
--
[email protected] -- Speaking for myself only.
From: Andi Kleen <[email protected]>
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 <[email protected]>
>> 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. :-)
On Wednesday 28 October 2009, Andi Kleen wrote:
> > > 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.
> >
> > s390 has to sign extend all 32-bit compat process pointers when
> > processing them in the 64-bit s390 kernel. I think one other
> > architecture has this kind of situation too.
>
> Which other architure? I reviewed all the definitions in tree
> and don't see any other than s390 doing magic there.
I'm also pretty sure that s390 is the only one needing this, I
added the compat_ptr stuff initially.
Note that a cast from pointer to unsigned long to u64 and back
in C does the correct 31 to 64 bit extension, which btw is not
a sign-extend but a unsigned extend clearing the upper 33 bits.
The easier rule to remember should be to always to compat_ptr()
on any pointer coming from user space, and to avoid pointers in
data structures where possible, as DaveM pointed out.
Arnd <><
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: Arnd Bergmann <[email protected]>
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 Bergmann <[email protected]>
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.
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 <><
On Wed, 28 Oct 2009, Andi Kleen wrote:
> > 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;
>
> Such hacks often have problems on BE.
And then some platforms (i.e. MIPS) require sign-extension rather than
zero-extension, that is:
if (is_compat_task()) ptr = ((ptr & 0xffffffff) ^ 0x80000000) - 0x80000000;
if doing this explicitly (with compat stuff hardware will do the right
thing automagically).
Maciej
From: Arnd Bergmann <[email protected]>
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 :-)
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 <><
On Wednesday 28 October 2009, Maciej W. Rozycki wrote:
>
> On Wed, 28 Oct 2009, Andi Kleen wrote:
>
> > > 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;
> >
> > Such hacks often have problems on BE.
>
> And then some platforms (i.e. MIPS) require sign-extension rather than
> zero-extension, that is:
>
> if (is_compat_task()) ptr = ((ptr & 0xffffffff) ^ 0x80000000) - 0x80000000;
>
> if doing this explicitly (with compat stuff hardware will do the right
> thing automagically).
Such conversion should *only* ever take place in compat_ptr().
Arnd <><
On Wed, Oct 28, 2009 at 10:41:57PM -0700, David Miller wrote:
> From: Arnd Bergmann <[email protected]>
> 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 <[email protected]>
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 <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Reported-by: David Miller <[email protected]>
Signed-off-by: Heiko Carstens <[email protected]>
---
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: Heiko Carstens <[email protected]>
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 <[email protected]>
>
> 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 <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Cc: Al Viro <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Reported-by: David Miller <[email protected]>
> Signed-off-by: Heiko Carstens <[email protected]>
Acked-by: David S. Miller <[email protected]>
On Wed, Oct 28, 2009 at 5:53 PM, David Miller <[email protected]> wrote:
> From: David Miller <[email protected]>
> Date: Tue, 27 Oct 2009 23:04:50 -0700 (PDT)
>
>> From: Dave Airlie <[email protected]>
>> 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.
On Friday 30 October 2009, Dave Airlie wrote:
> 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.
I think it would be better to do a conversion of the pointers in
a separate compat handler, but then call the regular function, which
in case of drm already take a kernel pointer. That would be much simpler
than the compat_alloc_user_space tricks in the current code and
also cleaner than trying to handle both cases in one function using
is_compat_task().
See the (not even compile-tested) example below as an illustration
of what I think you should do. If you convert all functions in
drm_ioc32.c to this scheme, you can replace drm_compat_ioctl and
drm_compat_ioctls[] with drm_generic_compat_ioctl.
Arnd <><
diff --git a/drivers/gpu/drm/drm_ioc32.c b/drivers/gpu/drm/drm_ioc32.c
index 282d9fd..334345b 100644
--- a/drivers/gpu/drm/drm_ioc32.c
+++ b/drivers/gpu/drm/drm_ioc32.c
@@ -1040,6 +1040,122 @@ static int compat_drm_wait_vblank(struct file *file, unsigned int cmd,
return 0;
}
+static int compat_drm_mode_getblob_ioctl(struct drm_device *dev,
+ struct drm_mode_get_blob __user *out_resp_user,
+ struct drm_file *file_priv)
+{
+ struct drm_mode_get_blob out_resp;
+ int ret;
+
+ ret = copy_from_user(&out_resp, out_resp_user, sizeof(out_resp))
+ if (ret)
+ return -EFAULT;
+
+ out_resp.data = (unsigned long)compat_ptr(out_resp.data);
+
+ ret = drm_mode_getblob_ioctl(dev, &out_resp, file_priv);
+ if (ret)
+ return ret;
+
+ ret = copy_to_user(out_resp_user, &out_resp, sizeof(out_resp))
+ if (ret)
+ return -EFAULT;
+ return 0;
+}
+
+static int compat_drm_mode_gamma_set_ioctl(struct drm_device *dev,
+ struct drm_mode_crtc_lut __user *crtc_lut_user,
+ struct drm_file *file_priv)
+{
+ struct drm_mode_crtc_lut crtc_lut;
+
+ ret = copy_from_user(&crtc_lut, crtc_lut_user, sizeof(crtc_lut))
+ if (ret)
+ return -EFAULT;
+
+ crtc_lut.red = (unsigned long)compat_ptr(crtc_lut.red);
+ crtc_lut.green = (unsigned long)compat_ptr(crtc_lut.green);
+ crtc_lut.blue = (unsigned long)compat_ptr(crtc_lut.blue);
+
+ ret = drm_mode_gamma_set_ioctl(dev, &crtc_lut, file_priv);
+
+ return ret;
+}
+
+static int compat_drm_mode_gamma_get_ioctl(struct drm_device *dev,
+ struct drm_mode_crtc_lut __user *crtc_lut_user,
+ struct drm_file *file_priv)
+{
+ struct drm_mode_crtc_lut crtc_lut;
+
+ ret = copy_from_user(&crtc_lut, crtc_lut_user, sizeof(crtc_lut))
+ if (ret)
+ return -EFAULT;
+
+ crtc_lut.red = (unsigned long)compat_ptr(crtc_lut.red);
+ crtc_lut.green = (unsigned long)compat_ptr(crtc_lut.green);
+ crtc_lut.blue = (unsigned long)compat_ptr(crtc_lut.blue);
+
+ ret = drm_mode_gamma_get_ioctl(dev, &crtc_lut, file_priv);
+
+ return ret;
+}
+
+static int drm_generic_compat_ioctl(struct file *filp, unsigned int cmd,
+ unsigned long arg)
+{
+ struct drm_file *file_priv = filp->private_data;
+ struct drm_device *dev = file_priv->minor->dev;
+ unsigned int nr = DRM_IOCTL_NR(cmd);
+ int ret;
+
+ atomic_inc(&dev->ioctl_count);
+ atomic_inc(&dev->counts[_DRM_STAT_IOCTLS]);
+ ++file_priv->ioctl_count;
+
+ if ((nr >= DRM_CORE_IOCTL_COUNT) &&
+ ((nr < DRM_COMMAND_BASE) || (nr >= DRM_COMMAND_END)))
+ goto err_i1;
+ if ((nr >= DRM_COMMAND_BASE) && (nr < DRM_COMMAND_END) &&
+ (nr < DRM_COMMAND_BASE + dev->driver->num_ioctls))
+ ioctl = &dev->driver->ioctls[nr - DRM_COMMAND_BASE];
+ else if ((nr >= DRM_COMMAND_END) || (nr < DRM_COMMAND_BASE)) {
+ ioctl = &drm_ioctls[nr];
+ cmd = ioctl->cmd;
+ } else
+ goto err_i1;
+
+ if (((ioctl->flags & DRM_ROOT_ONLY) && !capable(CAP_SYS_ADMIN)) ||
+ ((ioctl->flags & DRM_AUTH) && !file_priv->authenticated) ||
+ ((ioctl->flags & DRM_MASTER) && !file_priv->is_master) ||
+ (!(ioctl->flags & DRM_CONTROL_ALLOW) && (file_priv->minor->type == DRM_MINOR_CONTROL))) {
+ ret = -EACCES;
+ goto err_il;
+ }
+
+ switch (cmd) {
+ case DRM_IOCTL_MODE_GETPROPBLOB:
+ ret = compat_drm_mode_getblob_ioctl(dev,
+ compat_ptr(arg), file_priv);
+ break;
+
+ case DRM_IOCTL_MODE_GETGAMMA:
+ ret = compat_drm_mode_gamma_set_ioctl(dev,
+ compat_ptr(arg), file_priv);
+ break;
+
+ case DRM_IOCTL_MODE_GETGAMMA:
+ ret = compat_drm_mode_gamma_get_ioctl(dev,
+ compat_ptr(arg), file_priv);
+ break;
+ }
+
+ err_i1:
+ atomic_dec(&dev->ioctl_count);
+
+ return ret;
+}
+
drm_ioctl_compat_t *drm_compat_ioctls[] = {
[DRM_IOCTL_NR(DRM_IOCTL_VERSION32)] = compat_drm_version,
[DRM_IOCTL_NR(DRM_IOCTL_GET_UNIQUE32)] = compat_drm_getunique,
@@ -1072,6 +1188,9 @@ drm_ioctl_compat_t *drm_compat_ioctls[] = {
[DRM_IOCTL_NR(DRM_IOCTL_UPDATE_DRAW32)] = compat_drm_update_draw,
#endif
[DRM_IOCTL_NR(DRM_IOCTL_WAIT_VBLANK32)] = compat_drm_wait_vblank,
+ [DRM_IOCTL_MODE_GETPROPBLOB] = drm_generic_compat_ioctl,
+ [DRM_IOCTL_MODE_GETGAMMA] = drm_generic_compat_ioctl,
+ [DRM_IOCTL_MODE_SETGAMMA] = drm_generic_compat_ioctl,
};
/**
On Fri, Oct 30, 2009 at 8:13 PM, Arnd Bergmann <[email protected]> wrote:
> On Friday 30 October 2009, Dave Airlie wrote:
>> 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.
>
> I think it would be better to do a conversion of the pointers in
> a separate compat handler, but then call the regular function, which
> in case of drm already take a kernel pointer. That would be much simpler
> than the compat_alloc_user_space tricks in the current code and
> also cleaner than trying to handle both cases in one function using
> is_compat_task().
>
> See the (not even compile-tested) example below as an illustration
> of what I think you should do. If you convert all functions in
> drm_ioc32.c to this scheme, you can replace drm_compat_ioctl and
> drm_compat_ioctls[] with drm_generic_compat_ioctl.
>
I just got back out from F12 push to look at lkml again ;-)
My main problem which no-one seem to address with all these compat ioctls
is they suck for maintainance, adding the compat_ptr "hacks" into the actual
ioctls is much easier to maintain in that I can see in the code where we've done
these and if code flow has to change I can deal with it all in one place.
Doing all these out-of-line hidden over here in a separate file just
seems crazy,
and I'm having trouble wondering why ppl consider it a better idea at all.
It adds probably 1000 more LOC versus one inline with a decent name used
inline to replace current casting in the driver.
If the ioctl flow or interface "changes" someone has to remember about all
of these (granted this is unlikely since we pretty much set them in stone).
Dave.
> ? ? ? ?Arnd <><
>
> diff --git a/drivers/gpu/drm/drm_ioc32.c b/drivers/gpu/drm/drm_ioc32.c
> index 282d9fd..334345b 100644
> --- a/drivers/gpu/drm/drm_ioc32.c
> +++ b/drivers/gpu/drm/drm_ioc32.c
> @@ -1040,6 +1040,122 @@ static int compat_drm_wait_vblank(struct file *file, unsigned int cmd,
> ? ? ? ?return 0;
> ?}
>
> +static int compat_drm_mode_getblob_ioctl(struct drm_device *dev,
> + ? ? ? ? ? ? ? ? ? ? ? struct drm_mode_get_blob __user *out_resp_user,
> + ? ? ? ? ? ? ? ? ? ? ? struct drm_file *file_priv)
> +{
> + ? ? ? struct drm_mode_get_blob out_resp;
> + ? ? ? int ret;
> +
> + ? ? ? ret = copy_from_user(&out_resp, out_resp_user, sizeof(out_resp))
> + ? ? ? if (ret)
> + ? ? ? ? ? ? ? return -EFAULT;
> +
> + ? ? ? out_resp.data = (unsigned long)compat_ptr(out_resp.data);
> +
> + ? ? ? ret = drm_mode_getblob_ioctl(dev, &out_resp, file_priv);
> + ? ? ? if (ret)
> + ? ? ? ? ? ? ? return ret;
> +
> + ? ? ? ret = copy_to_user(out_resp_user, &out_resp, sizeof(out_resp))
> + ? ? ? if (ret)
> + ? ? ? ? ? ? ? return -EFAULT;
> + ? ? ? return 0;
> +}
> +
> +static int compat_drm_mode_gamma_set_ioctl(struct drm_device *dev,
> + ? ? ? ? ? ? ? ? ? ? ? struct drm_mode_crtc_lut __user *crtc_lut_user,
> + ? ? ? ? ? ? ? ? ? ? ? struct drm_file *file_priv)
> +{
> + ? ? ? struct drm_mode_crtc_lut crtc_lut;
> +
> + ? ? ? ret = copy_from_user(&crtc_lut, crtc_lut_user, sizeof(crtc_lut))
> + ? ? ? if (ret)
> + ? ? ? ? ? ? ? return -EFAULT;
> +
> + ? ? ? crtc_lut.red ? = (unsigned long)compat_ptr(crtc_lut.red);
> + ? ? ? crtc_lut.green = (unsigned long)compat_ptr(crtc_lut.green);
> + ? ? ? crtc_lut.blue ?= (unsigned long)compat_ptr(crtc_lut.blue);
> +
> + ? ? ? ret = drm_mode_gamma_set_ioctl(dev, &crtc_lut, file_priv);
> +
> + ? ? ? return ret;
> +}
> +
> +static int compat_drm_mode_gamma_get_ioctl(struct drm_device *dev,
> + ? ? ? ? ? ? ? ? ? ? ? struct drm_mode_crtc_lut __user *crtc_lut_user,
> + ? ? ? ? ? ? ? ? ? ? ? struct drm_file *file_priv)
> +{
> + ? ? ? struct drm_mode_crtc_lut crtc_lut;
> +
> + ? ? ? ret = copy_from_user(&crtc_lut, crtc_lut_user, sizeof(crtc_lut))
> + ? ? ? if (ret)
> + ? ? ? ? ? ? ? return -EFAULT;
> +
> + ? ? ? crtc_lut.red ? = (unsigned long)compat_ptr(crtc_lut.red);
> + ? ? ? crtc_lut.green = (unsigned long)compat_ptr(crtc_lut.green);
> + ? ? ? crtc_lut.blue ?= (unsigned long)compat_ptr(crtc_lut.blue);
> +
> + ? ? ? ret = drm_mode_gamma_get_ioctl(dev, &crtc_lut, file_priv);
> +
> + ? ? ? return ret;
> +}
> +
> +static int drm_generic_compat_ioctl(struct file *filp, unsigned int cmd,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?unsigned long arg)
> +{
> + ? ? ? struct drm_file *file_priv = filp->private_data;
> + ? ? ? struct drm_device *dev = file_priv->minor->dev;
> + ? ? ? unsigned int nr = DRM_IOCTL_NR(cmd);
> + ? ? ? int ret;
> +
> + ? ? ? atomic_inc(&dev->ioctl_count);
> + ? ? ? atomic_inc(&dev->counts[_DRM_STAT_IOCTLS]);
> + ? ? ? ++file_priv->ioctl_count;
> +
> + ? ? ? if ((nr >= DRM_CORE_IOCTL_COUNT) &&
> + ? ? ? ? ? ((nr < DRM_COMMAND_BASE) || (nr >= DRM_COMMAND_END)))
> + ? ? ? ? ? ? ? goto err_i1;
> + ? ? ? if ((nr >= DRM_COMMAND_BASE) && (nr < DRM_COMMAND_END) &&
> + ? ? ? ? ? (nr < DRM_COMMAND_BASE + dev->driver->num_ioctls))
> + ? ? ? ? ? ? ? ioctl = &dev->driver->ioctls[nr - DRM_COMMAND_BASE];
> + ? ? ? else if ((nr >= DRM_COMMAND_END) || (nr < DRM_COMMAND_BASE)) {
> + ? ? ? ? ? ? ? ioctl = &drm_ioctls[nr];
> + ? ? ? ? ? ? ? cmd = ioctl->cmd;
> + ? ? ? } else
> + ? ? ? ? ? ? ? goto err_i1;
> +
> + ? ? ? if (((ioctl->flags & DRM_ROOT_ONLY) && !capable(CAP_SYS_ADMIN)) ||
> + ? ? ? ? ? ? ? ? ?((ioctl->flags & DRM_AUTH) && !file_priv->authenticated) ||
> + ? ? ? ? ? ? ? ? ?((ioctl->flags & DRM_MASTER) && !file_priv->is_master) ||
> + ? ? ? ? ? ? ? ? ?(!(ioctl->flags & DRM_CONTROL_ALLOW) && (file_priv->minor->type == DRM_MINOR_CONTROL))) {
> + ? ? ? ? ? ? ? ret = -EACCES;
> + ? ? ? ? ? ? ? goto err_il;
> + ? ? ? }
> +
> + ? ? ? switch (cmd) {
> + ? ? ? case DRM_IOCTL_MODE_GETPROPBLOB:
> + ? ? ? ? ? ? ? ret = compat_drm_mode_getblob_ioctl(dev,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? compat_ptr(arg), file_priv);
> + ? ? ? ? ? ? ? break;
> +
> + ? ? ? case DRM_IOCTL_MODE_GETGAMMA:
> + ? ? ? ? ? ? ? ret = compat_drm_mode_gamma_set_ioctl(dev,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? compat_ptr(arg), file_priv);
> + ? ? ? ? ? ? ? break;
> +
> + ? ? ? case DRM_IOCTL_MODE_GETGAMMA:
> + ? ? ? ? ? ? ? ret = compat_drm_mode_gamma_get_ioctl(dev,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? compat_ptr(arg), file_priv);
> + ? ? ? ? ? ? ? break;
> + ? ? ? }
> +
> + ? ? ?err_i1:
> + ? ? ? atomic_dec(&dev->ioctl_count);
> +
> + ? ? ? return ret;
> +}
> +
> ?/**
>
> Doing all these out-of-line hidden over here in a separate file just
> seems crazy,
> and I'm having trouble wondering why ppl consider it a better idea at all.
The separate file is on the way out, the modern way is to use
->compat_ioctl in the driver itself. Near all drivers except
for a few exceptions like DRM put it into the same file.
I think you should just move the code around in DRM too
and drop the separate file.
-Andi
On Wednesday 18 November 2009, Andi Kleen wrote:
> > Doing all these out-of-line hidden over here in a separate file just
> > seems crazy,
> > and I'm having trouble wondering why ppl consider it a better idea at all.
>
> The separate file is on the way out, the modern way is to use
> ->compat_ioctl in the driver itself. Near all drivers except
> for a few exceptions like DRM put it into the same file.
> I think you should just move the code around in DRM too
> and drop the separate file.
Right. Commonly, you can simply handle the incompatible stuff in
->compat_ioctl and then call the regular function. Another way to
do it if you want to consolidate even more is to pass a flag down
the actual function and do
static int my_common_ioctl(struct my_object *my, unsigned int cmd, unsigned long arg,
void __user *argp, int compat)
{
...
}
static long my_native_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
{
return my_commmon_ioctl(file->private_data, cmd, arg, (void __user *)arg, 0);
}
static long my_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
{
return my_commmon_ioctl(file->private_data, cmd, arg, compat_ptr(arg), 1);
}
Doing that, you can have the ioctl handling for a subsystem consolidated in one
place, just doing the copy_from_user() part separately.
For the specific case of drm, where you have a table with all the ioctls, I can
see yet another option: add another function pointer to 'struct drm_ioctl_desc'
that does the conversion, e.g.
int drm_version_compat(void __user *argp, void *kdata, int dir)
{
struct compat_drm_version __user *uversion = argp;
struct compat_drm_version cversion;
struct drm_version *version = kdata;
if (dir == IOC_IN) {
if (copy_from_user(&cversion, uversion, sizeof(cversion);
return -EFAULT;
*version = (struct drm_version) {
.version_major = cversion.version_major,
.version_minor = cversion.version_minor,
.version_mpatchlevel = cversion.version_patchlevel,
.name_len = cversion.name_len,
.name = compat_ptr(cversion.name),
.date_len = cversion.date_len,
.date = compat_ptr(cversion.date),
.desc_len = cversion.desc_len,
.desc = compat_ptr(cversion.desc),
};
return 0;
} else if (dir == IOC_OUT) {
cversion = (struct drm_version) {
.version_major = version->version_major,
.version_minor = version->version_minor,
.version_mpatchlevel = version->version_patchlevel,
.name_len = version->name_len,
.name = (unsigned long)version->name,
.date_len = version->date_len,
.date = (unsigned long)version->date,
.desc_len = version->desc_len,
.desc = (unsigned long)version->desc,
};
if (copy_to_user(uversion, &cversion, sizeof(cversion);
return -EFAULT;
return 0;
}
return -EINVAL;
}
static struct drm_ioctl_desc drm_ioctls[] = {
DRM_IOCTL_DEF(DRM_IOCTL_VERSION, drm_version, drm_version_compat, 0),
...
};
Arnd <><