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 <[email protected]> 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 <[email protected]> > --- > 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. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
