[git patch] DRM 32/64 ioctl patch..

[git patch] DRM 32/64 ioctl patch..

[Dave Airlie]

Hi Linus,
	Please pull the 'drm-3264' branch of
rsync://rsync.kernel.org/pub/scm/linux/kernel/git/airlied/drm-2.6.git

This contains the initial patch from Paul Mackerras, for supporting
radeons on 32/64 systems, I'll try and submit patches for other chips
later as people get them working.

The patch is at
http://www.skynet.ie/~airlied/patches/lk_drm/drm_3264_git.diff
for anyone else interested.


 Makefile       |    5
 drmP.h         |    5
 drm_bufs.c     |   25 -
 drm_context.c  |    6
 drm_ioc32.c    | 1069 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 radeon_drv.c   |    3
 radeon_drv.h   |    3
 radeon_ioc32.c |  395 +++++++++++++++++++++
 8 files changed, 1501 insertions(+), 10 deletions(-)

commit 9a18664506dbce5e23f3c5de7b1c5a042dd26520
tree 8f047c14a507b3f925e50f797650b15e23058a77
parent ee98689be1b054897ff17655008c3048fe88be94
author Dave Airlie <airlied@starflyer.(none)> Thu, 23 Jun 2005 21:29:18 +1000
committer Dave Airlie <airlied@linux.ie> Thu, 23 Jun 2005 21:29:18 +1000

drm: 32/64-bit DRM ioctl compatibility patch

The patch is against a 2.6.11 kernel tree.  I am running this with a
32-bit X server (compiled up from X.org CVS as of a couple of weeks
ago) and 32-bit DRI libraries and clients.  All the userland stuff is
identical to what I am using under a 32-bit kernel on my G4 powerbook
(which is a 32-bit machine of course).  I haven't tried compiling up a
64-bit X server or clients yet.

In the compatibility routines I have assumed that the kernel can
safely access user addresses after set_fs(KERNEL_DS).  That is, where
an ioctl argument structure contains pointers to other structures, and
those other structures are already compatible between the 32-bit and
64-bit ABIs (i.e. they only contain things like chars, shorts or
ints), I just check the address with access_ok() and then pass it
through to the 64-bit ioctl code.  I believe this approach may not
work on sparc64, but it does work on ppc64 and x86_64 at least.

One tricky area which may need to be revisited is the question of how
to handle the handles which we pass back to userspace to identify
mappings.  These handles are generated in the ADDMAP ioctl and then
passed in as the offset value to mmap.  However, offset values for
mmap seem to be generated in other ways as well, particularly for AGP
mappings.

The approach I have ended up with is to generate a fake 32-bit handle
only for _DRM_SHM mappings.  The handles for other mappings (AGP, REG,
FB) are physical addresses which are already limited to 32 bits, and
generating fake handles for them created all sorts of problems in the
mmap/nopage code.

This patch has been updated to use the new compatibility ioctls.

From: Paul Mackerras <paulus@samba.org>
Signed-off-by: Dave Airlie <airlied@linux.ie>

Re: [git patch] DRM 32/64 ioctl patch..

[Christoph Hellwig]

On Sun, Jun 26, 2005 at 01:22:56PM +0100, Dave Airlie wrote:
> 
> Hi Linus,
> 	Please pull the 'drm-3264' branch of
> rsync://rsync.kernel.org/pub/scm/linux/kernel/git/airlied/drm-2.6.git
> 
> This contains the initial patch from Paul Mackerras, for supporting
> radeons on 32/64 systems, I'll try and submit patches for other chips
> later as people get them working.
> 
> The patch is at
> http://www.skynet.ie/~airlied/patches/lk_drm/drm_3264_git.diff
> for anyone else interested.

I talked to Egbert Eich at Linuxtag and he said he had different compat
ioctl patch for drm, which actually supports running a 64bit server
and 32bit clients.

The big question is here, does this patch help to reach this goal or does
it make that more awkward?

Re: [git patch] DRM 32/64 ioctl patch..

[Dave Airlie]

>
> I talked to Egbert Eich at Linuxtag and he said he had different compat
> ioctl patch for drm, which actually supports running a 64bit server
> and 32bit clients.
>
> The big question is here, does this patch help to reach this goal or does
> it make that more awkward?

It shouldn't break it, Paulus and Egbert were at least talking about this
stuff, Paulus gave me an easier to integrate patch and worked with me to
get it in shape, my next plan was to start taking pieces of Egberts work
for the other cards and putting it into the kernel...

Egberts work also fixes some problems in userspace but these aren't any
concern of mine really and those fixes need to be put into the Mesa and
X.org trees, it also makes sure backwards compat is better satisfied as
more people will test with a new/old combination..

Dave.

-- 
David Airlie, Software Engineer
http://www.skynet.ie/~airlied / airlied at skynet.ie
Linux kernel - DRI, VAX / pam_smb / ILUG

Re: [git patch] DRM 32/64 ioctl patch..

[Arnd Bergmann]

On Sünndag 26 Juni 2005 14:22, Dave Airlie wrote:

> In the compatibility routines I have assumed that the kernel can
> safely access user addresses after set_fs(KERNEL_DS).  That is, where
> an ioctl argument structure contains pointers to other structures, and
> those other structures are already compatible between the 32-bit and
> 64-bit ABIs (i.e. they only contain things like chars, shorts or
> ints), I just check the address with access_ok() and then pass it
> through to the 64-bit ioctl code.  I believe this approach may not
> work on sparc64, but it does work on ppc64 and x86_64 at least.

Are you sure that comment still applies? I can't find any reference
to set_fs in the drm code and compat_alloc_user_space() based handlers
do not have the problem.

Otherwise that approach opens up a security hole by giving user access to
kernel memory on all architectures that have separate address spaces for
user and kernel instead of different ranges in the same address space.

Guessing from the implementation of get_fs/set_fs, these would include
m68k, s390{,x}, sparc{,64} and i386 with the 4G/4G mapping, so these
must never build code that relies on working user pointer dereferences
under set_fs(KERNEL_DS).

> +typedef struct drm_version_32 {
> +	int	version_major;	  /**< Major version */
> +	int	version_minor;	  /**< Minor version */
> +	int	version_patchlevel;/**< Patch level */
> +	u32	name_len;	  /**< Length of name buffer */
> +	u32	name;		  /**< Name of driver */

compat_uptr_t ?

> +	u32	date_len;	  /**< Length of date buffer */
> +	u32	date;		  /**< User-space buffer to hold date */

same here

> +	u32	desc_len;	  /**< Length of desc buffer */
> +	u32	desc;		  /**< User-space buffer to hold desc */

and here

> +} drm_version32_t;
> +
> +static int compat_drm_version(struct file *file, unsigned int cmd,
> +			      unsigned long arg)
> +{
> +	drm_version32_t v32;
> +	drm_version_t __user *version;
> +	int err;
> +
> +	if (copy_from_user(&v32, (void __user *) arg, sizeof(v32)))

(void __user *) arg should really be compat_ptr(arg). In theory,
this is only necessary on s390, which does not implement drm,
but we just do it the right way so other people don't copy
the incorrect code.

> +		return -EFAULT;
> +
> +	version = compat_alloc_user_space(sizeof(*version));
> +	if (!access_ok(VERIFY_WRITE, version, sizeof(*version)))
> +		return -EFAULT;
> +	if (__put_user(v32.name_len, &version->name_len)
> +	    || __put_user((void __user *)(unsigned long)v32.name,
> +			  &version->name)
> +	    || __put_user(v32.date_len, &version->date_len)
> +	    || __put_user((void __user *)(unsigned long)v32.date,
> +			  &version->date)
> +	    || __put_user(v32.desc_len, &version->desc_len)
> +	    || __put_user((void __user *)(unsigned long)v32.desc,
> +			  &version->desc))
> +		return -EFAULT;

Same here. Note how compat_ptr also makes that more readable.
More of these are in other parts of the patch.

	Arnd <><

Re: [git patch] DRM 32/64 ioctl patch..

[Paul Mackerras]

Arnd Bergmann writes:

> Are you sure that comment still applies? I can't find any reference
> to set_fs in the drm code and compat_alloc_user_space() based handlers
> do not have the problem.

No, the comment is out of date; I changed the code to use
compat_alloc_user_space().

> (void __user *) arg should really be compat_ptr(arg). In theory,
> this is only necessary on s390, which does not implement drm,
> but we just do it the right way so other people don't copy
> the incorrect code.

Good point.

Paul.