From patchwork Thu Dec 6 21:22:16 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Alasdair G Kergon X-Patchwork-Id: 1846801 Return-Path: X-Original-To: patchwork-dm-devel@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork1.kernel.org Received: from mx3-phx2.redhat.com (mx3-phx2.redhat.com [209.132.183.24]) by patchwork1.kernel.org (Postfix) with ESMTP id 1BE04400ED for ; Thu, 6 Dec 2012 21:28:22 +0000 (UTC) Received: from lists01.pubmisc.prod.ext.phx2.redhat.com (lists01.pubmisc.prod.ext.phx2.redhat.com [10.5.19.33]) by mx3-phx2.redhat.com (8.13.8/8.13.8) with ESMTP id qB6LMO7M012397; Thu, 6 Dec 2012 16:22:27 -0500 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by lists01.pubmisc.prod.ext.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id qB6LMNpl008408 for ; Thu, 6 Dec 2012 16:22:23 -0500 Received: from agk.fab.redhat.com (agk.fab.redhat.com [10.33.0.19]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id qB6LMHrg012690 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Thu, 6 Dec 2012 16:22:18 -0500 Received: from agk by agk.fab.redhat.com with local (Exim 4.34) id 1Tgitc-0003V5-Ja; Thu, 06 Dec 2012 21:22:16 +0000 Date: Thu, 6 Dec 2012 21:22:16 +0000 From: Alasdair G Kergon To: Mikulas Patocka Message-ID: <20121206212216.GJ6834@agk.fab.redhat.com> Mail-Followup-To: Mikulas Patocka , dm-devel@redhat.com References: Mime-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.4.1i Organization: Red Hat UK Ltd. Registered in England and Wales, number 03798903. Registered Office: Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE. X-Scanned-By: MIMEDefang 2.68 on 10.5.11.23 X-loop: dm-devel@redhat.com Cc: dm-devel@redhat.com Subject: Re: [dm-devel] [PATCH 2/3] dm-ioctl: fix unsafe use of dm_ioctl.data_size X-BeenThere: dm-devel@redhat.com X-Mailman-Version: 2.1.12 Precedence: junk Reply-To: device-mapper development List-Id: device-mapper development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dm-devel-bounces@redhat.com Errors-To: dm-devel-bounces@redhat.com I don't think we need to support the situation when the value changes during the copying of the parameters, so how about something more like this instead? Alasdair From: Alasdair G Kergon Abort dm ioctl processing if userspace changes the data_size parameter after we validated it but before we finished copying the data buffer from userspace. The dm ioctl parameters are processed in the following sequence: 1. ctl_ioctl() calls copy_params(); 2. copy_params() makes a first copy of the fixed-sized portion of the userspace parameters into the local variable "tmp"; 3. copy_params() then validates tmp.data_size and allocates a new structure big enough to hold the complete data and copies the whole userspace buffer there; 4. ctl_ioctl() reads userspace data the second time and copies the whole buffer into the pointer "param"; 5. ctl_ioctl() reads param->data_size without any validation and stores it in the variable "input_param_size"; 6. "input_param_size" is further used as the authoritative size of the kernel buffer. The problem is that userspace code could change the contents of user memory between steps 2 and 4. In particular, the data_size parameter can be changed to an invalid value after the kernel has validated it. This lets userspace force the kernel to access invalid kernel memory. The fix is to ensure that the size has not changed at step 4. This patch shouldn't have a security impact because CAP_SYS_ADMIN is required to run this code, but it should be fixed anyway. Reported-by: Mikulas Patocka Signed-off-by: Alasdair G Kergon Cc: stable@kernel.org Acked-by: Mike Snitzer Acked-by: Mikulas Patocka --- drivers/md/dm-ioctl.c | 8 ++++++++ 1 file changed, 8 insertions(+) -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel Index: linux/drivers/md/dm-ioctl.c =================================================================== --- linux.orig/drivers/md/dm-ioctl.c +++ linux/drivers/md/dm-ioctl.c @@ -1566,6 +1566,14 @@ static int copy_params(struct dm_ioctl _ if (copy_from_user(dmi, user, tmp.data_size)) goto bad; + /* + * Abort if something changed the ioctl data while it was being copied. + */ + if (dmi->data_size != tmp.data_size) { + DMERR("rejecting ioctl: data size modified while processing parameters"); + goto bad; + } + /* Wipe the user buffer so we do not return it to userspace */ if (secure_data && clear_user(user, tmp.data_size)) goto bad;