[ovs-dev] tun-metadata: Add bounds check for Geneve TLV options

From: saladin0x1 <salaheddine.elouali@emsi-edu.ma>

This adds a bounds check in tun_metadata_from_geneve_nlattr() to ensure
that attr_len does not exceed the destination buffer size (256 bytes)
before performing memcpy().

While the kernel module validates Geneve option lengths before passing
data to userspace via netlink, adding redundant validation in userspace
provides defense-in-depth. If the kernel were to pass a malformed netlink
message (due to a bug or compromise), userspace would previously overflow
the tun->metadata.opts.gnv buffer.

This was discussed with Ilya Maximets, who noted:
  "In general, we trust the kernel to give us correct data. If we can't
   trust the kernel, there is not much we can do. It makes sense to add
   a check in userspace parsing, but it's not a security concern."

The function now returns -EINVAL on overflow and 0 on success, with
callers in odp-util.c handling the error appropriately.

Signed-off-by: Saladin EL Ouali <salaheddine.elouali@emsi-edu.ma>
---
 lib/odp-util.c     | 6 +++++-
 lib/tun-metadata.c | 7 ++++++-
 lib/tun-metadata.h | 2 +-
 3 files changed, 12 insertions(+), 3 deletions(-)

Hi.  Thanks for the patch!  And sorry for delay.

See some comments inline.

nit: As the 0-day bot pointed out the subject line should end with a dot.

Also, please, increment the version number in the subject prefix while
sending new versions of the patch, e.g. "[PATCH v2] ...".

On 1/15/26 12:44 PM, Saladin EL Ouali wrote:
> From: saladin0x1 <salaheddine.elouali@emsi-edu.ma>

The "From" field, which is the "Author" in the git, should match the
Signed-off-by tag.

> 
> This adds a bounds check in tun_metadata_from_geneve_nlattr() to ensure
> that attr_len does not exceed the destination buffer size (256 bytes)
> before performing memcpy().
> 
> While the kernel module validates Geneve option lengths before passing
> data to userspace via netlink, adding redundant validation in userspace
> provides defense-in-depth. If the kernel were to pass a malformed netlink
> message (due to a bug or compromise), userspace would previously overflow
> the tun->metadata.opts.gnv buffer.
> 
> This was discussed with Ilya Maximets, who noted:
>   "In general, we trust the kernel to give us correct data. If we can't
>    trust the kernel, there is not much we can do. It makes sense to add
>    a check in userspace parsing, but it's not a security concern."
> 
> The function now returns -EINVAL on overflow and 0 on success, with
> callers in odp-util.c handling the error appropriately.
> 
> Signed-off-by: Saladin EL Ouali <salaheddine.elouali@emsi-edu.ma>
> ---
>  lib/odp-util.c     | 6 +++++-
>  lib/tun-metadata.c | 7 ++++++-
>  lib/tun-metadata.h | 2 +-
>  3 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index ee1868202..d87506193 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> @@ -3255,7 +3255,11 @@ odp_tun_key_from_attr__(const struct nlattr *attr, bool is_mask,
>              break;
>          }
>          case OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS:
> -            tun_metadata_from_geneve_nlattr(a, is_mask, tun);
> +            if (tun_metadata_from_geneve_nlattr(a, is_mask, tun)) {
> +                odp_parse_error(&rl, errorp,
> +                                "Geneve options length exceeds maximum");

The code here doesn't actually know why the parsing failed, the exact
reason is not returned from the tun_metadata_from_geneve_nlattr().
So we should just print something more generic, e.g. look at the
message for VXLAN parsing failure.

> +                return ODP_FIT_ERROR;
> +            }
>              break;
>          case OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS: {
>              const struct erspan_metadata *opts = nl_attr_get(a);
> diff --git a/lib/tun-metadata.c b/lib/tun-metadata.c
> index af0bcbde8..1561da35f 100644
> --- a/lib/tun-metadata.c
> +++ b/lib/tun-metadata.c
> @@ -605,12 +605,15 @@ tun_metadata_del_entry(struct tun_table *map, uint8_t idx)
>   * on the wire. By always using UDPIF format, this allows us to process the
>   * flow key without any knowledge of the mapping table. We can do the
>   * conversion later if necessary. */
> -void
> +int
>  tun_metadata_from_geneve_nlattr(const struct nlattr *attr, bool is_mask,
>                                  struct flow_tnl *tun)
>  {
>      int attr_len = nl_attr_get_size(attr);
>  
> +    if (attr_len > sizeof(tun->metadata.opts.gnv)) {

sizeof of a variable doesn't need parenthesis.

> +        return -EINVAL;

Other functions here return positive error codes, might be better to
do the same here.

Also, there is a very similar memcpy() call in the parse_put_flow_set_action()
in lib/dpif-offload-tc-netdev.c.  We should probably add the check there as
well.  It's a TC netlink interface, but the data still comes from the kernel
in a form of a netlink message.

> +    }
>      memcpy(tun->metadata.opts.gnv, nl_attr_get(attr), attr_len);
>      tun->flags |= FLOW_TNL_F_UDPIF;
>  
> @@ -622,6 +625,8 @@ tun_metadata_from_geneve_nlattr(const struct nlattr *attr, bool is_mask,
>           * at the beginning but with additional options after. */
>          tun->metadata.present.len = 0xff;
>      }
> +
> +    return 0;
>  }
>  
>  /* Converts from the flat Geneve options representation extracted directly
> diff --git a/lib/tun-metadata.h b/lib/tun-metadata.h
> index 67dedae25..affc8c268 100644
> --- a/lib/tun-metadata.h
> +++ b/lib/tun-metadata.h
> @@ -54,7 +54,7 @@ void tun_metadata_set_match(const struct mf_field *,
>                              char **err_str);
>  void tun_metadata_get_fmd(const struct flow_tnl *, struct match *flow_metadata);
>  
> -void tun_metadata_from_geneve_nlattr(const struct nlattr *attr, bool is_mask,
> +int tun_metadata_from_geneve_nlattr(const struct nlattr *attr, bool is_mask,
>                                       struct flow_tnl *tun);

Need to adjust the indentation of the second line.

>  void tun_metadata_to_geneve_nlattr(const struct flow_tnl *tun,
>                                     const struct flow_tnl *flow,

Best regards, Ilya Maximets.