From patchwork Mon May 25 14:03:25 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jani Nikula X-Patchwork-Id: 6474781 Return-Path: X-Original-To: patchwork-dri-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 2F0699F399 for ; Mon, 25 May 2015 14:01:28 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 89E032044B for ; Mon, 25 May 2015 14:01:24 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id D142E20458 for ; Mon, 25 May 2015 14:01:20 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 2F4996E480; Mon, 25 May 2015 07:01:19 -0700 (PDT) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id 5EE766E480 for ; Mon, 25 May 2015 07:01:17 -0700 (PDT) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga102.jf.intel.com with ESMTP; 25 May 2015 07:01:16 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.13,491,1427785200"; d="scan'208";a="731355557" Received: from jnikula-mobl.fi.intel.com (HELO localhost) ([10.237.72.152]) by fmsmga002.fm.intel.com with ESMTP; 25 May 2015 07:01:12 -0700 From: Jani Nikula To: Andrey Ryabinin , David Airlie Subject: Re: [PATCH] drm/atomic: fix out of bounds read in for_each_*_in_state helpers In-Reply-To: <87iobgix28.fsf@intel.com> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo References: <1432549784-21966-1-git-send-email-a.ryabinin@samsung.com> <87oal8iyxh.fsf@intel.com> <556322E6.3020500@samsung.com> <87lhgcix4s.fsf@intel.com> <87iobgix28.fsf@intel.com> User-Agent: Notmuch/0.19+112~g77230b0 (http://notmuchmail.org) Emacs/24.4.1 (x86_64-pc-linux-gnu) Date: Mon, 25 May 2015 17:03:25 +0300 Message-ID: <87fv6kiwk2.fsf@intel.com> MIME-Version: 1.0 Cc: Ander Conselvan de Oliveira , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Mon, 25 May 2015, Jani Nikula wrote: > On Mon, 25 May 2015, Jani Nikula wrote: >> On Mon, 25 May 2015, Andrey Ryabinin wrote: >>> On 05/25/2015 04:12 PM, Jani Nikula wrote: >>>> On Mon, 25 May 2015, Andrey Ryabinin wrote: >>>>> for_each_*_in_state validate array index after >>>>> access to array elements, thus perform out of bounds read. >>>>> >>>>> Fix this by validating index in the first place and read >>>>> array element iff validation was successful. >>>>> >>>>> Fixes: df63b9994eaf ("drm/atomic: Add for_each_{connector,crtc,plane}_in_state helper macros") >>>>> Signed-off-by: Andrey Ryabinin >>>>> --- >>>>> include/drm/drm_atomic.h | 24 ++++++++++++------------ >>>>> 1 file changed, 12 insertions(+), 12 deletions(-) >>>>> >>>>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h >>>>> index c1571034..3f13b91 100644 >>>>> --- a/include/drm/drm_atomic.h >>>>> +++ b/include/drm/drm_atomic.h >>>>> @@ -77,26 +77,26 @@ int __must_check drm_atomic_async_commit(struct drm_atomic_state *state); >>>>> >>>>> #define for_each_connector_in_state(state, connector, connector_state, __i) \ >>>>> for ((__i) = 0; \ >>>>> - (connector) = (state)->connectors[__i], \ >>>>> - (connector_state) = (state)->connector_states[__i], \ >>>>> - (__i) < (state)->num_connector; \ >>>>> + (__i) < (state)->num_connector && \ >>>>> + ((connector) = (state)->connectors[__i], \ >>>>> + (connector_state) = (state)->connector_states[__i], 1); \ >>>> >>>> This will stop at the first NULL connector/connector_state. Similarly >>>> for the loops below. >>>> >>> >>> This will stop iff (__i) >= (state)->num_connector, because the result of expression: >>> ((connector) = (state)->connectors[__i], (connector_state) = (state)->connector_states[__i], 1) >>> is always 1. >> >> Why do you think it'll always be 1? > > That might be because there's the 1 at the end. *blush*. > > I do wonder if this is too subtle in general, or if it's just too subtle > for me. So in the mean time, I was looking at doing the below, not because of this patch or the bug it fixes, but because I think the construct for (...) \ if (...) in a for_something() style macro is a dangling else disaster waiting to happen. It's a bit tedious, I admit, and apparently makes some gcc versions whine about using uninitialized variables, because they're not smart enough to realize the pointers are initialized when the loop condition is met. BR, Jani. diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index e89db0c377ba..eb81f5930a8c 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -134,28 +134,70 @@ int __must_check drm_atomic_check_only(struct drm_atomic_state *state); int __must_check drm_atomic_commit(struct drm_atomic_state *state); int __must_check drm_atomic_async_commit(struct drm_atomic_state *state); +static inline int next_connector(struct drm_connector **pconnector, + struct drm_connector **connectors, + struct drm_connector_state **pstate, + struct drm_connector_state **states, + int index, int max) +{ + while (index < max && (connectors[index] == NULL || states[index] == NULL)) + index++; + + if (index < max) { + *pconnector = connectors[index]; + *pstate = states[index]; + } + + return index; +} + +static inline int next_crtc(struct drm_crtc **pcrtc, + struct drm_crtc **crtcs, + struct drm_crtc_state **pstate, + struct drm_crtc_state **states, + int index, int max) +{ + while (index < max && (crtcs[index] == NULL || states[index] == NULL)) + index++; + + if (index < max) { + *pcrtc = crtcs[index]; + *pstate = states[index]; + } + + return index; +} + +static inline int next_plane(struct drm_plane **pplane, + struct drm_plane **planes, + struct drm_plane_state **pstate, + struct drm_plane_state **states, + int index, int max) +{ + while (index < max && (planes[index] == NULL || states[index] == NULL)) + index++; + + if (index < max) { + *pplane = planes[index]; + *pstate = states[index]; + } + + return index; +} + #define for_each_connector_in_state(state, connector, connector_state, __i) \ - for ((__i) = 0; \ - (connector) = (state)->connectors[__i], \ - (connector_state) = (state)->connector_states[__i], \ + for ((__i) = next_connector(&(connector), (state)->connectors, &(connector_state), (state)->connector_states, 0, (state)->num_connector); \ (__i) < (state)->num_connector; \ - (__i)++) \ - if (connector) - -#define for_each_crtc_in_state(state, crtc, crtc_state, __i) \ - for ((__i) = 0; \ - (crtc) = (state)->crtcs[__i], \ - (crtc_state) = (state)->crtc_states[__i], \ - (__i) < (state)->dev->mode_config.num_crtc; \ - (__i)++) \ - if (crtc_state) - -#define for_each_plane_in_state(state, plane, plane_state, __i) \ - for ((__i) = 0; \ - (plane) = (state)->planes[__i], \ - (plane_state) = (state)->plane_states[__i], \ - (__i) < (state)->dev->mode_config.num_total_plane; \ - (__i)++) \ - if (plane_state) + (__i) = next_connector(&(connector), (state)->connectors, &(connector_state), (state)->connector_states, (__i), (state)->num_connector)) + +#define for_each_crtc_in_state(state, crtc, crtc_state, __i) \ + for ((__i) = next_crtc(&(crtc), (state)->crtcs, &(crtc_state), (state)->crtc_states, 0, (state)->dev->mode_config.num_crtc); \ + (__i) < (state)->dev->mode_config.num_crtc; \ + (__i) = next_crtc(&crtc, (state)->crtcs, &(crtc_state), (state)->crtc_states, (__i), (state)->dev->mode_config.num_crtc)) + +#define for_each_plane_in_state(state, plane, plane_state, __i) \ + for ((__i) = next_plane(&(plane), (state)->planes, &(plane_state), (state)->plane_states, 0, (state)->dev->mode_config.num_total_plane); \ + (__i) < (state)->dev->mode_config.num_total_plane; \ + (__i) = next_plane(&(plane), (state)->planes, &(plane_state), (state)->plane_states, (__i), (state)->dev->mode_config.num_total_plane)) #endif /* DRM_ATOMIC_H_ */