On Sat, Nov 05, 2022 at 11:11:53AM +0100, Ilya Maximets wrote:
> The value is right-justified after the string parsing with
> parse_int_string(), i.e. it is in BE byte order and aligned
> to the right side of the array.
> 
> For example, the 0x10011 value in a 4-byte field will look
> like 0x00 0x01 0x00 0x11.
> 
> However, value copy to the resulted ofpact is performed
> from the start of the memory.  So, in case the destination
> size is smaller than the original field size, incorrect
> part of the value will be copied.
> 
> In the 0x00 0x01 0x00 0x11 example above, if the copy is
> performed to a 3-byte field, the first 3 bytes will be
> copied, which are 0x00 0x01 0x00 instead of 0x01 0x00 0x11.
> 
> This leads to a problem where NXM_NX_REG3[0..16]=0x10011
> turns into NXM_NX_REG3[0..16]=0x100 after the parsing.
> 
> Fix that by offsetting the starting position to the size
> difference in bytes similarly to how it is done in
> learn_parse_load_immediate().
> 
> Fixes: 21b2fa617126 ("ofp-parse: Allow match field names in actions and 
> brackets in matches.")
> Reported-at: 
> https://mail.openvswitch.org/pipermail/ovs-discuss/2022-November/052100.html
> Reported-by: Thomas Lee <[email protected]>
> Signed-off-by: Ilya Maximets <[email protected]>
> ---
>  lib/learn.c    | 4 +++-
>  tests/learn.at | 4 ++--
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/learn.c b/lib/learn.c
> index a40209ec0..cfd762527 100644
> --- a/lib/learn.c
> +++ b/lib/learn.c
> @@ -310,9 +310,11 @@ learn_parse_spec(const char *orig, char *name, char 
> *value,
>  
>                  /* Push value last, as this may reallocate 'spec'! */
>                  unsigned int imm_bytes = DIV_ROUND_UP(dst.n_bits, 8);
> +                unsigned int offset = dst.field->n_bytes - imm_bytes;
>                  uint8_t *src_imm = ofpbuf_put_zeros(ofpacts,
>                                                      OFPACT_ALIGN(imm_bytes));
> -                memcpy(src_imm, &imm, imm_bytes);
> +
> +                memcpy(src_imm, (uint8_t *) &imm + offset, imm_bytes);

FWIIW, This seems fine to me, but I wonder if
it would nicer (and indeed valid) to use &imm.u8 here.

...
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to