If we send two TCA_FLOWER_KEY_ENC_OPTS_GENEVE packets and their total
size is 252 bytes(key->enc_opts.len = 252) then
key->enc_opts.len = opt->length = data_len / 4 = 0 when the third
TCA_FLOWER_KEY_ENC_OPTS_GENEVE packet enters fl_set_geneve_opt. This
bypasses the next bounds check and results in an out-of-bounds.
Fixes: 0a6e77784f49 ("net/sched: allow flower to match tunnel options")
Signed-off-by: Hangyu Hua <[email protected]>
---
net/sched/cls_flower.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index e960a46b0520..a326fbfe4339 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -1153,6 +1153,9 @@ static int fl_set_geneve_opt(const struct nlattr *nla, struct fl_flow_key *key,
if (option_len > sizeof(struct geneve_opt))
data_len = option_len - sizeof(struct geneve_opt);
+ if (key->enc_opts.len > FLOW_DIS_TUN_OPTS_MAX - 4)
+ return -ERANGE;
+
opt = (struct geneve_opt *)&key->enc_opts.data[key->enc_opts.len];
memset(opt, 0xff, option_len);
opt->length = data_len / 4;
--
2.34.1
[Updated Pieter's email address, dropped old email address of mine]
On Mon, May 29, 2023 at 12:36:15PM +0800, Hangyu Hua wrote:
> If we send two TCA_FLOWER_KEY_ENC_OPTS_GENEVE packets and their total
> size is 252 bytes(key->enc_opts.len = 252) then
> key->enc_opts.len = opt->length = data_len / 4 = 0 when the third
> TCA_FLOWER_KEY_ENC_OPTS_GENEVE packet enters fl_set_geneve_opt. This
> bypasses the next bounds check and results in an out-of-bounds.
>
> Fixes: 0a6e77784f49 ("net/sched: allow flower to match tunnel options")
> Signed-off-by: Hangyu Hua <[email protected]>
Hi Hangyu Hua,
Thanks. I think I see the problem too.
But I do wonder, is this more general than Geneve options?
That is, can this occur with any sequence of options, that
consume space in enc_opts (configured in fl_set_key()) that
in total are more than 256 bytes?
> ---
> net/sched/cls_flower.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
> index e960a46b0520..a326fbfe4339 100644
> --- a/net/sched/cls_flower.c
> +++ b/net/sched/cls_flower.c
> @@ -1153,6 +1153,9 @@ static int fl_set_geneve_opt(const struct nlattr *nla, struct fl_flow_key *key,
> if (option_len > sizeof(struct geneve_opt))
> data_len = option_len - sizeof(struct geneve_opt);
>
> + if (key->enc_opts.len > FLOW_DIS_TUN_OPTS_MAX - 4)
> + return -ERANGE;
> +
> opt = (struct geneve_opt *)&key->enc_opts.data[key->enc_opts.len];
> memset(opt, 0xff, option_len);
> opt->length = data_len / 4;
> --
> 2.34.1
>
>
On 30/05/2023 12:36, Simon Horman wrote:
> [Updated Pieter's email address, dropped old email address of mine]
Thank you Simon.
>
> On Mon, May 29, 2023 at 12:36:15PM +0800, Hangyu Hua wrote:
>> If we send two TCA_FLOWER_KEY_ENC_OPTS_GENEVE packets and their total
>> size is 252 bytes(key->enc_opts.len = 252) then
>> key->enc_opts.len = opt->length = data_len / 4 = 0 when the third
>> TCA_FLOWER_KEY_ENC_OPTS_GENEVE packet enters fl_set_geneve_opt. This
>> bypasses the next bounds check and results in an out-of-bounds.
>>
>> Fixes: 0a6e77784f49 ("net/sched: allow flower to match tunnel options")
>> Signed-off-by: Hangyu Hua <[email protected]>
>
> Hi Hangyu Hua,
>
> Thanks. I think I see the problem too.
> But I do wonder, is this more general than Geneve options?
> That is, can this occur with any sequence of options, that
> consume space in enc_opts (configured in fl_set_key()) that
> in total are more than 256 bytes?
>
Hi Hangyu Hua,
Thank you for the patch. In addition to Simon's comment; I think the subject
headline should include net, i.e. [PATCH net]. Also could you please provide
an example tc filter add dev... command to replicate the issue? (Just to make
it a bit easier to understand).
>> ---
>> net/sched/cls_flower.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
>> index e960a46b0520..a326fbfe4339 100644
>> --- a/net/sched/cls_flower.c
>> +++ b/net/sched/cls_flower.c
>> @@ -1153,6 +1153,9 @@ static int fl_set_geneve_opt(const struct nlattr *nla, struct fl_flow_key *key,
>> if (option_len > sizeof(struct geneve_opt))
>> data_len = option_len - sizeof(struct geneve_opt);
>>
>> + if (key->enc_opts.len > FLOW_DIS_TUN_OPTS_MAX - 4)
>> + return -ERANGE;
>> +
>> opt = (struct geneve_opt *)&key->enc_opts.data[key->enc_opts.len];
>> memset(opt, 0xff, option_len);
>> opt->length = data_len / 4;
>> --
>> 2.34.1
>>
>>
On 30/5/2023 19:36, Simon Horman wrote:
> [Updated Pieter's email address, dropped old email address of mine]
>
> On Mon, May 29, 2023 at 12:36:15PM +0800, Hangyu Hua wrote:
>> If we send two TCA_FLOWER_KEY_ENC_OPTS_GENEVE packets and their total
>> size is 252 bytes(key->enc_opts.len = 252) then
>> key->enc_opts.len = opt->length = data_len / 4 = 0 when the third
>> TCA_FLOWER_KEY_ENC_OPTS_GENEVE packet enters fl_set_geneve_opt. This
>> bypasses the next bounds check and results in an out-of-bounds.
>>
>> Fixes: 0a6e77784f49 ("net/sched: allow flower to match tunnel options")
>> Signed-off-by: Hangyu Hua <[email protected]>
>
> Hi Hangyu Hua,
>
> Thanks. I think I see the problem too.
> But I do wonder, is this more general than Geneve options?
> That is, can this occur with any sequence of options, that
> consume space in enc_opts (configured in fl_set_key()) that
> in total are more than 256 bytes?
>
I think you are right. It is a good idea to add check in
fl_set_vxlan_opt and fl_set_erspan_opt and fl_set_gtp_opt too.
But they should be submitted as other patches. fl_set_geneve_opt has
already check this with the following code:
static int fl_set_geneve_opt(const struct nlattr *nla, struct
fl_flow_key *key,
int depth, int option_len,
struct netlink_ext_ack *extack)
{
...
if (new_len > FLOW_DIS_TUN_OPTS_MAX) {
NL_SET_ERR_MSG(extack, "Tunnel options exceeds max size");
return -ERANGE;
}
...
}
This bug will only be triggered under this special
condition(key->enc_opts.len = 252). So I think it will be better
understood by submitting this patch independently.
By the way, I think memset's third param should be option_len in
fl_set_vxlan_opt and fl_set_erspan_opt. Do I need to submit another
patch to fix all these issues?
Thanks,
Hangyu
>> ---
>> net/sched/cls_flower.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
>> index e960a46b0520..a326fbfe4339 100644
>> --- a/net/sched/cls_flower.c
>> +++ b/net/sched/cls_flower.c
>> @@ -1153,6 +1153,9 @@ static int fl_set_geneve_opt(const struct nlattr *nla, struct fl_flow_key *key,
>> if (option_len > sizeof(struct geneve_opt))
>> data_len = option_len - sizeof(struct geneve_opt);
>>
>> + if (key->enc_opts.len > FLOW_DIS_TUN_OPTS_MAX - 4)
>> + return -ERANGE;
>> +
>> opt = (struct geneve_opt *)&key->enc_opts.data[key->enc_opts.len];
>> memset(opt, 0xff, option_len);
>> opt->length = data_len / 4;
>> --
>> 2.34.1
>>
>>
On 30/5/2023 22:29, Pieter Jansen van Vuuren wrote:
>
>
> On 30/05/2023 12:36, Simon Horman wrote:
>> [Updated Pieter's email address, dropped old email address of mine]
>
> Thank you Simon.
>
>>
>> On Mon, May 29, 2023 at 12:36:15PM +0800, Hangyu Hua wrote:
>>> If we send two TCA_FLOWER_KEY_ENC_OPTS_GENEVE packets and their total
>>> size is 252 bytes(key->enc_opts.len = 252) then
>>> key->enc_opts.len = opt->length = data_len / 4 = 0 when the third
>>> TCA_FLOWER_KEY_ENC_OPTS_GENEVE packet enters fl_set_geneve_opt. This
>>> bypasses the next bounds check and results in an out-of-bounds.
>>>
>>> Fixes: 0a6e77784f49 ("net/sched: allow flower to match tunnel options")
>>> Signed-off-by: Hangyu Hua <[email protected]>
>>
>> Hi Hangyu Hua,
>>
>> Thanks. I think I see the problem too.
>> But I do wonder, is this more general than Geneve options?
>> That is, can this occur with any sequence of options, that
>> consume space in enc_opts (configured in fl_set_key()) that
>> in total are more than 256 bytes?
>>
>
> Hi Hangyu Hua,
>
> Thank you for the patch. In addition to Simon's comment; I think the subject
> headline should include net, i.e. [PATCH net]. Also could you please provide
My bad. I forgot this rule. It seems this won't be included in the final
patch. Do i need to send a v2?
> an example tc filter add dev... command to replicate the issue? (Just to make
> it a bit easier to understand).
>
I use poc.c instead of commands to trigger this bug. If you want poc You
can check if there is an email named "Re: A possible LPE vulnerability
in fl_set_geneve_opt" in your e-mail. I should have sent it to you and
Simon while replying to [email protected].
Thanks,
Hangyu
>>> ---
>>> net/sched/cls_flower.c | 3 +++
>>> 1 file changed, 3 insertions(+)
>>>
>>> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
>>> index e960a46b0520..a326fbfe4339 100644
>>> --- a/net/sched/cls_flower.c
>>> +++ b/net/sched/cls_flower.c
>>> @@ -1153,6 +1153,9 @@ static int fl_set_geneve_opt(const struct nlattr *nla, struct fl_flow_key *key,
>>> if (option_len > sizeof(struct geneve_opt))
>>> data_len = option_len - sizeof(struct geneve_opt);
>>>
>>> + if (key->enc_opts.len > FLOW_DIS_TUN_OPTS_MAX - 4)
>>> + return -ERANGE;
>>> +
>>> opt = (struct geneve_opt *)&key->enc_opts.data[key->enc_opts.len];
>>> memset(opt, 0xff, option_len);
>>> opt->length = data_len / 4;
>>> --
>>> 2.34.1
>>>
>>>
On Wed, May 31, 2023 at 01:38:49PM +0800, Hangyu Hua wrote:
> On 30/5/2023 19:36, Simon Horman wrote:
> > [Updated Pieter's email address, dropped old email address of mine]
> >
> > On Mon, May 29, 2023 at 12:36:15PM +0800, Hangyu Hua wrote:
> > > If we send two TCA_FLOWER_KEY_ENC_OPTS_GENEVE packets and their total
> > > size is 252 bytes(key->enc_opts.len = 252) then
> > > key->enc_opts.len = opt->length = data_len / 4 = 0 when the third
> > > TCA_FLOWER_KEY_ENC_OPTS_GENEVE packet enters fl_set_geneve_opt. This
> > > bypasses the next bounds check and results in an out-of-bounds.
> > >
> > > Fixes: 0a6e77784f49 ("net/sched: allow flower to match tunnel options")
> > > Signed-off-by: Hangyu Hua <[email protected]>
> >
> > Hi Hangyu Hua,
> >
> > Thanks. I think I see the problem too.
> > But I do wonder, is this more general than Geneve options?
> > That is, can this occur with any sequence of options, that
> > consume space in enc_opts (configured in fl_set_key()) that
> > in total are more than 256 bytes?
> >
>
> I think you are right. It is a good idea to add check in fl_set_vxlan_opt
> and fl_set_erspan_opt and fl_set_gtp_opt too.
> But they should be submitted as other patches. fl_set_geneve_opt has already
> check this with the following code:
>
> static int fl_set_geneve_opt(const struct nlattr *nla, struct fl_flow_key
> *key,
> int depth, int option_len,
> struct netlink_ext_ack *extack)
> {
> ...
> if (new_len > FLOW_DIS_TUN_OPTS_MAX) {
> NL_SET_ERR_MSG(extack, "Tunnel options exceeds max size");
> return -ERANGE;
> }
> ...
> }
>
> This bug will only be triggered under this special
> condition(key->enc_opts.len = 252). So I think it will be better understood
> by submitting this patch independently.
A considered approach sounds good to me.
I do wonder, could the bounds checks be centralised in the caller?
Maybe not if it doesn't know the length that will be consumed.
> By the way, I think memset's third param should be option_len in
> fl_set_vxlan_opt and fl_set_erspan_opt. Do I need to submit another patch to
> fix all these issues?
I think that in general one fix per patch is best.
Some minor nits.
1. As this is a fix for networking code it is probably targeted
at the net, as opposed to net-next, tree. This should be indicated
in the patch subject.
Subject: [PATCH net v2] ...
2. I think the usual patch prefix for this file, of late,
has been 'net/sched: flower: '
Subject: [PATCH net v2] net/sched: flower: ...
On 31/5/2023 16:04, Simon Horman wrote:
> On Wed, May 31, 2023 at 01:38:49PM +0800, Hangyu Hua wrote:
>> On 30/5/2023 19:36, Simon Horman wrote:
>>> [Updated Pieter's email address, dropped old email address of mine]
>>>
>>> On Mon, May 29, 2023 at 12:36:15PM +0800, Hangyu Hua wrote:
>>>> If we send two TCA_FLOWER_KEY_ENC_OPTS_GENEVE packets and their total
>>>> size is 252 bytes(key->enc_opts.len = 252) then
>>>> key->enc_opts.len = opt->length = data_len / 4 = 0 when the third
>>>> TCA_FLOWER_KEY_ENC_OPTS_GENEVE packet enters fl_set_geneve_opt. This
>>>> bypasses the next bounds check and results in an out-of-bounds.
>>>>
>>>> Fixes: 0a6e77784f49 ("net/sched: allow flower to match tunnel options")
>>>> Signed-off-by: Hangyu Hua <[email protected]>
>>>
>>> Hi Hangyu Hua,
>>>
>>> Thanks. I think I see the problem too.
>>> But I do wonder, is this more general than Geneve options?
>>> That is, can this occur with any sequence of options, that
>>> consume space in enc_opts (configured in fl_set_key()) that
>>> in total are more than 256 bytes?
>>>
>>
>> I think you are right. It is a good idea to add check in fl_set_vxlan_opt
>> and fl_set_erspan_opt and fl_set_gtp_opt too.
>> But they should be submitted as other patches. fl_set_geneve_opt has already
>> check this with the following code:
>>
>> static int fl_set_geneve_opt(const struct nlattr *nla, struct fl_flow_key
>> *key,
>> int depth, int option_len,
>> struct netlink_ext_ack *extack)
>> {
>> ...
>> if (new_len > FLOW_DIS_TUN_OPTS_MAX) {
>> NL_SET_ERR_MSG(extack, "Tunnel options exceeds max size");
>> return -ERANGE;
>> }
>> ...
>> }
>>
>> This bug will only be triggered under this special
>> condition(key->enc_opts.len = 252). So I think it will be better understood
>> by submitting this patch independently.
>
> A considered approach sounds good to me.
>
> I do wonder, could the bounds checks be centralised in the caller?
> Maybe not if it doesn't know the length that will be consumed.
>
This may make code more complex. I am not sure if it is necessary to do
this.
>> By the way, I think memset's third param should be option_len in
>> fl_set_vxlan_opt and fl_set_erspan_opt. Do I need to submit another patch to
>> fix all these issues?
>
> I think that in general one fix per patch is best.
I see. I will try to handle these issues.
>
> Some minor nits.
>
> 1. As this is a fix for networking code it is probably targeted
> at the net, as opposed to net-next, tree. This should be indicated
> in the patch subject.
>
> Subject: [PATCH net v2] ...
>
> 2. I think the usual patch prefix for this file, of late,
> has been 'net/sched: flower: '
>
> Subject: [PATCH net v2] net/sched: flower: ...
>
Get it. I will send a v2 later.
On Wed, May 31, 2023 at 06:05:29PM +0800, Hangyu Hua wrote:
> On 31/5/2023 16:04, Simon Horman wrote:
> > On Wed, May 31, 2023 at 01:38:49PM +0800, Hangyu Hua wrote:
> > > On 30/5/2023 19:36, Simon Horman wrote:
> > > > [Updated Pieter's email address, dropped old email address of mine]
> > > >
> > > > On Mon, May 29, 2023 at 12:36:15PM +0800, Hangyu Hua wrote:
> > > > > If we send two TCA_FLOWER_KEY_ENC_OPTS_GENEVE packets and their total
> > > > > size is 252 bytes(key->enc_opts.len = 252) then
> > > > > key->enc_opts.len = opt->length = data_len / 4 = 0 when the third
> > > > > TCA_FLOWER_KEY_ENC_OPTS_GENEVE packet enters fl_set_geneve_opt. This
> > > > > bypasses the next bounds check and results in an out-of-bounds.
> > > > >
> > > > > Fixes: 0a6e77784f49 ("net/sched: allow flower to match tunnel options")
> > > > > Signed-off-by: Hangyu Hua <[email protected]>
> > > >
> > > > Hi Hangyu Hua,
> > > >
> > > > Thanks. I think I see the problem too.
> > > > But I do wonder, is this more general than Geneve options?
> > > > That is, can this occur with any sequence of options, that
> > > > consume space in enc_opts (configured in fl_set_key()) that
> > > > in total are more than 256 bytes?
> > > >
> > >
> > > I think you are right. It is a good idea to add check in fl_set_vxlan_opt
> > > and fl_set_erspan_opt and fl_set_gtp_opt too.
> > > But they should be submitted as other patches. fl_set_geneve_opt has already
> > > check this with the following code:
> > >
> > > static int fl_set_geneve_opt(const struct nlattr *nla, struct fl_flow_key
> > > *key,
> > > int depth, int option_len,
> > > struct netlink_ext_ack *extack)
> > > {
> > > ...
> > > if (new_len > FLOW_DIS_TUN_OPTS_MAX) {
> > > NL_SET_ERR_MSG(extack, "Tunnel options exceeds max size");
> > > return -ERANGE;
> > > }
> > > ...
> > > }
> > >
> > > This bug will only be triggered under this special
> > > condition(key->enc_opts.len = 252). So I think it will be better understood
> > > by submitting this patch independently.
> >
> > A considered approach sounds good to me.
> >
> > I do wonder, could the bounds checks be centralised in the caller?
> > Maybe not if it doesn't know the length that will be consumed.
> >
>
> This may make code more complex. I am not sure if it is necessary to do
> this.
Understood. I agree that complex seems undesirable.