| Message ID | 20230529043615.4761-1-hbh25y@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: sched: fix possible OOB write in fl_set_geneve_opt() | expand |
[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 <hbh25y@gmail.com> 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 <hbh25y@gmail.com> > > 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 <hbh25y@gmail.com> > > 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 <hbh25y@gmail.com> >> >> 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 security@kernel.org. 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 <hbh25y@gmail.com> > > > > 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 <hbh25y@gmail.com> >>> >>> 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 <hbh25y@gmail.com> > > > > > > > > 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.
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;
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 <hbh25y@gmail.com> --- net/sched/cls_flower.c | 3 +++ 1 file changed, 3 insertions(+)