[1/2] raw/ntb: check spad user index
Checks
Commit Message
From: Chengwen Feng <fengchengwen@huawei.com>
This patch adds checking spad user index validity when set or get attr.
Fixes: 277310027965 ("raw/ntb: introduce NTB raw device driver")
Cc: stable@dpdk.org
Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
drivers/raw/ntb/ntb.c | 8 ++++++++
1 file changed, 8 insertions(+)
Comments
Hi
> -----Original Message-----
> From: Min Hu (Connor) <humin29@huawei.com>
> Sent: Wednesday, April 21, 2021 10:08
> To: dev@dpdk.org
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Li, Xiaoyun <xiaoyun.li@intel.com>;
> Wu, Jingjing <jingjing.wu@intel.com>
> Subject: [PATCH 1/2] raw/ntb: check spad user index
>
> From: Chengwen Feng <fengchengwen@huawei.com>
>
> This patch adds checking spad user index validity when set or get attr.
>
> Fixes: 277310027965 ("raw/ntb: introduce NTB raw device driver")
> Cc: stable@dpdk.org
>
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
> drivers/raw/ntb/ntb.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/raw/ntb/ntb.c b/drivers/raw/ntb/ntb.c index
> 6dd213e..0f0e3f2 100644
> --- a/drivers/raw/ntb/ntb.c
> +++ b/drivers/raw/ntb/ntb.c
> @@ -1080,6 +1080,10 @@ ntb_attr_set(struct rte_rawdev *dev, const char
> *attr_name,
> if (hw->ntb_ops->spad_write == NULL)
> return -ENOTSUP;
> index = atoi(&attr_name[NTB_SPAD_USER_LEN]);
> + if (index < 0 || index >= NTB_SPAD_USER_MAX_NUM) {
> + NTB_LOG(ERR, "Invalid attribute (%s)", attr_name);
> + return -EINVAL;
> + }
It's unnecessary. The value will be checked in intel_ntb_spad_write(). There will be error remind in that.
> (*hw->ntb_ops->spad_write)(dev, hw->spad_user_list[index],
> 1, attr_value);
> NTB_LOG(DEBUG, "Set attribute (%s) Value (%" PRIu64 ")", @@
> -1174,6 +1178,10 @@ ntb_attr_get(struct rte_rawdev *dev, const char
> *attr_name,
> if (hw->ntb_ops->spad_read == NULL)
> return -ENOTSUP;
> index = atoi(&attr_name[NTB_SPAD_USER_LEN]);
> + if (index < 0 || index >= NTB_SPAD_USER_MAX_NUM) {
> + NTB_LOG(ERR, "Attribute (%s) out of range",
> attr_name);
> + return -EINVAL;
> + }
Same as above.
> *attr_value = (*hw->ntb_ops->spad_read)(dev,
> hw->spad_user_list[index], 0);
> NTB_LOG(DEBUG, "Attribute (%s) Value (%" PRIu64 ")",
> --
> 2.7.4
On 2021/4/21 11:31, Li, Xiaoyun wrote:
> Hi
>
>> -----Original Message-----
>> From: Min Hu (Connor) <humin29@huawei.com>
>> Sent: Wednesday, April 21, 2021 10:08
>> To: dev@dpdk.org
>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Li, Xiaoyun <xiaoyun.li@intel.com>;
>> Wu, Jingjing <jingjing.wu@intel.com>
>> Subject: [PATCH 1/2] raw/ntb: check spad user index
>>
>> From: Chengwen Feng <fengchengwen@huawei.com>
>>
>> This patch adds checking spad user index validity when set or get attr.
>>
>> Fixes: 277310027965 ("raw/ntb: introduce NTB raw device driver")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>> ---
>> drivers/raw/ntb/ntb.c | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/raw/ntb/ntb.c b/drivers/raw/ntb/ntb.c index
>> 6dd213e..0f0e3f2 100644
>> --- a/drivers/raw/ntb/ntb.c
>> +++ b/drivers/raw/ntb/ntb.c
>> @@ -1080,6 +1080,10 @@ ntb_attr_set(struct rte_rawdev *dev, const char
>> *attr_name,
>> if (hw->ntb_ops->spad_write == NULL)
>> return -ENOTSUP;
>> index = atoi(&attr_name[NTB_SPAD_USER_LEN]);
>> + if (index < 0 || index >= NTB_SPAD_USER_MAX_NUM) {
>> + NTB_LOG(ERR, "Invalid attribute (%s)", attr_name);
>> + return -EINVAL;
>> + }
>
> It's unnecessary. The value will be checked in intel_ntb_spad_write(). There will be error remind in that.
index maybe large, and then hw->spad_user_list[index] may lead to segmentation fault.
so the verification is required.
>
>> (*hw->ntb_ops->spad_write)(dev, hw->spad_user_list[index],
>> 1, attr_value);
>> NTB_LOG(DEBUG, "Set attribute (%s) Value (%" PRIu64 ")", @@
>> -1174,6 +1178,10 @@ ntb_attr_get(struct rte_rawdev *dev, const char
>> *attr_name,
>> if (hw->ntb_ops->spad_read == NULL)
>> return -ENOTSUP;
>> index = atoi(&attr_name[NTB_SPAD_USER_LEN]);
>> + if (index < 0 || index >= NTB_SPAD_USER_MAX_NUM) {
>> + NTB_LOG(ERR, "Attribute (%s) out of range",
>> attr_name);
>> + return -EINVAL;
>> + }
>
> Same as above.
>
>> *attr_value = (*hw->ntb_ops->spad_read)(dev,
>> hw->spad_user_list[index], 0);
>> NTB_LOG(DEBUG, "Attribute (%s) Value (%" PRIu64 ")",
>> --
>> 2.7.4
>
>
> .
>
Hi, xiaoyun,
在 2021/4/21 11:31, Li, Xiaoyun 写道:
> Hi
>
>> -----Original Message-----
>> From: Min Hu (Connor) <humin29@huawei.com>
>> Sent: Wednesday, April 21, 2021 10:08
>> To: dev@dpdk.org
>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Li, Xiaoyun <xiaoyun.li@intel.com>;
>> Wu, Jingjing <jingjing.wu@intel.com>
>> Subject: [PATCH 1/2] raw/ntb: check spad user index
>>
>> From: Chengwen Feng <fengchengwen@huawei.com>
>>
>> This patch adds checking spad user index validity when set or get attr.
>>
>> Fixes: 277310027965 ("raw/ntb: introduce NTB raw device driver")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>> ---
>> drivers/raw/ntb/ntb.c | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/raw/ntb/ntb.c b/drivers/raw/ntb/ntb.c index
>> 6dd213e..0f0e3f2 100644
>> --- a/drivers/raw/ntb/ntb.c
>> +++ b/drivers/raw/ntb/ntb.c
>> @@ -1080,6 +1080,10 @@ ntb_attr_set(struct rte_rawdev *dev, const char
>> *attr_name,
>> if (hw->ntb_ops->spad_write == NULL)
>> return -ENOTSUP;
>> index = atoi(&attr_name[NTB_SPAD_USER_LEN]);
>> + if (index < 0 || index >= NTB_SPAD_USER_MAX_NUM) {
>> + NTB_LOG(ERR, "Invalid attribute (%s)", attr_name);
>> + return -EINVAL;
>> + }
>
> It's unnecessary. The value will be checked in intel_ntb_spad_write(). There will be error remind in that.
>
Nothing to do with intel_ntb_spad_write. If index is no checked,
hw->spad_user_list[index] may be be out of memory and result in
segmentation default.
>> (*hw->ntb_ops->spad_write)(dev, hw->spad_user_list[index],
>> 1, attr_value);
>> NTB_LOG(DEBUG, "Set attribute (%s) Value (%" PRIu64 ")", @@
>> -1174,6 +1178,10 @@ ntb_attr_get(struct rte_rawdev *dev, const char
>> *attr_name,
>> if (hw->ntb_ops->spad_read == NULL)
>> return -ENOTSUP;
>> index = atoi(&attr_name[NTB_SPAD_USER_LEN]);
>> + if (index < 0 || index >= NTB_SPAD_USER_MAX_NUM) {
>> + NTB_LOG(ERR, "Attribute (%s) out of range",
>> attr_name);
>> + return -EINVAL;
>> + }
>
> Same as above.
>
>> *attr_value = (*hw->ntb_ops->spad_read)(dev,
>> hw->spad_user_list[index], 0);
>> NTB_LOG(DEBUG, "Attribute (%s) Value (%" PRIu64 ")",
>> --
>> 2.7.4
>
> .
>
> -----Original Message-----
> From: Min Hu (Connor) <humin29@huawei.com>
> Sent: Wednesday, April 21, 2021 12:37
> To: Li, Xiaoyun <xiaoyun.li@intel.com>; dev@dpdk.org
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>
> Subject: Re: [PATCH 1/2] raw/ntb: check spad user index
>
> Hi, xiaoyun,
>
> 在 2021/4/21 11:31, Li, Xiaoyun 写道:
> > Hi
> >
> >> -----Original Message-----
> >> From: Min Hu (Connor) <humin29@huawei.com>
> >> Sent: Wednesday, April 21, 2021 10:08
> >> To: dev@dpdk.org
> >> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Li, Xiaoyun
> >> <xiaoyun.li@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>
> >> Subject: [PATCH 1/2] raw/ntb: check spad user index
> >>
> >> From: Chengwen Feng <fengchengwen@huawei.com>
> >>
> >> This patch adds checking spad user index validity when set or get attr.
> >>
> >> Fixes: 277310027965 ("raw/ntb: introduce NTB raw device driver")
> >> Cc: stable@dpdk.org
> >>
> >> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> >> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> >> ---
> >> drivers/raw/ntb/ntb.c | 8 ++++++++
> >> 1 file changed, 8 insertions(+)
> >>
> >> diff --git a/drivers/raw/ntb/ntb.c b/drivers/raw/ntb/ntb.c index
> >> 6dd213e..0f0e3f2 100644
> >> --- a/drivers/raw/ntb/ntb.c
> >> +++ b/drivers/raw/ntb/ntb.c
> >> @@ -1080,6 +1080,10 @@ ntb_attr_set(struct rte_rawdev *dev, const
> >> char *attr_name,
> >> if (hw->ntb_ops->spad_write == NULL)
> >> return -ENOTSUP;
> >> index = atoi(&attr_name[NTB_SPAD_USER_LEN]);
> >> + if (index < 0 || index >= NTB_SPAD_USER_MAX_NUM) {
> >> + NTB_LOG(ERR, "Invalid attribute (%s)", attr_name);
> >> + return -EINVAL;
> >> + }
> >
> > It's unnecessary. The value will be checked in intel_ntb_spad_write(). There
> will be error remind in that.
> >
> Nothing to do with intel_ntb_spad_write. If index is no checked,
> hw->spad_user_list[index] may be be out of memory and result in
> segmentation default.
Are you using this driver externally? Or you just check everything in DPDK.
This is actually only used for ntb example in file trans mode. And only 0 and 1 are used for index.
>
>
> >> (*hw->ntb_ops->spad_write)(dev, hw->spad_user_list[index],
> >> 1, attr_value);
> >> NTB_LOG(DEBUG, "Set attribute (%s) Value (%" PRIu64 ")", @@
> >> -1174,6 +1178,10 @@ ntb_attr_get(struct rte_rawdev *dev, const char
> >> *attr_name,
> >> if (hw->ntb_ops->spad_read == NULL)
> >> return -ENOTSUP;
> >> index = atoi(&attr_name[NTB_SPAD_USER_LEN]);
> >> + if (index < 0 || index >= NTB_SPAD_USER_MAX_NUM) {
> >> + NTB_LOG(ERR, "Attribute (%s) out of range",
> >> attr_name);
> >> + return -EINVAL;
> >> + }
> >
> > Same as above.
> >
> >> *attr_value = (*hw->ntb_ops->spad_read)(dev,
> >> hw->spad_user_list[index], 0);
> >> NTB_LOG(DEBUG, "Attribute (%s) Value (%" PRIu64 ")",
> >> --
> >> 2.7.4
> >
> > .
> >
在 2021/4/21 12:54, Li, Xiaoyun 写道:
>
>
>> -----Original Message-----
>> From: Min Hu (Connor) <humin29@huawei.com>
>> Sent: Wednesday, April 21, 2021 12:37
>> To: Li, Xiaoyun <xiaoyun.li@intel.com>; dev@dpdk.org
>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>
>> Subject: Re: [PATCH 1/2] raw/ntb: check spad user index
>>
>> Hi, xiaoyun,
>>
>> 在 2021/4/21 11:31, Li, Xiaoyun 写道:
>>> Hi
>>>
>>>> -----Original Message-----
>>>> From: Min Hu (Connor) <humin29@huawei.com>
>>>> Sent: Wednesday, April 21, 2021 10:08
>>>> To: dev@dpdk.org
>>>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Li, Xiaoyun
>>>> <xiaoyun.li@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>
>>>> Subject: [PATCH 1/2] raw/ntb: check spad user index
>>>>
>>>> From: Chengwen Feng <fengchengwen@huawei.com>
>>>>
>>>> This patch adds checking spad user index validity when set or get attr.
>>>>
>>>> Fixes: 277310027965 ("raw/ntb: introduce NTB raw device driver")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>>> ---
>>>> drivers/raw/ntb/ntb.c | 8 ++++++++
>>>> 1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/drivers/raw/ntb/ntb.c b/drivers/raw/ntb/ntb.c index
>>>> 6dd213e..0f0e3f2 100644
>>>> --- a/drivers/raw/ntb/ntb.c
>>>> +++ b/drivers/raw/ntb/ntb.c
>>>> @@ -1080,6 +1080,10 @@ ntb_attr_set(struct rte_rawdev *dev, const
>>>> char *attr_name,
>>>> if (hw->ntb_ops->spad_write == NULL)
>>>> return -ENOTSUP;
>>>> index = atoi(&attr_name[NTB_SPAD_USER_LEN]);
>>>> + if (index < 0 || index >= NTB_SPAD_USER_MAX_NUM) {
>>>> + NTB_LOG(ERR, "Invalid attribute (%s)", attr_name);
>>>> + return -EINVAL;
>>>> + }
>>>
>>> It's unnecessary. The value will be checked in intel_ntb_spad_write(). There
>> will be error remind in that.
>>>
>> Nothing to do with intel_ntb_spad_write. If index is no checked,
>> hw->spad_user_list[index] may be be out of memory and result in
>> segmentation default.
>
> Are you using this driver externally? Or you just check everything in DPDK.
> This is actually only used for ntb example in file trans mode. And only 0 and 1 are used for index.
>
Well, I just reviewed codes and found this bug.
>>
>>
>>>> (*hw->ntb_ops->spad_write)(dev, hw->spad_user_list[index],
>>>> 1, attr_value);
>>>> NTB_LOG(DEBUG, "Set attribute (%s) Value (%" PRIu64 ")", @@
>>>> -1174,6 +1178,10 @@ ntb_attr_get(struct rte_rawdev *dev, const char
>>>> *attr_name,
>>>> if (hw->ntb_ops->spad_read == NULL)
>>>> return -ENOTSUP;
>>>> index = atoi(&attr_name[NTB_SPAD_USER_LEN]);
>>>> + if (index < 0 || index >= NTB_SPAD_USER_MAX_NUM) {
>>>> + NTB_LOG(ERR, "Attribute (%s) out of range",
>>>> attr_name);
>>>> + return -EINVAL;
>>>> + }
>>>
>>> Same as above.
>>>
>>>> *attr_value = (*hw->ntb_ops->spad_read)(dev,
>>>> hw->spad_user_list[index], 0);
>>>> NTB_LOG(DEBUG, "Attribute (%s) Value (%" PRIu64 ")",
>>>> --
>>>> 2.7.4
>>>
>>> .
>>>
> .
>
> -----Original Message-----
> From: Min Hu (Connor) <humin29@huawei.com>
> Sent: Wednesday, April 21, 2021 10:08
> To: dev@dpdk.org
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Li, Xiaoyun <xiaoyun.li@intel.com>;
> Wu, Jingjing <jingjing.wu@intel.com>
> Subject: [PATCH 1/2] raw/ntb: check spad user index
>
> From: Chengwen Feng <fengchengwen@huawei.com>
>
> This patch adds checking spad user index validity when set or get attr.
>
> Fixes: 277310027965 ("raw/ntb: introduce NTB raw device driver")
> Cc: stable@dpdk.org
>
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
> drivers/raw/ntb/ntb.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/raw/ntb/ntb.c b/drivers/raw/ntb/ntb.c index
> 6dd213e..0f0e3f2 100644
> --- a/drivers/raw/ntb/ntb.c
> +++ b/drivers/raw/ntb/ntb.c
> @@ -1080,6 +1080,10 @@ ntb_attr_set(struct rte_rawdev *dev, const char
> *attr_name,
> if (hw->ntb_ops->spad_write == NULL)
> return -ENOTSUP;
> index = atoi(&attr_name[NTB_SPAD_USER_LEN]);
> + if (index < 0 || index >= NTB_SPAD_USER_MAX_NUM) {
> + NTB_LOG(ERR, "Invalid attribute (%s)", attr_name);
> + return -EINVAL;
> + }
> (*hw->ntb_ops->spad_write)(dev, hw->spad_user_list[index],
> 1, attr_value);
> NTB_LOG(DEBUG, "Set attribute (%s) Value (%" PRIu64 ")", @@
> -1174,6 +1178,10 @@ ntb_attr_get(struct rte_rawdev *dev, const char
> *attr_name,
> if (hw->ntb_ops->spad_read == NULL)
> return -ENOTSUP;
> index = atoi(&attr_name[NTB_SPAD_USER_LEN]);
> + if (index < 0 || index >= NTB_SPAD_USER_MAX_NUM) {
> + NTB_LOG(ERR, "Attribute (%s) out of range",
> attr_name);
> + return -EINVAL;
> + }
> *attr_value = (*hw->ntb_ops->spad_read)(dev,
> hw->spad_user_list[index], 0);
> NTB_LOG(DEBUG, "Attribute (%s) Value (%" PRIu64 ")",
> --
> 2.7.4
Acked-by: Xiaoyun Li <xiaoyun.li@intel.com>
21/04/2021 08:08, Min Hu (Connor):
> 在 2021/4/21 12:54, Li, Xiaoyun 写道:
> > From: Min Hu (Connor) <humin29@huawei.com>
> >> 在 2021/4/21 11:31, Li, Xiaoyun 写道:
> >>> From: Min Hu (Connor) <humin29@huawei.com>
> >>>> From: Chengwen Feng <fengchengwen@huawei.com>
> >>>> --- a/drivers/raw/ntb/ntb.c
> >>>> +++ b/drivers/raw/ntb/ntb.c
> >>>> index = atoi(&attr_name[NTB_SPAD_USER_LEN]);
> >>>> + if (index < 0 || index >= NTB_SPAD_USER_MAX_NUM) {
> >>>> + NTB_LOG(ERR, "Invalid attribute (%s)", attr_name);
> >>>> + return -EINVAL;
> >>>> + }
> >>>
> >>> It's unnecessary. The value will be checked in intel_ntb_spad_write().
> >>> There will be error remind in that.
> >>>
> >> Nothing to do with intel_ntb_spad_write. If index is no checked,
> >> hw->spad_user_list[index] may be be out of memory and result in
> >> segmentation default.
> >
> > Are you using this driver externally? Or you just check everything in DPDK.
> > This is actually only used for ntb example in file trans mode. And only 0 and 1 are used for index.
>
> Well, I just reviewed codes and found this bug.
Are you using some tools to detect bugs?
22/04/2021 07:05, Li, Xiaoyun:
> > From: Chengwen Feng <fengchengwen@huawei.com>
> >
> > This patch adds checking spad user index validity when set or get attr.
> >
> > Fixes: 277310027965 ("raw/ntb: introduce NTB raw device driver")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> > Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>
> Acked-by: Xiaoyun Li <xiaoyun.li@intel.com>
Series applied, thanks.
@@ -1080,6 +1080,10 @@ ntb_attr_set(struct rte_rawdev *dev, const char *attr_name,
if (hw->ntb_ops->spad_write == NULL)
return -ENOTSUP;
index = atoi(&attr_name[NTB_SPAD_USER_LEN]);
+ if (index < 0 || index >= NTB_SPAD_USER_MAX_NUM) {
+ NTB_LOG(ERR, "Invalid attribute (%s)", attr_name);
+ return -EINVAL;
+ }
(*hw->ntb_ops->spad_write)(dev, hw->spad_user_list[index],
1, attr_value);
NTB_LOG(DEBUG, "Set attribute (%s) Value (%" PRIu64 ")",
@@ -1174,6 +1178,10 @@ ntb_attr_get(struct rte_rawdev *dev, const char *attr_name,
if (hw->ntb_ops->spad_read == NULL)
return -ENOTSUP;
index = atoi(&attr_name[NTB_SPAD_USER_LEN]);
+ if (index < 0 || index >= NTB_SPAD_USER_MAX_NUM) {
+ NTB_LOG(ERR, "Attribute (%s) out of range", attr_name);
+ return -EINVAL;
+ }
*attr_value = (*hw->ntb_ops->spad_read)(dev,
hw->spad_user_list[index], 0);
NTB_LOG(DEBUG, "Attribute (%s) Value (%" PRIu64 ")",