[PATCH net-next 0/3] net: warn about attempts to register negative ifindex

[PATCH net-next 0/3] net: warn about attempts to register negative ifindex

[Jakub Kicinski]

Follow up to the recently posted fix for OvS lacking input
validation:
https://lore.kernel.org/all/20230814203840.2908710-1-kuba@kernel.org/

Warn about negative ifindex more explicitly and misc YNL updates.

Jakub Kicinski (3):
  net: warn about attempts to register negative ifindex
  netlink: specs: add ovs_vport new command
  tools: ynl: add more info to KeyErrors on missing attrs

 Documentation/netlink/specs/ovs_vport.yaml | 18 ++++++++++++++++++
 net/core/dev.c                             |  5 +++++
 tools/net/ynl/lib/ynl.py                   | 15 ++++++++++++---
 3 files changed, 35 insertions(+), 3 deletions(-)

-- 
2.41.0

[PATCH net-next 1/3] net: warn about attempts to register negative ifindex

[Jakub Kicinski]

Since the xarray changes we mix returning valid ifindex and negative
errno in a single int returned from dev_index_reserve(). This depends
on the fact that ifindexes can't be negative. Otherwise we may insert
into the xarray and return a very large negative value. This in turn
may break ERR_PTR().

OvS is susceptible to this problem and lacking validation (fix posted
separately for net).

Reject negative ifindex explicitly. Add a warning because the input
validation is better handled by the caller.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
Turns out concerns from reviewers that callers may not check bounds on
ifindex were legit...
---
 net/core/dev.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/core/dev.c b/net/core/dev.c
index 636b41f0b32d..17e6281e408c 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -9589,6 +9589,11 @@ static int dev_index_reserve(struct net *net, u32 ifindex)
 {
 	int err;
 
+	if (ifindex > INT_MAX) {
+		DEBUG_NET_WARN_ON_ONCE(1);
+		return -EINVAL;
+	}
+
 	if (!ifindex)
 		err = xa_alloc_cyclic(&net->dev_by_index, &ifindex, NULL,
 				      xa_limit_31b, &net->ifindex, GFP_KERNEL);
-- 
2.41.0

[PATCH net-next 2/3] netlink: specs: add ovs_vport new command

[Jakub Kicinski]

Add NEW to the spec, it was useful testing the fix for OvS
input validation.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: donald.hunter@gmail.com
---
 Documentation/netlink/specs/ovs_vport.yaml | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/Documentation/netlink/specs/ovs_vport.yaml b/Documentation/netlink/specs/ovs_vport.yaml
index 17336455bec1..ef298b001445 100644
--- a/Documentation/netlink/specs/ovs_vport.yaml
+++ b/Documentation/netlink/specs/ovs_vport.yaml
@@ -81,6 +81,10 @@ uapi-header: linux/openvswitch.h
     name-prefix: ovs-vport-attr-
     enum-name: ovs-vport-attr
     attributes:
+      -
+        name: unspec
+        type: unused
+        value: 0
       -
         name: port-no
         type: u32
@@ -120,6 +124,20 @@ uapi-header: linux/openvswitch.h
 operations:
   name-prefix: ovs-vport-cmd-
   list:
+    -
+      name: new
+      doc: Create a new OVS vport
+      attribute-set: vport
+      fixed-header: ovs-header
+      do:
+        request:
+          attributes:
+            - name
+            - type
+            - upcall-pid
+            - dp-ifindex
+            - ifindex
+            - options
     -
       name: get
       doc: Get / dump OVS vport configuration and state
-- 
2.41.0

[PATCH net-next 3/3] tools: ynl: add more info to KeyErrors on missing attrs

[Jakub Kicinski]

When developing specs its useful to know which attr space
YNL was trying to find an attribute in on key error.

Instead of printing:
 KeyError: 0
add info about the space:
 Exception: Space 'vport' has no attribute with value '0'

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: donald.hunter@gmail.com
---
 tools/net/ynl/lib/ynl.py | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
index 3ca28d4bcb18..6951bcc7efdc 100644
--- a/tools/net/ynl/lib/ynl.py
+++ b/tools/net/ynl/lib/ynl.py
@@ -395,7 +395,10 @@ genl_family_name_to_id = None
                              self.family.genl_family['mcast'][mcast_name])
 
     def _add_attr(self, space, name, value):
-        attr = self.attr_sets[space][name]
+        try:
+            attr = self.attr_sets[space][name]
+        except KeyError:
+            raise Exception(f"Space '{space}' has no attribute '{name}'")
         nl_type = attr.value
         if attr["type"] == 'nest':
             nl_type |= Netlink.NLA_F_NESTED
@@ -450,7 +453,10 @@ genl_family_name_to_id = None
         attr_space = self.attr_sets[space]
         rsp = dict()
         for attr in attrs:
-            attr_spec = attr_space.attrs_by_val[attr.type]
+            try:
+                attr_spec = attr_space.attrs_by_val[attr.type]
+            except KeyError:
+                raise Exception(f"Space '{space}' has no attribute with value '{attr.type}'")
             if attr_spec["type"] == 'nest':
                 subdict = self._decode(NlAttrs(attr.raw), attr_spec['nested-attributes'])
                 decoded = subdict
@@ -479,7 +485,10 @@ genl_family_name_to_id = None
 
     def _decode_extack_path(self, attrs, attr_set, offset, target):
         for attr in attrs:
-            attr_spec = attr_set.attrs_by_val[attr.type]
+            try:
+                attr_spec = attr_set.attrs_by_val[attr.type]
+            except KeyError:
+                raise Exception(f"Space '{attr_set.name}' has no attribute with value '{attr.type}'")
             if offset > target:
                 break
             if offset == target:
-- 
2.41.0

Re: [PATCH net-next 1/3] net: warn about attempts to register negative ifindex

[Leon Romanovsky]

On Mon, Aug 14, 2023 at 01:56:25PM -0700, Jakub Kicinski wrote:
> Since the xarray changes we mix returning valid ifindex and negative
> errno in a single int returned from dev_index_reserve(). This depends
> on the fact that ifindexes can't be negative. Otherwise we may insert
> into the xarray and return a very large negative value. This in turn
> may break ERR_PTR().
> 
> OvS is susceptible to this problem and lacking validation (fix posted
> separately for net).
> 
> Reject negative ifindex explicitly. Add a warning because the input
> validation is better handled by the caller.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> Turns out concerns from reviewers that callers may not check bounds on
> ifindex were legit...
> ---
>  net/core/dev.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 

Thanks,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>

Re: [PATCH net-next 2/3] netlink: specs: add ovs_vport new command

[Donald Hunter]

Jakub Kicinski <kuba@kernel.org> writes:

> Add NEW to the spec, it was useful testing the fix for OvS
> input validation.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: donald.hunter@gmail.com
> ---
>  Documentation/netlink/specs/ovs_vport.yaml | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/Documentation/netlink/specs/ovs_vport.yaml b/Documentation/netlink/specs/ovs_vport.yaml
> index 17336455bec1..ef298b001445 100644
> --- a/Documentation/netlink/specs/ovs_vport.yaml
> +++ b/Documentation/netlink/specs/ovs_vport.yaml
> @@ -81,6 +81,10 @@ uapi-header: linux/openvswitch.h
>      name-prefix: ovs-vport-attr-
>      enum-name: ovs-vport-attr
>      attributes:
> +      -
> +        name: unspec
> +        type: unused
> +        value: 0
>        -
>          name: port-no
>          type: u32
> @@ -120,6 +124,20 @@ uapi-header: linux/openvswitch.h
>  operations:
>    name-prefix: ovs-vport-cmd-
>    list:
> +    -
> +      name: new
> +      doc: Create a new OVS vport
> +      attribute-set: vport
> +      fixed-header: ovs-header
> +      do:
> +        request:
> +          attributes:
> +            - name
> +            - type
> +            - upcall-pid
> +            - dp-ifindex
> +            - ifindex
> +            - options
>      -
>        name: get
>        doc: Get / dump OVS vport configuration and state

Reviewed-by: Donald Hunter <donald.hunter@gmail.com>

Re: [PATCH net-next 3/3] tools: ynl: add more info to KeyErrors on missing attrs

[Donald Hunter]

Jakub Kicinski <kuba@kernel.org> writes:

> When developing specs its useful to know which attr space
> YNL was trying to find an attribute in on key error.
>
> Instead of printing:
>  KeyError: 0
> add info about the space:
>  Exception: Space 'vport' has no attribute with value '0'
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: donald.hunter@gmail.com
> ---
>  tools/net/ynl/lib/ynl.py | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
> index 3ca28d4bcb18..6951bcc7efdc 100644
> --- a/tools/net/ynl/lib/ynl.py
> +++ b/tools/net/ynl/lib/ynl.py
> @@ -395,7 +395,10 @@ genl_family_name_to_id = None
>                               self.family.genl_family['mcast'][mcast_name])
>  
>      def _add_attr(self, space, name, value):
> -        attr = self.attr_sets[space][name]
> +        try:
> +            attr = self.attr_sets[space][name]
> +        except KeyError:
> +            raise Exception(f"Space '{space}' has no attribute '{name}'")
>          nl_type = attr.value
>          if attr["type"] == 'nest':
>              nl_type |= Netlink.NLA_F_NESTED
> @@ -450,7 +453,10 @@ genl_family_name_to_id = None
>          attr_space = self.attr_sets[space]
>          rsp = dict()
>          for attr in attrs:
> -            attr_spec = attr_space.attrs_by_val[attr.type]
> +            try:
> +                attr_spec = attr_space.attrs_by_val[attr.type]
> +            except KeyError:
> +                raise Exception(f"Space '{space}' has no attribute with value '{attr.type}'")
>              if attr_spec["type"] == 'nest':
>                  subdict = self._decode(NlAttrs(attr.raw), attr_spec['nested-attributes'])
>                  decoded = subdict
> @@ -479,7 +485,10 @@ genl_family_name_to_id = None
>  
>      def _decode_extack_path(self, attrs, attr_set, offset, target):
>          for attr in attrs:
> -            attr_spec = attr_set.attrs_by_val[attr.type]
> +            try:
> +                attr_spec = attr_set.attrs_by_val[attr.type]
> +            except KeyError:
> +                raise Exception(f"Space '{attr_set.name}' has no attribute with value '{attr.type}'")
>              if offset > target:
>                  break
>              if offset == target:

Reviewed-by: Donald Hunter <donald.hunter@gmail.com>

Re: [PATCH net-next 0/3] net: warn about attempts to register negative ifindex

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 14 Aug 2023 13:56:24 -0700 you wrote:
> Follow up to the recently posted fix for OvS lacking input
> validation:
> https://lore.kernel.org/all/20230814203840.2908710-1-kuba@kernel.org/
> 
> Warn about negative ifindex more explicitly and misc YNL updates.
> 
> Jakub Kicinski (3):
>   net: warn about attempts to register negative ifindex
>   netlink: specs: add ovs_vport new command
>   tools: ynl: add more info to KeyErrors on missing attrs
> 
> [...]

Here is the summary with links:
  - [net-next,1/3] net: warn about attempts to register negative ifindex
    https://git.kernel.org/netdev/net-next/c/956db0a13b47
  - [net-next,2/3] netlink: specs: add ovs_vport new command
    https://git.kernel.org/netdev/net-next/c/ded67d90815a
  - [net-next,3/3] tools: ynl: add more info to KeyErrors on missing attrs
    https://git.kernel.org/netdev/net-next/c/7582113c6917

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html