[3/6] net/tap: check interface name in kvargs
Checks
Commit Message
If interface name is passed to remote or iface then check
the length and for invalid characters. This avoids problems where
name gets truncated or rejected by kernel.
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
drivers/net/tap/rte_eth_tap.c | 37 +++++++++++++++++++++++++++++++----
1 file changed, 33 insertions(+), 4 deletions(-)
Comments
> On Jan 11, 2019, at 12:06 PM, Stephen Hemminger <stephen@networkplumber.org> wrote:
>
> If interface name is passed to remote or iface then check
> the length and for invalid characters. This avoids problems where
> name gets truncated or rejected by kernel.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> drivers/net/tap/rte_eth_tap.c | 37 +++++++++++++++++++++++++++++++----
> 1 file changed, 33 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index d7f77d664502..6a388eed0dd4 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -37,6 +37,7 @@
> #include <linux/if_tun.h>
> #include <linux/if_ether.h>
> #include <fcntl.h>
> +#include <ctype.h>
>
> #include <tap_rss.h>
> #include <rte_eth_tap.h>
> @@ -1884,6 +1885,23 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
> return -EINVAL;
> }
>
> +/* make sure name is a possible Linux network device name */
> +static bool is_valid_iface(const char *name)
I am sorry, but the function name must be on the next line as per the style. I know you do not like it, but that is what the style states.
> +{
> + if (*name == '\0')
> + return false;
> +
> + if (strnlen(name, IFNAMSIZ) == IFNAMSIZ)
> + return false;
> +
> + while (*name) {
> + if (*name == '/' || *name == ':' || isspace(*name))
> + return false;
> + name++;
> + }
> + return true;
> +}
> +
> static int
> set_interface_name(const char *key __rte_unused,
> const char *value,
> @@ -1891,12 +1909,17 @@ set_interface_name(const char *key __rte_unused,
> {
> char *name = (char *)extra_args;
>
> - if (value)
> + if (value) {
> + if (!is_valid_iface(value)) {
> + TAP_LOG(ERR, "TAP invalid remote interface name (%s)",
> + value);
> + return -1;
> + }
> strlcpy(name, value, RTE_ETH_NAME_MAX_LEN);
> - else
> + } else {
> snprintf(name, RTE_ETH_NAME_MAX_LEN, "%s%d",
> DEFAULT_TAP_NAME, tun_unit - 1);
> -
> + }
This is also a one line else and the style states to remove the {}.
> return 0;
> }
>
> @@ -1907,8 +1930,14 @@ set_remote_iface(const char *key __rte_unused,
> {
> char *name = (char *)extra_args;
>
> - if (value)
> + if (value) {
> + if (!is_valid_iface(value)) {
> + TAP_LOG(ERR, "TAP invalid remote interface name (%s)",
> + value);
> + return -1;
> + }
> strlcpy(name, value, RTE_ETH_NAME_MAX_LEN);
> + }
>
> return 0;
> }
> --
> 2.20.1
>
I hate having to be the style police, but until we use something to validate the DPDK coding style (as I posted the uncrustify config last month) we have to keep the style the same or change the DPDK coding standard.
Regards,
Keith
On Fri, 11 Jan 2019 19:37:00 +0000
"Wiles, Keith" <keith.wiles@intel.com> wrote:
> > +/* make sure name is a possible Linux network device name */
> > +static bool is_valid_iface(const char *name)
>
> I am sorry, but the function name must be on the next line as per the style. I know you do not like it, but that is what the style states.
I am surprised because most of the DPDK follows kernel style.
And in kernel style one line is preferred if line is not too long.
> On Jan 11, 2019, at 1:49 PM, Stephen Hemminger <stephen@networkplumber.org> wrote:
>
> On Fri, 11 Jan 2019 19:37:00 +0000
> "Wiles, Keith" <keith.wiles@intel.com> wrote:
>
>>> +/* make sure name is a possible Linux network device name */
>>> +static bool is_valid_iface(const char *name)
>>
>> I am sorry, but the function name must be on the next line as per the style. I know you do not like it, but that is what the style states.
>
> I am surprised because most of the DPDK follows kernel style.
> And in kernel style one line is preferred if line is not too long.
Last time I looked at the DPDK coding style it is not the case. We do not really follow Linux coding style only started with it and made changes :-(
Regards,
Keith
Thanks Jim!!
-from my iPhone
> On Jan 11, 2019, at 11:37 AM, Wiles, Keith <keith.wiles@intel.com> wrote:
>
>
>
>> On Jan 11, 2019, at 12:06 PM, Stephen Hemminger <stephen@networkplumber.org> wrote:
>>
>> If interface name is passed to remote or iface then check
>> the length and for invalid characters. This avoids problems where
>> name gets truncated or rejected by kernel.
>>
>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>> ---
>> drivers/net/tap/rte_eth_tap.c | 37 +++++++++++++++++++++++++++++++----
>> 1 file changed, 33 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>> index d7f77d664502..6a388eed0dd4 100644
>> --- a/drivers/net/tap/rte_eth_tap.c
>> +++ b/drivers/net/tap/rte_eth_tap.c
>> @@ -37,6 +37,7 @@
>> #include <linux/if_tun.h>
>> #include <linux/if_ether.h>
>> #include <fcntl.h>
>> +#include <ctype.h>
>>
>> #include <tap_rss.h>
>> #include <rte_eth_tap.h>
>> @@ -1884,6 +1885,23 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
>> return -EINVAL;
>> }
>>
>> +/* make sure name is a possible Linux network device name */
>> +static bool is_valid_iface(const char *name)
>
> I am sorry, but the function name must be on the next line as per the style. I know you do not like it, but that is what the style states.
>> +{
>> + if (*name == '\0')
>> + return false;
>> +
>> + if (strnlen(name, IFNAMSIZ) == IFNAMSIZ)
>> + return false;
>> +
>> + while (*name) {
>> + if (*name == '/' || *name == ':' || isspace(*name))
>> + return false;
>> + name++;
>> + }
>> + return true;
>> +}
>> +
>> static int
>> set_interface_name(const char *key __rte_unused,
>> const char *value,
>> @@ -1891,12 +1909,17 @@ set_interface_name(const char *key __rte_unused,
>> {
>> char *name = (char *)extra_args;
>>
>> - if (value)
>> + if (value) {
>> + if (!is_valid_iface(value)) {
>> + TAP_LOG(ERR, "TAP invalid remote interface name (%s)",
>> + value);
>> + return -1;
>> + }
>> strlcpy(name, value, RTE_ETH_NAME_MAX_LEN);
>> - else
>> + } else {
>> snprintf(name, RTE_ETH_NAME_MAX_LEN, "%s%d",
>> DEFAULT_TAP_NAME, tun_unit - 1);
>> -
>> + }
>
> This is also a one line else and the style states to remove the {}.
>> return 0;
>> }
>>
>> @@ -1907,8 +1930,14 @@ set_remote_iface(const char *key __rte_unused,
>> {
>> char *name = (char *)extra_args;
>>
>> - if (value)
>> + if (value) {
>> + if (!is_valid_iface(value)) {
>> + TAP_LOG(ERR, "TAP invalid remote interface name (%s)",
>> + value);
>> + return -1;
>> + }
>> strlcpy(name, value, RTE_ETH_NAME_MAX_LEN);
>> + }
>>
>> return 0;
>> }
>> --
>> 2.20.1
>>
>
> I hate having to be the style police, but until we use something to validate the DPDK coding style (as I posted the uncrustify config last month) we have to keep the style the same or change the DPDK coding standard.
>
> Regards,
> Keith
>
11/01/2019 20:50, Wiles, Keith:
>
> > On Jan 11, 2019, at 1:49 PM, Stephen Hemminger <stephen@networkplumber.org> wrote:
> >
> > On Fri, 11 Jan 2019 19:37:00 +0000
> > "Wiles, Keith" <keith.wiles@intel.com> wrote:
> >
> >>> +/* make sure name is a possible Linux network device name */
> >>> +static bool is_valid_iface(const char *name)
> >>
> >> I am sorry, but the function name must be on the next line as per the style. I know you do not like it, but that is what the style states.
> >
> > I am surprised because most of the DPDK follows kernel style.
> > And in kernel style one line is preferred if line is not too long.
>
> Last time I looked at the DPDK coding style it is not the case. We do not really follow Linux coding style only started with it and made changes :-(
Not really. The original code style is borrowed from FreeBSD:
https://www.freebsd.org/cgi/man.cgi?query=style&sektion=9
@@ -37,6 +37,7 @@
#include <linux/if_tun.h>
#include <linux/if_ether.h>
#include <fcntl.h>
+#include <ctype.h>
#include <tap_rss.h>
#include <rte_eth_tap.h>
@@ -1884,6 +1885,23 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
return -EINVAL;
}
+/* make sure name is a possible Linux network device name */
+static bool is_valid_iface(const char *name)
+{
+ if (*name == '\0')
+ return false;
+
+ if (strnlen(name, IFNAMSIZ) == IFNAMSIZ)
+ return false;
+
+ while (*name) {
+ if (*name == '/' || *name == ':' || isspace(*name))
+ return false;
+ name++;
+ }
+ return true;
+}
+
static int
set_interface_name(const char *key __rte_unused,
const char *value,
@@ -1891,12 +1909,17 @@ set_interface_name(const char *key __rte_unused,
{
char *name = (char *)extra_args;
- if (value)
+ if (value) {
+ if (!is_valid_iface(value)) {
+ TAP_LOG(ERR, "TAP invalid remote interface name (%s)",
+ value);
+ return -1;
+ }
strlcpy(name, value, RTE_ETH_NAME_MAX_LEN);
- else
+ } else {
snprintf(name, RTE_ETH_NAME_MAX_LEN, "%s%d",
DEFAULT_TAP_NAME, tun_unit - 1);
-
+ }
return 0;
}
@@ -1907,8 +1930,14 @@ set_remote_iface(const char *key __rte_unused,
{
char *name = (char *)extra_args;
- if (value)
+ if (value) {
+ if (!is_valid_iface(value)) {
+ TAP_LOG(ERR, "TAP invalid remote interface name (%s)",
+ value);
+ return -1;
+ }
strlcpy(name, value, RTE_ETH_NAME_MAX_LEN);
+ }
return 0;
}