From patchwork Tue Sep 25 21:58:00 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Sylwester Nawrocki X-Patchwork-Id: 11723 Return-Path: X-Original-To: patchwork@peony.canonical.com Delivered-To: patchwork@peony.canonical.com Received: from fiordland.canonical.com (fiordland.canonical.com [91.189.94.145]) by peony.canonical.com (Postfix) with ESMTP id 2458823EB4 for ; Tue, 25 Sep 2012 21:58:08 +0000 (UTC) Received: from mail-ie0-f180.google.com (mail-ie0-f180.google.com [209.85.223.180]) by fiordland.canonical.com (Postfix) with ESMTP id 6A0E5A18620 for ; Tue, 25 Sep 2012 21:58:07 +0000 (UTC) Received: by ieje10 with SMTP id e10so14192668iej.11 for ; Tue, 25 Sep 2012 14:58:06 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=x-forwarded-to:x-forwarded-for:delivered-to:received-spf :dkim-signature:message-id:date:from:user-agent:mime-version:to:cc :subject:references:in-reply-to:content-type :content-transfer-encoding:x-gm-message-state; bh=Cgj2GqV4LSW6HdHEC2KwczCc0Rgspf8cSWUmaqNs/1E=; b=oQXdUBdwFe01Tv2os3l+ehsgaZTaGuoXvOY+f6L6d7GEMyxiT7QjzkpX5C8qXYOhge i0YredYI0wMMdwVHUX8SF3hkBepeWPy1/eIWUvzpSa6VToq179u+lXUiQPowrhmJDMwA i4g6DKnMRKmKD1eQ6rP42Yz1V2K6N26Jv8CeVfcUuHbMPCzt9iuOryPFDpijEoY2tiEy VsEwiCKItrb1+GcnUaTJFVb5RVeH2IE98lOhLvwG8fYAWKYtSpmFnYh/CW+tzS2fswDQ z79CjoNKb/5+ynbI56xgPgal+I7zsxfpEFRmbqhQgYiHVc/FLoKqqQ0LXOJDpDoseCRq x7+w== Received: by 10.50.217.227 with SMTP id pb3mr9520765igc.28.1348610286665; Tue, 25 Sep 2012 14:58:06 -0700 (PDT) X-Forwarded-To: linaro-patchwork@canonical.com X-Forwarded-For: patch@linaro.org linaro-patchwork@canonical.com Delivered-To: patches@linaro.org Received: by 10.50.184.232 with SMTP id ex8csp320973igc; Tue, 25 Sep 2012 14:58:05 -0700 (PDT) Received: by 10.204.8.194 with SMTP id i2mr2628311bki.117.1348610284623; Tue, 25 Sep 2012 14:58:04 -0700 (PDT) Received: from mail-bk0-f50.google.com (mail-bk0-f50.google.com [209.85.214.50]) by mx.google.com with ESMTPS id ha3si1930244bkc.122.2012.09.25.14.58.03 (version=TLSv1/SSLv3 cipher=OTHER); Tue, 25 Sep 2012 14:58:04 -0700 (PDT) Received-SPF: pass (google.com: domain of sylvester.nawrocki@gmail.com designates 209.85.214.50 as permitted sender) client-ip=209.85.214.50; Authentication-Results: mx.google.com; spf=pass (google.com: domain of sylvester.nawrocki@gmail.com designates 209.85.214.50 as permitted sender) smtp.mail=sylvester.nawrocki@gmail.com; dkim=pass header.i=@gmail.com Received: by bkwq16 with SMTP id q16so2815980bkw.37 for ; Tue, 25 Sep 2012 14:58:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=message-id:date:from:user-agent:mime-version:to:cc:subject :references:in-reply-to:content-type:content-transfer-encoding; bh=Cgj2GqV4LSW6HdHEC2KwczCc0Rgspf8cSWUmaqNs/1E=; b=qt7BGuIMHi79PvXF2Ry12zEX3h0Lwasxc9h+fPGoYl1WpVDfiU/DwOqwXrkF54s+5K 0EMdTSq9BbE7Rlphds45yHHJdNbv6bxa7pL85poPKC8KpW+n9fdneXyQoBtgGe9IHXx7 gVuCbNJJB/tK/hVf0F+snBd8GbBn05orPId5ET+ovo1+zPCeEc0BdnROMMCZ60fa0jXe JCtLJXDH5uj26zd0VLO+UtWBUjwIbROMn1z5itVYvFkn4BTt6hsHAZAZqKpmAXnVxfy+ SeOj9lcrs9D9A6lBEXTR09/rEAzXk7BCEK+VjiF0DuQ040b/DtUPFk5gHn1HCBWUvfGD KVHQ== Received: by 10.204.148.146 with SMTP id p18mr1591877bkv.51.1348610283040; Tue, 25 Sep 2012 14:58:03 -0700 (PDT) Received: from [192.168.1.110] (031011252076.warszawa.vectranet.pl. [31.11.252.76]) by mx.google.com with ESMTPS id e3sm697498bks.7.2012.09.25.14.58.01 (version=SSLv3 cipher=OTHER); Tue, 25 Sep 2012 14:58:01 -0700 (PDT) Message-ID: <506228E8.1040704@gmail.com> Date: Tue, 25 Sep 2012 23:58:00 +0200 From: Sylwester Nawrocki User-Agent: Mozilla/5.0 (X11; Linux i686; rv:12.0) Gecko/20120430 Thunderbird/12.0.1 MIME-Version: 1.0 To: Sachin Kamat CC: linux-media@vger.kernel.org, s.nawrocki@samsung.com, mchehab@infradead.org, patches@linaro.org Subject: Re: [PATCH] [media] s5k6aa: Fix possible NULL pointer dereference References: <1348298907-20791-1-git-send-email-sachin.kamat@linaro.org> In-Reply-To: <1348298907-20791-1-git-send-email-sachin.kamat@linaro.org> X-Gm-Message-State: ALoCoQl3it2YpHsuEg2qpzIPc+P2U8kbyNpqDsQxD1JpOLrpk3xNTbXGrXdAs1bm1ZyY/sYsSRwF Hi Sachin, On 09/22/2012 09:28 AM, Sachin Kamat wrote: > It is previously assumed that 'rect' could be NULL. > Hence add a check to print the members of 'rect' only when it is not > NULL. > > Signed-off-by: Sachin Kamat > --- > drivers/media/i2c/s5k6aa.c | 5 +++-- > 1 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/i2c/s5k6aa.c b/drivers/media/i2c/s5k6aa.c > index 045ca7f..7531edb 100644 > --- a/drivers/media/i2c/s5k6aa.c > +++ b/drivers/media/i2c/s5k6aa.c > @@ -1177,8 +1177,9 @@ static int s5k6aa_get_crop(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh, > > mutex_unlock(&s5k6aa->lock); > > - v4l2_dbg(1, debug, sd, "Current crop rectangle: (%d,%d)/%dx%d\n", > - rect->left, rect->top, rect->width, rect->height); > + if (rect) > + v4l2_dbg(1, debug, sd, "Current crop rectangle: (%d,%d)/%dx%d\n", > + rect->left, rect->top, rect->width, rect->height); > > return 0; > } Thank you for the patch. I would attack this problem form slightly Acked-by: Sachin Kamat different angle though, i.e. I would make sure __s5k6aa_get_crop_rect() always returns valid pointer. There is similar issue in s5k6aa_set_crop(). Since crop->which is already validated in drivers/media/v4l2-core/v4l2-subdev.c and can have only values: V4L2_SUBDEV_FORMAT_TRY, V4L2_SUBDEV_FORMAT_ACTIVE it's safe to do something like: 8<------------------------------------------------------------------------- >From 724aa5f1fefcaca2dee4f75ba960a1f620400e1a Mon Sep 17 00:00:00 2001 From: Sylwester Nawrocki Date: Tue, 25 Sep 2012 23:53:27 +0200 Subject: [PATCH] s5k6aa: Fix potential null pointer dereference Make sure __s5k6aa_get_crop_rect() always returns valid pointer, as it is assumed at the callers. crop->which is already validated when subdev set_crop and get_crop callbacks are called from within the v4l2-core. If it ever happens the crop operations are called directly for some reason in kernel space, with incorrect crop->which argument, just log it with WARN and return reference to the TRY crop. Reported-by: Sachin Kamat Signed-off-by: Sylwester Nawrocki --- drivers/media/i2c/s5k6aa.c | 11 ++++------- 1 files changed, 4 insertions(+), 7 deletions(-) diff --git a/drivers/media/i2c/s5k6aa.c b/drivers/media/i2c/s5k6aa.c index 045ca7f..57cd4fa 100644 --- a/drivers/media/i2c/s5k6aa.c +++ b/drivers/media/i2c/s5k6aa.c @@ -1061,10 +1061,9 @@ __s5k6aa_get_crop_rect(struct s5k6aa *s5k6aa, struct v4l2_subdev_fh *fh, { if (which == V4L2_SUBDEV_FORMAT_ACTIVE) return &s5k6aa->ccd_rect; - if (which == V4L2_SUBDEV_FORMAT_TRY) - return v4l2_subdev_get_try_crop(fh, 0); - return NULL; + WARN_ON(which != V4L2_SUBDEV_FORMAT_TRY); + return v4l2_subdev_get_try_crop(fh, 0); } static void s5k6aa_try_format(struct s5k6aa *s5k6aa, @@ -1169,12 +1168,10 @@ static int s5k6aa_get_crop(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh, struct v4l2_rect *rect; memset(crop->reserved, 0, sizeof(crop->reserved)); - mutex_lock(&s5k6aa->lock); + mutex_lock(&s5k6aa->lock); rect = __s5k6aa_get_crop_rect(s5k6aa, fh, crop->which); - if (rect) - crop->rect = *rect; - + crop->rect = *rect; mutex_unlock(&s5k6aa->lock); v4l2_dbg(1, debug, sd, "Current crop rectangle: (%d,%d)/%dx%d\n",