On Wed, Oct 30, 2019 at 02:26:21PM -0500, Eric W. Biederman wrote:

Andrea Righi [off-list ref] writes:

quoted

On Tue, Oct 29, 2019 at 02:02:11PM -0500, Eric W. Biederman wrote:

quoted

Dan Carpenter [off-list ref] writes:

quoted

The "fix" struct has a 2 byte hole after ->ywrapstep and the
"fix = info->fix;" assignment doesn't necessarily clear it.  It depends
on the compiler.

Fixes: 1f5e31d7e55a ("fbmem: don't call copy_from/to_user() with mutex held")
Signed-off-by: Dan Carpenter <redacted>
---
I have 13 more similar places to patch...  I'm not totally sure I
understand all the issues involved.

What I have done in a similar situation with struct siginfo, is that
where the structure first appears I have initialized it with memset,
and then field by field.

Then when the structure is copied I copy the structure with memcpy.

That ensures all of the bytes in the original structure are initialized
and that all of the bytes are copied.

The goal is to avoid memory that has values of the previous users of
that memory region from leaking to userspace.  Which depending on who
the previous user of that memory region is could tell userspace
information about what the kernel is doing that it should not be allowed
to find out.

I tried to trace through where "info" and thus presumably "info->fix" is
coming from and only made it as far as  register_framebuffer.  Given
that I suspect a local memset, and then a field by field copy right
before copy_to_user might be a sound solution.  But ick.  That is a lot
of fields to copy.

I know it might sound quite inefficient, but what about making struct
fb_fix_screeninfo __packed?

This doesn't solve other potential similar issues, but for this
particular case it could be a reasonable and simple fix.

It is part of the user space ABI.  As such you can't move the fields.

Eric

Oh, that's right! Then memset() + memcpy() is probably the best option,
since copying all those fields one by one looks quite ugly to me...

-Andrea