| Message ID | 4d6d9d9e-b4d9-42f1-aa78-1a5979679b2e@stanley.mountain (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: ovs: prevent underflow in queue_userspace_packet() | expand |
Dan Carpenter <dan.carpenter@linaro.org> writes: > If "hlen" less than "cutlen" then when we call upcall_msg_size() > the "hlen - cutlen" parameter will be a very high positive > number. > > Later in the function we use "skb->len - cutlen" but this change > addresses that potential underflow since skb->len is always going to > be greater than or equal to hlen. > > Fixes: f2a4d086ed4c ("openvswitch: Add packet truncation support.") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > From code review not testing. I think it's pretty difficult to trigger this case. 'cutlen' will only be set by a TRUNC action attribute, which does a length check there. > net/openvswitch/datapath.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c > index 225f6048867f..bb25a2bbe8a0 100644 > --- a/net/openvswitch/datapath.c > +++ b/net/openvswitch/datapath.c > @@ -477,6 +477,11 @@ static int queue_userspace_packet(struct datapath *dp, struct sk_buff *skb, > else > hlen = skb->len; > > + if (hlen < cutlen) { > + err = -EINVAL; > + goto out; > + } > + I think the right change would be more like: if (hlen < cutlen) cutlen = 0; Since trunc is supposed to be a cap on the length to trim the packet. If the cut length would be longer than the packet, then the whole packet should fit. > len = upcall_msg_size(upcall_info, hlen - cutlen, > OVS_CB(skb)->acts_origlen); > user_skb = genlmsg_new(len, GFP_ATOMIC);
On Fri, Jan 24, 2025 at 07:42:26PM -0500, Aaron Conole wrote: > Dan Carpenter <dan.carpenter@linaro.org> writes: > > > If "hlen" less than "cutlen" then when we call upcall_msg_size() > > the "hlen - cutlen" parameter will be a very high positive > > number. > > > > Later in the function we use "skb->len - cutlen" but this change > > addresses that potential underflow since skb->len is always going to > > be greater than or equal to hlen. > > > > Fixes: f2a4d086ed4c ("openvswitch: Add packet truncation support.") > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > > --- > > From code review not testing. > > I think it's pretty difficult to trigger this case. 'cutlen' will only > be set by a TRUNC action attribute, which does a length check there. > Ah. Yes. You're right. I assumed that since we neede to do a upper bounds check on skb->len then probably the lower bounds needed to be checked too but that's not correct. My bad. No need to do anything here because the code is fine as-is. net/openvswitch/actions.c 1402 case OVS_ACTION_ATTR_TRUNC: { 1403 struct ovs_action_trunc *trunc = nla_data(a); 1404 1405 if (skb->len > trunc->max_len) 1406 OVS_CB(skb)->cutlen = skb->len - trunc->max_len; 1407 break; 1408 } regards, dan carpenter
diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c index 225f6048867f..bb25a2bbe8a0 100644 --- a/net/openvswitch/datapath.c +++ b/net/openvswitch/datapath.c @@ -477,6 +477,11 @@ static int queue_userspace_packet(struct datapath *dp, struct sk_buff *skb, else hlen = skb->len; + if (hlen < cutlen) { + err = -EINVAL; + goto out; + } + len = upcall_msg_size(upcall_info, hlen - cutlen, OVS_CB(skb)->acts_origlen); user_skb = genlmsg_new(len, GFP_ATOMIC);
If "hlen" less than "cutlen" then when we call upcall_msg_size() the "hlen - cutlen" parameter will be a very high positive number. Later in the function we use "skb->len - cutlen" but this change addresses that potential underflow since skb->len is always going to be greater than or equal to hlen. Fixes: f2a4d086ed4c ("openvswitch: Add packet truncation support.") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- From code review not testing. net/openvswitch/datapath.c | 5 +++++ 1 file changed, 5 insertions(+)