linux-kernel - Re: [PATCH] media: omap3isp: Extract struct group for memcpy() region

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <202106231547.1212335D@keescook>
Date:   Wed, 23 Jun 2021 15:53:38 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc:     Mauro Carvalho Chehab <mchehab+huawei@...nel.org>,
        Arnd Bergmann <arnd@...db.de>,
        "Gustavo A. R. Silva" <gustavoars@...nel.org>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        linux-kernel@...r.kernel.org, linux-media@...r.kernel.org
Subject: Re: [PATCH] media: omap3isp: Extract struct group for memcpy() region

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