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

[Kees Cook <keescook@chromium.org>]

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