| 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
| ||
|
Message-ID: <a497f7d7-51c9-849a-49a3-0a509476e4f3@oracle.com>
Date: Fri, 3 Feb 2023 11:32:32 -0800
From: Si-Wei Liu <si-wei.liu@...cle.com>
To: "Michael S. Tsirkin" <mst@...hat.com>
Cc: jasowang@...hat.com, parav@...dia.com, elic@...dia.com,
virtualization@...ts.linux-foundation.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 5/7] vdpa: validate device feature provisioning against
supported class
On 2/3/2023 12:09 AM, Michael S. Tsirkin wrote:
> On Tue, Jan 31, 2023 at 03:22:23PM -0800, Si-Wei Liu wrote:
>> Today when device features are explicitly provisioned, the features
>> user supplied may contain device class specific features that are
>> not supported by the parent managment device. On the other hand,
>> when parent managment device supports more than one class, the
>> device features to provision may be ambiguous if none of the class
>> specific attributes is provided at the same time. Validate these
>> cases and prompt appropriate user errors accordingly.
>>
>> Signed-off-by: Si-Wei Liu <si-wei.liu@...cle.com>
>> ---
>> drivers/vdpa/vdpa.c | 51 ++++++++++++++++++++++++++++++++++++++++++---------
>> 1 file changed, 42 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>> index 1eba978..35a72d6 100644
>> --- a/drivers/vdpa/vdpa.c
>> +++ b/drivers/vdpa/vdpa.c
>> @@ -460,12 +460,30 @@ static int vdpa_nl_mgmtdev_handle_fill(struct sk_buff *msg, const struct vdpa_mg
>> return 0;
>> }
>>
>> +static u64 vdpa_mgmtdev_get_classes(const struct vdpa_mgmt_dev *mdev,
>> + unsigned int *nclasses)
> given max value is apparently 64 how important is it that it's unsigned?
> Just make it an int.
Not sure I understand what you really meant/want. I consider making
unsigned is a (good) habit of keeping data type consistent to ensure
non-negative value is returned so callers run free of worry for false
complaint from (dumb) static code analyzer, and the next caller can
promptly interpret possible range of return value just from the function
prototype without having to dig into internals implemented by another
author. If your intent is to limit the range I can certainly make it an
unsigned char or u8, otherwise I don't get why you think int is better
than unsigned int. Does it not conform to the coding standard documented
somewhere?
> Also I'd return u64 through a pointer too for consistency.
Here the intent is to make the class bitmask number mandatory to return,
while the number of classes returned can be optional. If there's future
need to optionally return bitmask, the code can be revisited for sure.
For now I'd just keep it this way for simplicity and readability.
>> +{
>> + u64 supported_classes = 0;
>> + unsigned int n = 0;
>> + int i = 0;
>> +
>> + while (mdev->id_table[i].device) {
>> + if (mdev->id_table[i].device <= 63) {
>> + supported_classes |= BIT_ULL(mdev->id_table[i].device);
>> + n++;
>> + }
>> + i++;
>> + }
>
> Better as a for loop. I know you are just moving code if you
> want to make it very clear it's a refactoring split
> as a separate patch, but ok anyway.
I can make it a for loop. Generally if just moving code people tend to
keep the original code as-is without refactoring too much (separate
patch needed). But for this simple rewrite it might be okay, it's your call.
Thanks,
-Siwei
>
>> + if (nclasses)
>> + *nclasses = n;
>> +
>> + return supported_classes;
>> +}
>> +
>> static int vdpa_mgmtdev_fill(const struct vdpa_mgmt_dev *mdev, struct sk_buff *msg,
>> u32 portid, u32 seq, int flags)
>> {
>> - u64 supported_classes = 0;
>> void *hdr;
>> - int i = 0;
>> int err;
>>
>> hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags, VDPA_CMD_MGMTDEV_NEW);
>> @@ -475,14 +493,9 @@ static int vdpa_mgmtdev_fill(const struct vdpa_mgmt_dev *mdev, struct sk_buff *m
>> if (err)
>> goto msg_err;
>>
>> - while (mdev->id_table[i].device) {
>> - if (mdev->id_table[i].device <= 63)
>> - supported_classes |= BIT_ULL(mdev->id_table[i].device);
>> - i++;
>> - }
>> -
>> if (nla_put_u64_64bit(msg, VDPA_ATTR_MGMTDEV_SUPPORTED_CLASSES,
>> - supported_classes, VDPA_ATTR_UNSPEC)) {
>> + vdpa_mgmtdev_get_classes(mdev, NULL),
>> + VDPA_ATTR_UNSPEC)) {
>> err = -EMSGSIZE;
>> goto msg_err;
>> }
>> @@ -571,8 +584,10 @@ static int vdpa_nl_cmd_dev_add_set_doit(struct sk_buff *skb, struct genl_info *i
>> struct vdpa_dev_set_config config = {};
>> struct nlattr **nl_attrs = info->attrs;
>> struct vdpa_mgmt_dev *mdev;
>> + unsigned int ncls = 0;
>> const u8 *macaddr;
>> const char *name;
>> + u64 classes;
>> int err = 0;
>>
>> if (!info->attrs[VDPA_ATTR_DEV_NAME])
>> @@ -649,6 +664,24 @@ static int vdpa_nl_cmd_dev_add_set_doit(struct sk_buff *skb, struct genl_info *i
>> goto err;
>> }
>>
>> + classes = vdpa_mgmtdev_get_classes(mdev, &ncls);
>> + if (config.mask & VDPA_DEV_NET_ATTRS_MASK &&
>> + !(classes & BIT_ULL(VIRTIO_ID_NET))) {
>> + NL_SET_ERR_MSG_MOD(info->extack,
>> + "Network class attributes provided on unsupported management device");
>> + err = -EINVAL;
>> + goto err;
>> + }
>> + if (!(config.mask & VDPA_DEV_NET_ATTRS_MASK) &&
>> + config.mask & BIT_ULL(VDPA_ATTR_DEV_FEATURES) &&
>> + classes & BIT_ULL(VIRTIO_ID_NET) && ncls > 1 &&
>> + config.device_features & VIRTIO_DEVICE_F_MASK) {
>> + NL_SET_ERR_MSG_MOD(info->extack,
>> + "Management device supports multi-class while device features specified are ambiguous");
>> + err = -EINVAL;
>> + goto err;
>> + }
>> +
>> err = mdev->ops->dev_add(mdev, name, &config);
>> err:
>> up_write(&vdpa_dev_lock);
>> --
>> 1.8.3.1
Powered by blists - more mailing lists