From patchwork Thu Jul 26 19:08:59 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Alex Elder X-Patchwork-Id: 1244261 Return-Path: X-Original-To: patchwork-ceph-devel@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork2.kernel.org (Postfix) with ESMTP id 7E6CCDFFCE for ; Thu, 26 Jul 2012 19:09:14 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752558Ab2GZTJD (ORCPT ); Thu, 26 Jul 2012 15:09:03 -0400 Received: from mail-gg0-f174.google.com ([209.85.161.174]:64259 "EHLO mail-gg0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752472Ab2GZTJB (ORCPT ); Thu, 26 Jul 2012 15:09:01 -0400 Received: by mail-gg0-f174.google.com with SMTP id u4so2340802ggl.19 for ; Thu, 26 Jul 2012 12:09:01 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=message-id:date:from:user-agent:mime-version:to:subject:references :in-reply-to:content-type:content-transfer-encoding :x-gm-message-state; bh=+tmv9Tz0PkKaQgOZwxabGGmdO0iiO4KUvMasNYo1iwo=; b=mQwQ84WHGkbARX27dl8kVN3/0rlWd0gdTIPAOhRGsaaBuRmOQ2ePj5/jOWmvTl9lcc lZOvMtMPoe6OGWoqVwG4imkyoNQ8foeeSAj+H7gu17iNfYnu6y/YP6xnznbdTewzmNgA HALQOBMexML+LuuLjZgdw6IM7+T/ClylIsUQrT15FTu7z8LEv963daYpwKbVtt/sLHqL mFYp2JB+hFtzlPx5Y6XV3eGe9Rs/WOqA25HwpYXOg6VUR3Zgzl2l9KgG0QuDVZeYeuA9 CLJtS+U3jwEUf5vg6/+f8jzanyrKr9bBvgybpzD+GGhkFfAeWihJCBfd4z+M93PwVY+s vphA== Received: by 10.236.145.198 with SMTP id p46mr27411206yhj.49.1343329741195; Thu, 26 Jul 2012 12:09:01 -0700 (PDT) Received: from [172.22.22.4] (c-71-195-31-37.hsd1.mn.comcast.net. [71.195.31.37]) by mx.google.com with ESMTPS id y66sm46591yhi.10.2012.07.26.12.09.00 (version=SSLv3 cipher=OTHER); Thu, 26 Jul 2012 12:09:00 -0700 (PDT) Message-ID: <501195CB.7050902@inktank.com> Date: Thu, 26 Jul 2012 14:08:59 -0500 From: Alex Elder User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:14.0) Gecko/20120714 Thunderbird/14.0 MIME-Version: 1.0 To: ceph-devel@vger.kernel.org Subject: [PATCH 5/7] rbd: fixes in rbd_header_from_disk() References: <50119421.1090702@inktank.com> In-Reply-To: <50119421.1090702@inktank.com> X-Gm-Message-State: ALoCoQk8SKD17o4YniGasYWUX70kBTJHOZKuvDhUtxXCOVhjdw4wqbmoL2QITJHph6dVc6+FtGIm Sender: ceph-devel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: ceph-devel@vger.kernel.org This fixes a few issues in rbd_header_from_disk(): - There is a check intended to catch overflow, but it's wrong in two ways. - First, the type we don't want to overflow is size_t, not unsigned int, and there is now a SIZE_MAX we can use for use with that type. - Second, we're allocating the snapshot ids and snapshot image sizes separately (each has type u64; on disk they grouped together as a rbd_image_header_ondisk structure). So we can use the size of u64 in this overflow check. - If there are no snapshots, then there should be no snapshot names. Enforce this, and issue a warning if we encounter a header with no snapshots but a non-zero snap_names_len. - When saving the snapshot names into the header, be more direct in defining the offset in the on-disk structure from which they're being copied by using "snap_count" rather than "i" in the array index. - If an error occurs, the "snapc" and "snap_names" fields are freed at the end of the function. Make those fields be null pointers after they're freed, to be explicit that they are no longer valid. - Finally, move the definition of the local variable "i" to the innermost scope in which it's needed. Signed-off-by: Alex Elder Reviewed-by: Josh Durgin --- drivers/block/rbd.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) snap_count * sizeof(u64), @@ -509,8 +509,8 @@ static int rbd_header_from_disk(struct rbd_image_header *header, if (!header->snapc) return -ENOMEM; - header->snap_names_len = le64_to_cpu(ondisk->snap_names_len); if (snap_count) { + header->snap_names_len = le64_to_cpu(ondisk->snap_names_len); header->snap_names = kmalloc(header->snap_names_len, GFP_KERNEL); if (!header->snap_names) @@ -520,6 +520,8 @@ static int rbd_header_from_disk(struct rbd_image_header *header, if (!header->snap_sizes) goto err_names; } else { + WARN_ON(ondisk->snap_names_len); + header->snap_names_len = 0; header->snap_names = NULL; header->snap_sizes = NULL; } @@ -544,6 +546,8 @@ static int rbd_header_from_disk(struct rbd_image_header *header, header->total_snaps = snap_count; if (snap_count && allocated_snaps == snap_count) { + int i; + for (i = 0; i < snap_count; i++) { header->snapc->snaps[i] = le64_to_cpu(ondisk->snaps[i].id); @@ -552,7 +556,7 @@ static int rbd_header_from_disk(struct rbd_image_header *header, } /* copy snapshot names */ - memcpy(header->snap_names, &ondisk->snaps[i], + memcpy(header->snap_names, &ondisk->snaps[snap_count], header->snap_names_len); } @@ -562,8 +566,11 @@ err_sizes: kfree(header->snap_sizes); err_names: kfree(header->snap_names); + header->snap_names = NULL; err_snapc: kfree(header->snapc); + header->snapc = NULL; + return -ENOMEM; } diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c index 4584500..3daf8fb 100644 --- a/drivers/block/rbd.c +++ b/drivers/block/rbd.c @@ -494,14 +494,14 @@ static int rbd_header_from_disk(struct rbd_image_header *header, struct rbd_image_header_ondisk *ondisk, u32 allocated_snaps) { - u32 i, snap_count; + u32 snap_count; if (!rbd_dev_ondisk_valid(ondisk)) return -ENXIO; snap_count = le32_to_cpu(ondisk->snap_count); - if (snap_count > (UINT_MAX - sizeof(struct ceph_snap_context)) - / sizeof (*ondisk)) + if (snap_count > (SIZE_MAX - sizeof(struct ceph_snap_context)) + / sizeof (u64)) return -EINVAL; header->snapc = kmalloc(sizeof(struct ceph_snap_context) +