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

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Fri, Sep 11, 2020 at 05:12:49PM +0800, Kent Gibson wrote:
> 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:

...

> > > > +#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.

But in any case it makes code a bit hackish, no?

We calculate the size of one structure and by the fact *partially* copy
another one.

And actually if you look closer to the types, the compat_u64 is not the same as u64.
So, I'm not sure your solution would work in all cases.

-- 
With Best Regards,
Andy Shevchenko