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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <AM0PR0502MB368378B824556ED230AF10C7BF470@AM0PR0502MB3683.eurprd05.prod.outlook.com>
Date:   Tue, 24 Oct 2017 21:22:27 +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

> 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;
> +	}

...
> +static int devlink_nl_single_param_set(struct sk_buff *msg,
> +				       struct devlink *devlink,
> +				       u32 param, u8 type, void *value)
> +{
> +	const struct devlink_ops *ops = devlink->ops;
> +	struct nlattr *cfgparam_attr;
> +	u8 need_restart;
> +	int err;
> +
> +	/* Now set parameter */
> +	err = ops->perm_config_set(devlink, param, type, value,
> &need_restart);
> +	if (err)
> +		return err;

Likewise