netdev - Re: [PATCH net-next RFC] Add NDOs for hardware timestamp get/set

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 31 Mar 2023 11:51:06 -0600
From:   Max Georgiev <glipus@...il.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     kory.maincent@...tlin.com, netdev@...r.kernel.org,
        maxime.chevallier@...tlin.com, vladimir.oltean@....com
Subject: Re: [PATCH net-next RFC] Add NDOs for hardware timestamp get/set

Jakub, thank you for taking a look!

On Thu, Mar 30, 2023 at 11:35 PM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Thu, 30 Mar 2023 22:56:19 -0600 Maxim Georgiev wrote:
> > @@ -1642,6 +1650,10 @@ struct net_device_ops {
> >       ktime_t                 (*ndo_get_tstamp)(struct net_device *dev,
> >                                                 const struct skb_shared_hwtstamps *hwtstamps,
> >                                                 bool cycles);
> > +     int                     (*ndo_hwtstamp_get)(struct net_device *dev,
> > +                                                 struct hwtstamp_config *config);
> > +     int                     (*ndo_hwtstamp_set)(struct net_device *dev,
> > +                                                 struct hwtstamp_config *config);
>
> I wonder if we should pass in
>
>         struct netlink_ext_ack *extack
>
> and maybe another structure for future extensions?
> So we don't have to change the drivers again when we extend uAPI.

Would these two extra parameters be ignored by drivers in this initial
version of NDO hw timestamp API implementation?

>
> >  };
> >
> >  struct xdp_metadata_ops {
> > diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
> > index 5cdbfbf9a7dc..c90fac9a9b2e 100644
> > --- a/net/core/dev_ioctl.c
> > +++ b/net/core/dev_ioctl.c
> > @@ -277,6 +277,39 @@ static int dev_siocbond(struct net_device *dev,
> >       return -EOPNOTSUPP;
> >  }
> >
> > +static int dev_hwtstamp(struct net_device *dev, struct ifreq *ifr,
> > +                     unsigned int cmd)
> > +{
> > +     const struct net_device_ops *ops = dev->netdev_ops;
> > +     int err;
> > +     struct hwtstamp_config config;
>
> nit: reorder int err after config we like lines longest to shortest

Will do. Thank you for pointing it out!

>
> > +
> > +     if ((cmd == SIOCGHWTSTAMP && !ops->ndo_hwtstamp_get) ||
> > +         (cmd == SIOCSHWTSTAMP && !ops->ndo_hwtstamp_set))
> > +             return dev_eth_ioctl(dev, ifr, cmd);
> > +
> > +     err = dsa_ndo_eth_ioctl(dev, ifr, cmd);
> > +     if (err == 0 || err != -EOPNOTSUPP)
> > +             return err;
> > +
> > +     if (!netif_device_present(dev))
> > +             return -ENODEV;
> > +
> > +     if (cmd == SIOCSHWTSTAMP) {
> > +             if (copy_from_user(&config, ifr->ifr_data, sizeof(config)))
> > +                     err = -EFAULT;
> > +             else
> > +                     err = ops->ndo_hwtstamp_set(dev, &config);
> > +     } else if (cmd == SIOCGHWTSTAMP) {
> > +             err = ops->ndo_hwtstamp_get(dev, &config);
> > +     }
> > +
> > +     if (err == 0)
> > +             err = copy_to_user(ifr->ifr_data, &config,
> > +                                sizeof(config)) ? -EFAULT : 0;
>
> nit: just error check each return value, don't try to save LoC

Will do!

>
> > +     return err;
> > +}
> > +
> >  static int dev_siocdevprivate(struct net_device *dev, struct ifreq *ifr,
> >                             void __user *data, unsigned int cmd)
> >  {
> > @@ -391,11 +424,14 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, void __user *data,
> >               rtnl_lock();
> >               return err;
> >
> > +     case SIOCGHWTSTAMP:
> > +             return dev_hwtstamp(dev, ifr, cmd);
> > +
> >       case SIOCSHWTSTAMP:
> >               err = net_hwtstamp_validate(ifr);
> >               if (err)
> >                       return err;
> > -             fallthrough;
> > +             return dev_hwtstamp(dev, ifr, cmd);
>
> Let's refactor this differently, we need net_hwtstamp_validate()
> to run on the same in-kernel copy as we'll send down to the driver.
> If we copy_from_user() twice we may validate a different thing
> than the driver will end up seeing (ToCToU).

Got it, that would be a nice optimization for the NDO execution path!
We still will need a version of net_hwtstamp_validate(struct ifreq *ifr)
to do validation for drivers not implementing ndo_hwtstamp_set().
Also we'll need to implement validation for dsa_ndo_eth_ioctl() which
usually has an empty implementation, but can do something
meaningful depending on kernel configuration if I understand
it correctly. I'm not sure where to insert the validation code for
the DSA code path, would greatly appreciate some advice here.

>
> TBH I'm not sure if keeping GET and SET in a common dev_hwtstamp()
> ends up being beneficial. If we fold in the validation check half
> of the code will be under and if (GET) or if (SET)..

I was on a fence about splitting dev_hwtstamp() into GET and SET versions.
If you believe separate implementations will provide a cleaner implementation
I'll be glad to split them.