| Message ID | 20190107152229.GA17925@asgard.redhat.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net,1/2] ptp: check that rsv field is zero in struct ptp_sys_offset_extended | expand |
From: Eugene Syromiatnikov <esyr@redhat.com> Date: Mon, 7 Jan 2019 16:22:29 +0100 > Otherwise it is impossible to use it for something else, as it will break > userspace that puts garbage there. > > The same check should be done in other structures, but the fact that > data in reserved fields is ignored is already part of the kernel ABI. > > Signed-off-by: Eugene Syromiatnikov <esyr@redhat.com> I think the opportunity to enforce this has passed and you will break userspace by doing this.
On Mon, Jan 07, 2019 at 08:29:38AM -0800, David Miller wrote: > From: Eugene Syromiatnikov <esyr@redhat.com> > Date: Mon, 7 Jan 2019 16:22:29 +0100 > > > Otherwise it is impossible to use it for something else, as it will break > > userspace that puts garbage there. > > > > The same check should be done in other structures, but the fact that > > data in reserved fields is ignored is already part of the kernel ABI. > > > > Signed-off-by: Eugene Syromiatnikov <esyr@redhat.com> > > I think the opportunity to enforce this has passed and you will break > userspace by doing this. FWIW, this ioctl wasn't present in a stable Linux release yet and the two applications that I know that support it both zero the rsv field, so I think at least this patch is very unlikely to break anything. Anyway, thanks to Eugene for catching the issues.
From: Miroslav Lichvar <mlichvar@redhat.com> Date: Mon, 7 Jan 2019 17:57:41 +0100 > On Mon, Jan 07, 2019 at 08:29:38AM -0800, David Miller wrote: >> From: Eugene Syromiatnikov <esyr@redhat.com> >> Date: Mon, 7 Jan 2019 16:22:29 +0100 >> >> > Otherwise it is impossible to use it for something else, as it will break >> > userspace that puts garbage there. >> > >> > The same check should be done in other structures, but the fact that >> > data in reserved fields is ignored is already part of the kernel ABI. >> > >> > Signed-off-by: Eugene Syromiatnikov <esyr@redhat.com> >> >> I think the opportunity to enforce this has passed and you will break >> userspace by doing this. > > FWIW, this ioctl wasn't present in a stable Linux release yet and the > two applications that I know that support it both zero the rsv field, > so I think at least this patch is very unlikely to break anything. > > Anyway, thanks to Eugene for catching the issues. Oh, this ioctl went into this merge window didn't it. Ok. I'll keep reviewing this then.
On Mon, Jan 07, 2019 at 08:29:38AM -0800, David Miller wrote: > From: Eugene Syromiatnikov <esyr@redhat.com> > Date: Mon, 7 Jan 2019 16:22:29 +0100 > > > Otherwise it is impossible to use it for something else, as it will break > > userspace that puts garbage there. > > > > The same check should be done in other structures, but the fact that > > data in reserved fields is ignored is already part of the kernel ABI. > > > > Signed-off-by: Eugene Syromiatnikov <esyr@redhat.com> > > I think the opportunity to enforce this has passed and you will break > userspace by doing this. Does this seriously mean that the 'rsv' field in struct ptp_extts_request { unsigned int index; /* Which channel to configure. */ unsigned int flags; /* Bit field for PTP_xxx flags. */ unsigned int rsv[2]; /* Reserved for future use. */ }; can never be extended with some semantics? Thanks, Richard
On Mon, Jan 07, 2019 at 09:19:23PM -0800, Richard Cochran wrote: > On Mon, Jan 07, 2019 at 08:29:38AM -0800, David Miller wrote: > > From: Eugene Syromiatnikov <esyr@redhat.com> > > Date: Mon, 7 Jan 2019 16:22:29 +0100 > > > > > Otherwise it is impossible to use it for something else, as it will break > > > userspace that puts garbage there. > > > > > > The same check should be done in other structures, but the fact that > > > data in reserved fields is ignored is already part of the kernel ABI. > > > > > > Signed-off-by: Eugene Syromiatnikov <esyr@redhat.com> > > > > I think the opportunity to enforce this has passed and you will break > > userspace by doing this. > > Does this seriously mean that the 'rsv' field in > > struct ptp_extts_request { > unsigned int index; /* Which channel to configure. */ > unsigned int flags; /* Bit field for PTP_xxx flags. */ > unsigned int rsv[2]; /* Reserved for future use. */ > }; > > can never be extended with some semantics? Yes[*], since there's no check for garbage in both unused flags bits and rsv values. The same for ptp_perout_request, ptp_sys_offset, ptp_pin_desc in PTP_PIN_SETFUNC, and to some extent for ptp_sys_offset_precise, ptp_clock_caps, and ptp_pin_desc in PTP_PIN_GETFUNC (all newly added data has to be non-zero there). See also [1][2]. It can be worked around by adding new ioctl commands that operate on the same structures, but also perform proper checks, though. [*] Well, it could be extended with some data that is written from kernel to user space, but, again, it is not possible due to the fact that no new flags can be added there and it is an _IOW and not _IOWR command. [1] https://www.kernel.org/doc/Documentation/process/adding-syscalls.rsti "Designing the API: Planning for Extension" [2] http://man7.org/conf/lcna2015/designing_linux_kernel_APIs-LCNA_2015-Kerrisk.pdf > Thanks, > Richard
> -----Original Message----- > From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On > Behalf Of Eugene Syromiatnikov > Sent: Tuesday, January 08, 2019 6:38 AM > To: Richard Cochran <richardcochran@gmail.com> > Cc: David Miller <davem@davemloft.net>; mlichvar@redhat.com; > netdev@vger.kernel.org; Keller, Jacob E <jacob.e.keller@intel.com>; > mtosatti@redhat.com > Subject: Re: [PATCH net 1/2] ptp: check that rsv field is zero in struct > ptp_sys_offset_extended > > On Mon, Jan 07, 2019 at 09:19:23PM -0800, Richard Cochran wrote: > > On Mon, Jan 07, 2019 at 08:29:38AM -0800, David Miller wrote: > > > From: Eugene Syromiatnikov <esyr@redhat.com> > > > Date: Mon, 7 Jan 2019 16:22:29 +0100 > > > > > > > Otherwise it is impossible to use it for something else, as it will break > > > > userspace that puts garbage there. > > > > > > > > The same check should be done in other structures, but the fact that > > > > data in reserved fields is ignored is already part of the kernel ABI. > > > > > > > > Signed-off-by: Eugene Syromiatnikov <esyr@redhat.com> > > > > > > I think the opportunity to enforce this has passed and you will break > > > userspace by doing this. > > > > Does this seriously mean that the 'rsv' field in > > > > struct ptp_extts_request { > > unsigned int index; /* Which channel to configure. */ > > unsigned int flags; /* Bit field for PTP_xxx flags. */ > > unsigned int rsv[2]; /* Reserved for future use. */ > > }; > > > > can never be extended with some semantics? > > Yes[*], since there's no check for garbage in both unused flags bits and rsv > values. The same for ptp_perout_request, ptp_sys_offset, ptp_pin_desc in > PTP_PIN_SETFUNC, and to some extent for ptp_sys_offset_precise, ptp_clock_caps, > and ptp_pin_desc in PTP_PIN_GETFUNC (all newly added data has to be > non-zero there). See also [1][2]. > > It can be worked around by adding new ioctl commands that operate on the > same structures, but also perform proper checks, though. > So if/when we want to use these fields, this is what we'd have to do... Is it worth doing that sooner rather than later? i.e. so that the new ioctl command is available for userspace to update to? Thanks, Jake
diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c index 797fab3..7cbea79 100644 --- a/drivers/ptp/ptp_chardev.c +++ b/drivers/ptp/ptp_chardev.c @@ -224,7 +224,8 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg) extoff = NULL; break; } - if (extoff->n_samples > PTP_MAX_SAMPLES) { + if (extoff->n_samples > PTP_MAX_SAMPLES + || extoff->rsv[0] || extoff->rsv[1] || extoff->rsv[2]) { err = -EINVAL; break; }
Otherwise it is impossible to use it for something else, as it will break userspace that puts garbage there. The same check should be done in other structures, but the fact that data in reserved fields is ignored is already part of the kernel ABI. Signed-off-by: Eugene Syromiatnikov <esyr@redhat.com> --- drivers/ptp/ptp_chardev.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)