| Message ID | 20230119022208.51463-1-wangchuanlei@inspur.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v2.17] flow_netlink: fix OOB access in reserve_sfa_size() | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | fail | apply and check: fail |
| ovsrobot/intel-ovs-compilation | fail | test: fail |
On 1/19/23 03:22, wangchuanlei wrote: > Given a sufficiently large number of actions, while copying and > reserving memory for a new action of a new flow, if next_offset is > greater than MAX_ACTIONS_BUFSIZE, the function reserve_sfa_size() does > not return -EMSGSIZE as expected, but it allocates MAX_ACTIONS_BUFSIZE > bytes increasing actions_len by req_size. This can then lead to an OOB > write access, especially when further actions need to be copied. > > Fix it by rearranging the flow action size check. > > Signed-off-by: wangchuanlei <wangchuanlei@inspur.com> > --- > > This commit is sync commit by pvalerio in kernel, commit > id is cefa91b2332d70 > > datapath/flow_netlink.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c > index 996041602..1ad637392 100644 > --- a/datapath/flow_netlink.c > +++ b/datapath/flow_netlink.c > @@ -2345,7 +2345,7 @@ static struct nlattr *reserve_sfa_size(struct sw_flow_actions **sfa, > new_acts_size = max(next_offset + req_size, ksize(*sfa) * 2); > > if (new_acts_size > MAX_ACTIONS_BUFSIZE) { > - if ((MAX_ACTIONS_BUFSIZE - next_offset) < req_size) { > + if ((next_offset + req_size) > MAX_ACTIONS_BUFSIZE) { > OVS_NLERR(log, "Flow action size exceeds max %u", > MAX_ACTIONS_BUFSIZE); > return ERR_PTR(-EMSGSIZE); Hi. Thanks for the patch. Unfortunately, it doesn't follow the common format for backports. Please, see the guidelines on how to create backports and which subject prefix to use for stable branches here: Documentation/internals/contributing/backporting-patches.rst Best regards, Ilya Maximets.
diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c index 996041602..1ad637392 100644 --- a/datapath/flow_netlink.c +++ b/datapath/flow_netlink.c @@ -2345,7 +2345,7 @@ static struct nlattr *reserve_sfa_size(struct sw_flow_actions **sfa, new_acts_size = max(next_offset + req_size, ksize(*sfa) * 2); if (new_acts_size > MAX_ACTIONS_BUFSIZE) { - if ((MAX_ACTIONS_BUFSIZE - next_offset) < req_size) { + if ((next_offset + req_size) > MAX_ACTIONS_BUFSIZE) { OVS_NLERR(log, "Flow action size exceeds max %u", MAX_ACTIONS_BUFSIZE); return ERR_PTR(-EMSGSIZE);
Given a sufficiently large number of actions, while copying and reserving memory for a new action of a new flow, if next_offset is greater than MAX_ACTIONS_BUFSIZE, the function reserve_sfa_size() does not return -EMSGSIZE as expected, but it allocates MAX_ACTIONS_BUFSIZE bytes increasing actions_len by req_size. This can then lead to an OOB write access, especially when further actions need to be copied. Fix it by rearranging the flow action size check. Signed-off-by: wangchuanlei <wangchuanlei@inspur.com> --- This commit is sync commit by pvalerio in kernel, commit id is cefa91b2332d70 datapath/flow_netlink.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)