Re: [ovs-dev] [PATCH v2] learn: Fix parsing immediate value for a field match.
On 11/24/22 09:44, Simon Horman wrote:
> On Wed, Nov 23, 2022 at 10:23:37PM +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().
>>
>> While at it, changing to imm.b in function calls that
>> expect byte arrays as an argument. The old way is technically
>> correct, but more error prone.
>>
>> The mf_write_subfield_value() call was also incorrect.
>> However, the 'match' variable is actually not used for
>> anything since checking removal in commit:
>>
>> dd43a558597b ("Do not perform validation in learn_parse();")
>>
>> So, just removing the call and the 'match' variable
>> entirely instead of fixing it.
>>
>> 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
>> Signed-off-by: Ilya Maximets
>
> Reviewed-by: Simon Horman
>
Thanks! Applied and backported down to 2.13.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] learn: Fix parsing immediate value for a field match.
On Wed, Nov 23, 2022 at 10:23:37PM +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().
>
> While at it, changing to imm.b in function calls that
> expect byte arrays as an argument. The old way is technically
> correct, but more error prone.
>
> The mf_write_subfield_value() call was also incorrect.
> However, the 'match' variable is actually not used for
> anything since checking removal in commit:
>
> dd43a558597b ("Do not perform validation in learn_parse();")
>
> So, just removing the call and the 'match' variable
> entirely instead of fixing it.
>
> 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
> Signed-off-by: Ilya Maximets
Reviewed-by: Simon Horman
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v2] learn: Fix parsing immediate value for a field match.
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().
While at it, changing to imm.b in function calls that
expect byte arrays as an argument. The old way is technically
correct, but more error prone.
The mf_write_subfield_value() call was also incorrect.
However, the 'match' variable is actually not used for
anything since checking removal in commit:
dd43a558597b ("Do not perform validation in learn_parse();")
So, just removing the call and the 'match' variable
entirely instead of fixing it.
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
Signed-off-by: Ilya Maximets
---
Version 2:
- Switch from using byte arithmetic on a union mf_value address
to the byte array inside of it. This makes a code a bit more
clear and easier to read.
- Removed the incorrect call to mf_write_subfield_value() along
with the unused 'match' variable.
lib/learn.c| 18 +++---
tests/learn.at | 4 ++--
2 files changed, 9 insertions(+), 13 deletions(-)
diff --git a/lib/learn.c b/lib/learn.c
index a40209ec0..a62add2fd 100644
--- a/lib/learn.c
+++ b/lib/learn.c
@@ -241,7 +241,7 @@ static char * OVS_WARN_UNUSED_RESULT
learn_parse_spec(const char *orig, char *name, char *value,
const struct ofputil_port_map *port_map,
struct ofpact_learn_spec *spec,
- struct ofpbuf *ofpacts, struct match *match)
+ struct ofpbuf *ofpacts)
{
/* Parse destination and check prerequisites. */
struct mf_subfield dst;
@@ -275,14 +275,14 @@ learn_parse_spec(const char *orig, char *name, char
*value,
} else {
char *tail;
/* Partial field value. */
-if (parse_int_string(value, (uint8_t *),
+if (parse_int_string(value, imm.b,
dst.field->n_bytes, )
|| *tail != 0) {
imm_error = xasprintf("%s: cannot parse integer
value", orig);
}
if (!imm_error &&
-!bitwise_is_all_zeros(, dst.field->n_bytes,
+!bitwise_is_all_zeros(imm.b, dst.field->n_bytes,
dst.n_bits,
dst.field->n_bytes * 8 -
dst.n_bits)) {
struct ds ds;
@@ -304,15 +304,13 @@ learn_parse_spec(const char *orig, char *name, char
*value,
spec->src_type = NX_LEARN_SRC_IMMEDIATE;
-/* Update 'match' to allow for satisfying destination
- * prerequisites. */
-mf_write_subfield_value(, , match);
-
/* Push value last, as this may reallocate 'spec'! */
unsigned int imm_bytes = DIV_ROUND_UP(dst.n_bits, 8);
uint8_t *src_imm = ofpbuf_put_zeros(ofpacts,
OFPACT_ALIGN(imm_bytes));
-memcpy(src_imm, , imm_bytes);
+
+memcpy(src_imm, [dst.field->n_bytes - imm_bytes],
+ imm_bytes);
free(error);
return NULL;
@@ -391,7 +389,6 @@ learn_parse__(char *orig, char *arg, const struct
ofputil_port_map *port_map,
struct ofpbuf *ofpacts)
{
struct ofpact_learn *learn;
-struct match match;
char *name, *value;
learn = ofpact_put_LEARN(ofpacts);
@@ -400,7 +397,6 @@ learn_parse__(char *orig, char *arg, const struct
ofputil_port_map *port_map,
learn->priority = OFP_DEFAULT_PRIORITY;
learn->table_id = 1;
-match_init_catchall();
while (ofputil_parse_key_value(, , )) {
if (!strcmp(name, "table")) {
if (!ofputil_table_from_string(value, table_map,
@@ -448,7 +444,7 @@ learn_parse__(char *orig, char *arg, const struct
ofputil_port_map *port_map,
spec = ofpbuf_put_zeros(ofpacts, sizeof *spec);
error =
