Re: [PATCH v1 1/2] gpiolib: Fix line event handling in syscall compatible mode

[Kent Gibson <warthog618@gmail.com>]

On Fri, Sep 11, 2020 at 11:31:09AM +0300, Andy Shevchenko wrote:
> On Fri, Sep 11, 2020 at 11:05:39AM +0800, Kent Gibson wrote:
> > On Thu, Sep 10, 2020 at 01:19:34PM +0300, Andy Shevchenko wrote:
> > > The introduced line even handling ABI in the commit
> > > 
> > 
> > s/even/event/
> > 
> > >   61f922db7221 ("gpio: userspace ABI for reading GPIO line events")
> > > 
> > > missed the fact that 64-bit kernel may serve for 32-bit applications.
> > > In such case the very first check in the lineevent_read() will fail
> > > due to alignment differences.
> > > 
> > > To workaround this introduce lineeven_to_user() helper which returns actual
> > > size of the structure and copies its content to user if asked.
> > > 
> > 
> > And again here.
> 
> Thanks!
>
> ...
> 
> > > +#ifdef __x86_64__
> > > +	/* i386 has no padding after 'id' */
> > > +	if (in_ia32_syscall()) {
> > > +		struct compat_ge {
> > > +			compat_u64	timestamp __packed;
> > > +			u32		id;
> > > +		} cge;
> > > +
> > > +		if (buf && ge) {
> > > +			cge = (struct compat_ge){ ge->timestamp, ge->id };
> > > +			if (copy_to_user(buf, &cge, sizeof(cge)))
> > > +				return -EFAULT;
> > > +		}
> > > +
> > > +		return sizeof(cge);
> > > +	}
> > > +#endif
> > > +
> > > +	if (buf && ge) {
> > > +		if (copy_to_user(buf, ge, sizeof(*ge)))
> > > +			return -EFAULT;
> > > +	}
> > > +
> > > +	return sizeof(*ge);
> > > +}
> > >  
> > 
> > The dual-purpose nature of this function makes it more complicated than
> > it needs to be.
> > I was going to suggest splitting it into separate functions, but...
> > 
> > Given that struct compat_ge is a strict truncation of struct
> > gpioevent_data, how about reducing this function to just determining the
> > size of the event for user space, say lineevent_user_size(), and
> > replacing sizeof(ge) with that calculcated size throughout
> > lineevent_read()?
> 
> So you mean something like
> 
> 	struct compat_gpioeevent_data {
> 		compat_u64	timestamp;
> 		u32		id;
> 	} __packed;
> 
> #ifdef __x86_64__
> 	/* i386 has no padding after 'id' */
> 	size_t ge_size = in_ia32_syscall() ? sizeof(struct compat_gpioevent_data) : sizeof(struct gpioevent_data);
> #else
> 	size_t ge_size = sizeof(struct gpioevent_data) ;
> #endif
> 
> ?
> 

Pretty much, though I was suggesting keeping it in a helper function,
say lineevent_user_size(), not in lineevent_read() itself, to isolate
all the ugliness in one small place.

So in lineevent_read() you would:

   size_t ge_size = lineevent_user_size();

and then use that to replace all the sizeof(ge) occurrences.

Cheers,
Kent.