Re: [ovs-dev] [PATCH ovn] physical.c: Fix heap-buffer-overflow.

2022-08-18 Thread Han Zhou
On Thu, Aug 18, 2022 at 10:20 PM Ales Musil  wrote:
>
>
>
> On Fri, Aug 19, 2022 at 4:08 AM Han Zhou  wrote:
>>
>> Fix the problem introduced by commit e52c245136, reported by ASAN for
test case:
>> "options:activation-strategy for logical port"
>>
>> ==1480622==ERROR: AddressSanitizer: heap-buffer-overflow on address
0x6110c7d0 at pc 0x004fbd91 bp 0x7ffec4244630 sp 0x7ffec4243de0
>> READ of size 168 at 0x6110c7d0 thread T0
>> #0 0x4fbd90 in __interceptor_memcpy.part.0
(/home/hanzhou/src/ovn/_build_as/controller/ovn-controller+0x4fbd90)
>> #1 0x73457f in ofpbuf_put
/home/hanzhou/src/ovs/_build/../lib/ofpbuf.c:431:5
>> #2 0x73457f in ofpbuf_put
/home/hanzhou/src/ovs/_build/../lib/ofpbuf.c:424:1
>> #3 0x72ba29 in ofpprop_put
/home/hanzhou/src/ovs/_build/../lib/ofp-prop.c:294:5
>> #4 0x7046e7 in encode_CONTROLLER
/home/hanzhou/src/ovs/_build/../lib/ofp-actions.c:914:13
>> #5 0x7046e7 in encode_ofpact
/home/hanzhou/src/ovs/_build/../lib/ofp-actions.c:8890:9
>> #6 0x705878 in ofpacts_put_openflow_instructions
/home/hanzhou/src/ovs/_build/../lib/ofp-actions.c:8943:17
>> #7 0x716287 in ofputil_encode_flow_mod
/home/hanzhou/src/ovs/_build/../lib/ofp-flow.c:426:9
>> #8 0x5c5978 in encode_flow_mod
/home/hanzhou/src/ovn/_build_as/../controller/ofctrl.c:1893:12
>> #9 0x5c5978 in add_flow_mod
/home/hanzhou/src/ovn/_build_as/../controller/ofctrl.c:1912:26
>> #10 0x5c5978 in installed_flow_add
/home/hanzhou/src/ovn/_build_as/../controller/ofctrl.c:2254:5
>> #11 0x5bfd62 in update_installed_flows_by_track
/home/hanzhou/src/ovn/_build_as/../controller/ofctrl.c:2484:17
>> #12 0x5bc5b9 in ofctrl_put
/home/hanzhou/src/ovn/_build_as/../controller/ofctrl.c:2715:13
>> #13 0x600e97 in main
/home/hanzhou/src/ovn/_build_as/../controller/ovn-controller.c:4186:25
>> #14 0x7f7ae80ce1a1 in __libc_start_main (/lib64/libc.so.6+0x281a1)
>> #15 0x49432d in _start
(/home/hanzhou/src/ovn/_build_as/controller/ovn-controller+0x49432d)
>>
>> Fixes: e52c245136 ("physical.c: Fix bug of wrong use in vm migration")
>> Signed-off-by: Han Zhou 
>> ---
>>  controller/physical.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/controller/physical.c b/controller/physical.c
>> index 552837bcd..c14a752f0 100644
>> --- a/controller/physical.c
>> +++ b/controller/physical.c
>> @@ -1001,6 +1001,7 @@ setup_rarp_activation_strategy(const struct
sbrec_port_binding *binding,
>>
>>  load_logical_ingress_metadata(binding, zone_ids, );
>>
>> +size_t ofs = ofpacts.size;
>>  struct ofpact_controller *oc = ofpact_put_CONTROLLER();
>>  oc->max_len = UINT16_MAX;
>>  oc->reason = OFPR_ACTION;
>> @@ -1011,7 +1012,7 @@ setup_rarp_activation_strategy(const struct
sbrec_port_binding *binding,
>>  ofpbuf_put(, , sizeof ah);
>>
>>  ofpacts.header = oc;
>> -oc->userdata_len = ofpacts.size - (sizeof *oc);
>> +oc->userdata_len = ofpacts.size - (ofs + sizeof *oc);
>>  ofpact_finish_CONTROLLER(, );
>>  put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, );
>>
>> --
>> 2.30.2
>>
>> ___
>> dev mailing list
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>
> Hi Han,
>
> the fix is actually a bit more complex as Dumitru mentioned on the first
patch [0].
> The result of the conversation is v2 [1].

Sorry that I didn't notice your patch, and I thought it was just a simple
regression without carefully looking at the original problem. I am replying
to your patch.

Thanks,
Han

>
> Thanks,
> Ales
>
> [0] https://mail.openvswitch.org/pipermail/ovs-dev/2022-August/397165.html
> [1]
https://patchwork.ozlabs.org/project/ovn/patch/20220818094110.160733-1-amu...@redhat.com/
>
>
> --
>
> Ales Musil
>
> Senior Software Engineer - OVN Core
>
> Red Hat EMEA
>
> amu...@redhat.comIM: amusil
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Re: [ovs-dev] [PATCH ovn] physical.c: Fix heap-buffer-overflow.

2022-08-18 Thread Ales Musil
On Fri, Aug 19, 2022 at 4:08 AM Han Zhou  wrote:

> Fix the problem introduced by commit e52c245136, reported by ASAN for test
> case:
> "options:activation-strategy for logical port"
>
> ==1480622==ERROR: AddressSanitizer: heap-buffer-overflow on address
> 0x6110c7d0 at pc 0x004fbd91 bp 0x7ffec4244630 sp 0x7ffec4243de0
> READ of size 168 at 0x6110c7d0 thread T0
> #0 0x4fbd90 in __interceptor_memcpy.part.0
> (/home/hanzhou/src/ovn/_build_as/controller/ovn-controller+0x4fbd90)
> #1 0x73457f in ofpbuf_put
> /home/hanzhou/src/ovs/_build/../lib/ofpbuf.c:431:5
> #2 0x73457f in ofpbuf_put
> /home/hanzhou/src/ovs/_build/../lib/ofpbuf.c:424:1
> #3 0x72ba29 in ofpprop_put
> /home/hanzhou/src/ovs/_build/../lib/ofp-prop.c:294:5
> #4 0x7046e7 in encode_CONTROLLER
> /home/hanzhou/src/ovs/_build/../lib/ofp-actions.c:914:13
> #5 0x7046e7 in encode_ofpact
> /home/hanzhou/src/ovs/_build/../lib/ofp-actions.c:8890:9
> #6 0x705878 in ofpacts_put_openflow_instructions
> /home/hanzhou/src/ovs/_build/../lib/ofp-actions.c:8943:17
> #7 0x716287 in ofputil_encode_flow_mod
> /home/hanzhou/src/ovs/_build/../lib/ofp-flow.c:426:9
> #8 0x5c5978 in encode_flow_mod
> /home/hanzhou/src/ovn/_build_as/../controller/ofctrl.c:1893:12
> #9 0x5c5978 in add_flow_mod
> /home/hanzhou/src/ovn/_build_as/../controller/ofctrl.c:1912:26
> #10 0x5c5978 in installed_flow_add
> /home/hanzhou/src/ovn/_build_as/../controller/ofctrl.c:2254:5
> #11 0x5bfd62 in update_installed_flows_by_track
> /home/hanzhou/src/ovn/_build_as/../controller/ofctrl.c:2484:17
> #12 0x5bc5b9 in ofctrl_put
> /home/hanzhou/src/ovn/_build_as/../controller/ofctrl.c:2715:13
> #13 0x600e97 in main
> /home/hanzhou/src/ovn/_build_as/../controller/ovn-controller.c:4186:25
> #14 0x7f7ae80ce1a1 in __libc_start_main (/lib64/libc.so.6+0x281a1)
> #15 0x49432d in _start
> (/home/hanzhou/src/ovn/_build_as/controller/ovn-controller+0x49432d)
>
> Fixes: e52c245136 ("physical.c: Fix bug of wrong use in vm migration")
> Signed-off-by: Han Zhou 
> ---
>  controller/physical.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/controller/physical.c b/controller/physical.c
> index 552837bcd..c14a752f0 100644
> --- a/controller/physical.c
> +++ b/controller/physical.c
> @@ -1001,6 +1001,7 @@ setup_rarp_activation_strategy(const struct
> sbrec_port_binding *binding,
>
>  load_logical_ingress_metadata(binding, zone_ids, );
>
> +size_t ofs = ofpacts.size;
>  struct ofpact_controller *oc = ofpact_put_CONTROLLER();
>  oc->max_len = UINT16_MAX;
>  oc->reason = OFPR_ACTION;
> @@ -1011,7 +1012,7 @@ setup_rarp_activation_strategy(const struct
> sbrec_port_binding *binding,
>  ofpbuf_put(, , sizeof ah);
>
>  ofpacts.header = oc;
> -oc->userdata_len = ofpacts.size - (sizeof *oc);
> +oc->userdata_len = ofpacts.size - (ofs + sizeof *oc);
>  ofpact_finish_CONTROLLER(, );
>  put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, );
>
> --
> 2.30.2
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Hi Han,

the fix is actually a bit more complex as Dumitru mentioned on the first
patch [0].
The result of the conversation is v2 [1].

Thanks,
Ales

[0] https://mail.openvswitch.org/pipermail/ovs-dev/2022-August/397165.html
[1]
https://patchwork.ozlabs.org/project/ovn/patch/20220818094110.160733-1-amu...@redhat.com/


-- 

Ales Musil

Senior Software Engineer - OVN Core

Red Hat EMEA 

amu...@redhat.comIM: amusil

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev