Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 23 Jun 2021 15:53:38 -0700
From:   Kees Cook <keescook@chromium.org>
To:     Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc:     Mauro Carvalho Chehab <mchehab+huawei@kernel.org>,
        Arnd Bergmann <arnd@arndb.de>,
        "Gustavo A. R. Silva" <gustavoars@kernel.org>,
        Sakari Ailus <sakari.ailus@linux.intel.com>,
        linux-kernel@vger.kernel.org, linux-media@vger.kernel.org
Subject: Re: [PATCH] media: omap3isp: Extract struct group for memcpy() region
Message-ID: <202106231547.1212335D@keescook>
References: <20210616185938.1225218-1-keescook@chromium.org>
 <YMpUR34kFSbiyi+q@pendragon.ideasonboard.com>
 <202106162119.859E9A80B@keescook>
 <YNOxuybjpLxr6sin@pendragon.ideasonboard.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YNOxuybjpLxr6sin@pendragon.ideasonboard.com>
Precedence: bulk

On Thu, Jun 24, 2021 at 01:12:11AM +0300, Laurent Pinchart wrote:
> Hi Kees,
> 
> On Wed, Jun 16, 2021 at 09:22:23PM -0700, Kees Cook wrote:
> > On Wed, Jun 16, 2021 at 10:43:03PM +0300, Laurent Pinchart wrote:
> > > On Wed, Jun 16, 2021 at 11:59:38AM -0700, Kees Cook wrote:
> > > > Avoid writing past the end of a structure member by wrapping the target
> > > > region in a common named structure. This additionally fixes a
> > > > misalignment of the copy (since the size of "buf" changes between 64-bit
> > > > and 32-bit).
> > > 
> > > Could you have been mislead by the data64 name ? The difference between
> > > omap3isp_stat_data_time and omap3isp_stat_data_time32 is the size of the
> > > ts field, using 32-bit timestamps with legacy userspace, and 64-bit
> > > timestamps with more recent userspace. In both cases we're dealing with
> > > a 32-bit platform, as the omap3isp is not used in any 64-bit ARM SoC.
> > > The size of void __user *buf is thus 4 bytes in all cases, as is __u32
> > > buf.
> > 
> > Ah, yes, that's true. I was hitting this on arm64 builds
> > (CONFIG_COMPILE_TEST) where __user *buf is 64-bit. So, the "additionally
> > fixes" bit above is misleading in the sense that nothing was ever built
> > in the real world like that.
> > 
> > The patch still fixes the compile-time warnings, though.
> 
> I What's the compile-time warning ? I tried compiling the driver for
> ARM64 and didn't notice any.

Sorry, I didn't include the background well enough in the commit log,
but it's part of a tightening of memcpy() under FORTIFY_SOURCE and
also -Warray-bounds enablement. Here's what I've been saying on other
patches (this one was different because it seemed to be just broken
code):

  In preparation for FORTIFY_SOURCE performing compile-time and run-time
  field bounds checking for memcpy(), memmove(), and memset(), avoid
  intentionally writing across neighboring fields.

Anyway, I can carry this until the full series is posted, but I'm still
working through a few more fixes before I send the whole thing. This
patch was one of a handful that didn't have any series dependencies.

-Kees

-- 
Kees Cook