[RFC] pps: fixing CONFIG_COMPAT issues

* [RFC] pps: fixing CONFIG_COMPAT issues
@ 2016-12-22 20:39 Matt Ranostay
  2016-12-23 15:04 ` Rodolfo Giometti
  0 siblings, 1 reply; 3+ messages in thread
From: Matt Ranostay @ 2016-12-22 20:39 UTC (permalink / raw)
  To: Rodolfo Giometti; +Cc: linux-kernel, David Woodhouse

Rodolfo,

I'd like to get some feedback on what would be an upstreamable patch
series for correcting some issues with a 64-bit kernel and using a
32-bit userspace.

First issue is the compat_ioctl has to be sort of hacked since the
IOCTL defines are using pointer sizes in the macro generation (which
works if you don't mix bit sizes of the kernel and userspace) which
should have been just the struct size originally

#define PPS_GETPARAMS           _IOR('p', 0xa1, struct pps_kparams *)
#define PPS_SETPARAMS           _IOW('p', 0xa2, struct pps_kparams *)
#define PPS_GETCAP              _IOR('p', 0xa3, int *)
#define PPS_FETCH               _IOWR('p', 0xa4, struct pps_fdata *)
#define PPS_KC_BIND             _IOW('p', 0xa5, struct pps_bind_args *)

So basically the workaround we have for that is as follows:

...
static long pps_cdev_compat_ioctl(struct file *file,
               unsigned int cmd, unsigned long arg)
{
       cmd = _IOC(_IOC_DIR(cmd), _IOC_TYPE(cmd), _IOC_NR(cmd), sizeof(void *));
       return pps_cdev_ioctl(file, cmd, arg);
}
...

Now the bigger and more ABI breaking issue is with the padding in
struct pps_ktime (which David Woodhouse has a comment in the header).
Which is worked around by __attribute__((aligned, 4)) which of course
breaks userspace for a precompiled 64-bit binary.

So the issue is that on 32-bit x86 aligns at 4-bytes, but 64-bit is at
8-bytes, so the data ferried from the former userspace to the latter
kernel is incorrect. For instance struct pps_kinfo is padded 4 bytes
more in between info and timeout members.

What would be the best way to fix the padding issue without breaking
userspace applications? Just fixing the alignment with explicit
padding is of course the clean easy way, but bashing the data in
compat_ioctl would avoid breakage.

Thanks,

Matt

^ permalink raw reply	[flat|nested] 3+ messages in thread