From patchwork Thu Apr 11 18:59:27 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tommi Rantala X-Patchwork-Id: 2430051 Return-Path: X-Original-To: patchwork-dri-devel@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by patchwork2.kernel.org (Postfix) with ESMTP id E96D5DF230 for ; Thu, 11 Apr 2013 18:59:41 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 0A5AEE6602 for ; Thu, 11 Apr 2013 11:59:42 -0700 (PDT) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mail-vc0-f182.google.com (mail-vc0-f182.google.com [209.85.220.182]) by gabe.freedesktop.org (Postfix) with ESMTP id 94D0AE5C20; Thu, 11 Apr 2013 11:59:28 -0700 (PDT) Received: by mail-vc0-f182.google.com with SMTP id ht10so1581192vcb.13 for ; Thu, 11 Apr 2013 11:59:27 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:x-received:in-reply-to:references:date:message-id :subject:from:to:content-type; bh=rzkzwv/nv4cwCrpWg0uYVPf246qoSplyPCYBqVRrDws=; b=hT5tjt7hTW9SCl1+96Lp6A3B57bFe5r+co/y+e1nJ9tdVhGF4ZSEv8tjkAIbj7NFAA XwF501fFOOWSqUEIparUoFBwkf0yRIK27rASSbZW3skFFJxOWEsicCp8m8We5hKeDB0P Ghxj8FZdmJiWNnD+SaPnJmq/OtxcnCAjKEcLzWyjLBDOCR4/8Picz+mGF/p+OsDlib9D +F+3KMHgH0NRA53w/3WR2kOHzGruwO4WI4/p8jAretCv2vk6/JgcAhRvPQuwP1DLMCCa KAyTS5nioUXdfa9sXV4YHwzBeCTwT3PpvF7gdy6ThNhy+03QiQfmd4CeaT4JhZ5zLFRh 535Q== MIME-Version: 1.0 X-Received: by 10.220.73.68 with SMTP id p4mr6086650vcj.41.1365706767815; Thu, 11 Apr 2013 11:59:27 -0700 (PDT) Received: by 10.58.161.78 with HTTP; Thu, 11 Apr 2013 11:59:27 -0700 (PDT) In-Reply-To: <20130317215142.GC26102@cantiga.alporthouse.com> References: <1363265997-29023-1-git-send-email-chris@chris-wilson.co.uk> <20130315045004.GA14747@bwidawsk.net> <20130315082403.GA29916@cantiga.alporthouse.com> <20130315163606.GA17773@bwidawsk.net> <20130315220619.GA17405@cantiga.alporthouse.com> <20130315234942.GB715@bwidawsk.net> <20130316101920.GB17405@cantiga.alporthouse.com> <20130317214049.GB26102@cantiga.alporthouse.com> <20130317215142.GC26102@cantiga.alporthouse.com> Date: Thu, 11 Apr 2013 21:59:27 +0300 Message-ID: Subject: Re: [Intel-gfx] [PATCH] drm/i915: Sanity check incoming ioctl data for a NULL pointer From: Tommi Rantala To: Chris Wilson , Dave Airlie , Daniel Vetter , Ben Widawsky , Tommi Rantala , David Airlie , intel-gfx@lists.freedesktop.org, LKML , dri-devel@lists.freedesktop.org, Dave Jones X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+patchwork-dri-devel=patchwork.kernel.org@lists.freedesktop.org Errors-To: dri-devel-bounces+patchwork-dri-devel=patchwork.kernel.org@lists.freedesktop.org 2013/3/17 Chris Wilson : > On Mon, Mar 18, 2013 at 07:42:58AM +1000, Dave Airlie wrote: >> On Mon, Mar 18, 2013 at 7:40 AM, Chris Wilson wrote: >> > On Sun, Mar 17, 2013 at 08:50:03PM +0100, Daniel Vetter wrote: >> >> On Sat, Mar 16, 2013 at 11:19 AM, Chris Wilson wrote: >> >> > If *userspace* doesn't request either IOC_IN | IOC_OUT in their ioctl >> >> > command (which are seperate from the ioctl number), then kdata is set to >> >> > NULL. >> >> >> >> Doesn't that mean that we need these checks everywhere? Or at least a >> >> fixup in drm core proper? >> > >> > That's my conclusion. We either add a flag to ask drm_ioctl to prevent >> > passing NULL pointers (as the existing behaviour may be useful >> > somewhere, and I have not checked all callees) or saturate our callbacks >> > with NULL checks. >> >> Do we have the kernel's expected IOC_IN/IOC_OUT flags at that point as well? >> >> we could check them and block NULL in that case. > > Yes. For the core ioctls, we use drm_ioctls[nr].cmd rather than the > value passed in by userspace for the IOC_IN|IN_OUT bits. So: Thanks, trinity can indeed set the in/out bits randomly, in a way that does not match the driver ioctl definition. Your patch almost fixes this. For the driver ioctls we will want to grab the cmd from cmd_drv. So the patch should be: Can you please submit this officially? > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > index 25f91cd..79b8bd1 100644 > --- a/drivers/gpu/drm/drm_drv.c > +++ b/drivers/gpu/drm/drm_drv.c > @@ -408,6 +408,7 @@ long drm_ioctl(struct file *filp, > usize = asize = _IOC_SIZE(cmd); > if (drv_size > asize) > asize = drv_size; > + cmd = ioctl->cmd; > } > else if ((nr >= DRM_COMMAND_END) || (nr < DRM_COMMAND_BASE)) { > ioctl = &drm_ioctls[nr]; > > > -- > Chris Wilson, Intel Open Source Technology Centre diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 25f91cd..5210f33 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -408,6 +408,7 @@ long drm_ioctl(struct file *filp, usize = asize = _IOC_SIZE(cmd); if (drv_size > asize) asize = drv_size; + cmd = ioctl->cmd_drv; } else if ((nr >= DRM_COMMAND_END) || (nr < DRM_COMMAND_BASE)) { ioctl = &drm_ioctls[nr];