netdev - RE: [PATCH net-next v3 01/10] devlink: Add permanent config parameter get/set operations

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 for Android: free password hash cracker in your pocket

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

Message-ID: <AM0PR0502MB3683FC629A6F0465F4D49170BF440@AM0PR0502MB3683.eurprd05.prod.outlook.com>
Date:   Wed, 25 Oct 2017 05:41:06 +0000
From:   Yuval Mintz <yuvalm@...lanox.com>
To:     Steve Lin <steven.lin1@...adcom.com>
CC:     Jiri Pirko <jiri@...lanox.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "michael.chan@...adcom.com" <michael.chan@...adcom.com>,
        "linville@...driver.com" <linville@...driver.com>,
        "gospo@...adcom.com" <gospo@...adcom.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH net-next v3 01/10] devlink: Add permanent config parameter
 get/set operations

> On Tue, Oct 24, 2017 at 5:22 PM, Yuval Mintz <yuvalm@...lanox.com>
> wrote:
> >> Add support for permanent config parameter get/set commands. Used
> >> for persistent device configuration parameters.
> >>
> > ...
> >> +     int (*perm_config_get)(struct devlink *devlink,
> >> +                            enum devlink_perm_config_param param, u8
> >> type,
> >> +                            void *value);
> >> +     int (*perm_config_set)(struct devlink *devlink,
> >> +                            enum devlink_perm_config_param param, u8
> >> type,
> >> +                            void *value, u8 *restart_reqd);
> >>  };
> >> +static int devlink_nl_single_param_get(struct sk_buff *msg,
> >> +                                    struct devlink *devlink,
> >> +                                    u32 param, u8 type)
> >> +{
> >> +     const struct devlink_ops *ops = devlink->ops;
> >> +     struct nlattr *param_attr;
> >> +     void *value;
> >> +     u32 val;
> >> +     int err;
> >> +
> >> +     /* Allocate buffer for parameter value */
> >> +     switch (type) {
> >> +     case NLA_U8:
> >> +             value = kmalloc(sizeof(u8), GFP_KERNEL);
> >> +             break;
> >> +     case NLA_U16:
> >> +             value = kmalloc(sizeof(u16), GFP_KERNEL);
> >> +             break;
> >> +     case NLA_U32:
> >> +             value = kmalloc(sizeof(u32), GFP_KERNEL);
> >> +             break;
> >> +     default:
> >> +             return -EINVAL; /* Unsupported Type */
> >> +     }
> >> +
> >> +     if (!value)
> >> +             return -ENOMEM;
> >> +
> >> +     err = ops->perm_config_get(devlink, param, type, value);
> >> +     if (err)
> >> +             return err;
> >
> > I suspect this logic might be risky - its dependent on the driver to cast the
> > 'value' into the proper type or else, E.g., the following switch might break
> > for BE platforms.
> > Is there any reason to have the devlink <-> driver API be based on void*
> > and not on some typed data [of sufficient size]?
> > ...
> >> +     switch (type) {
> >> +     case NLA_U8:
> >> +             val = *((u8 *)value);
> >> +             if (nla_put_u8(msg, DEVLINK_ATTR_PERM_CONFIG_VALUE,
> >> val))
> >> +                     goto nest_err;
> >> +             break;
> >> +     case NLA_U16:
> >> +             val = *((u16 *)value);
> >> +             if (nla_put_u16(msg,
> >> DEVLINK_ATTR_PERM_CONFIG_VALUE, val))
> >> +                     goto nest_err;
> >> +             break;
> >> +     case NLA_U32:
> >> +             val = *((u32 *)value);
> >> +             if (nla_put_u32(msg,
> >> DEVLINK_ATTR_PERM_CONFIG_VALUE, val))
> >> +                     goto nest_err;
> >> +             break;
> >> +     }
> 
> Why might this break on a BE system?  It's not as though driver will
> be compiled LE and kernel BE or vice versa - as long as driver and
> kernel are same endian-ness, I would think it should be okay?

It depends on the driver implementation to cast your pointer to the right type.
E.g., driver needs to fill in a u8 data in *value for a given parameter.
If the driver casted the pointer to (u8*) everything is fine. But if he casted
it to (u32*) [naïve implementation that doesn't care about the type]
and filled it, then on a LE machine value[0] would contain the data while on
BE value[3] would contain it.


> 
> In general, the issue is that the parameter could be any of the
> netlink types (per Jiri's suggestion to the previous version of this
> patch).  So, we allocate some space, tell the driver the type we're
> expecting (the type argument to the perm_config_get() function), and
> yes, we rely on the driver to write something of the type we request
> to the pointer we provide.  Are you suggesting defining a union of
> U8/U16/U32, and passing a pointer to that for the driver to fill in?

Problem is that the driver-side could always use the biggest data-type
as long as it's working on a LE machine, but that approach would break
if same driver would be tried on a BE machine.
And the developer would have no way of knowing other than via code-review.

> The issue is that whatever the types we support now, we want future
> parameters to be able to be of arbitrary types.  Defining the
> interface to use the void pointer means that some future parameter can
> be of some other type, without having to update all the drivers using
> this API...
> 
> Or did I misunderstand your suggestion?