| Message ID | 1550602502-17280-1-git-send-email-cpp.code.lv@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,PATCHv3] netlink: added check to prevent netlink attribute overflow | expand |
On 2/19/2019 10:55 AM, Toms Atteka wrote: > If enough large input is passed to odp_actions_from_string it can > cause netlink attribute to overflow. > Check for buffer size was added to prevent entering this function > and returning appropriate error code. > > Basic manual testing was performed. > > Reported-by: > https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=12231 > Signed-off-by: Toms Atteka <cpp.code.lv@gmail.com> > --- > lib/odp-util.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/lib/odp-util.c b/lib/odp-util.c > index e893f46..e288ae8 100644 > --- a/lib/odp-util.c > +++ b/lib/odp-util.c > @@ -2161,6 +2161,10 @@ parse_action_list(const char *s, const struct simap *port_names, > n += retval; > } > > + if (actions->size > UINT16_MAX) { > + return -EFBIG; > + } > + > return n; > } > Hi Toms, Thanks for the patch. Question though, why EFBIG instead of E2BIG? This would appear to be a situation in which too many arguments are sent (E2BIG) but then maybe it is from a file (EFBIG)? Also, I see this is a version 3 of this patch? What changed from version 1 to version 3? Commonly the changes from each version of a patch are posted beneath the git separator '---'. Like below... Thanks, - Greg Signed-off-by: Martin Xu <martinxu9.ovs@gmail.com> CC: Flavio Leitner <fbl@sysclose.org> CC: Yi-Hung Wei <yihung.wei@gmail.com> CC: Yifeng Sun <pkusunyifeng@gmail.com> CC: Zak Whittington <zwhitt.vmware@gmail.com> CC: Ben Pfaff <blp@ovn.org> --- v1->v2: adds "Obsoletes" tag needed for upgrade after renaming adds more reviewers
> On 20 Feb 2019, at 02:27, Gregory Rose <gvrose8192@gmail.com> wrote: > > > On 2/19/2019 10:55 AM, Toms Atteka wrote: >> If enough large input is passed to odp_actions_from_string it can >> cause netlink attribute to overflow. >> Check for buffer size was added to prevent entering this function >> and returning appropriate error code. >> >> Basic manual testing was performed. >> >> Reported-by: >> https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=12231 <https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=12231> >> Signed-off-by: Toms Atteka <cpp.code.lv@gmail.com> <mailto:cpp.code.lv@gmail.com> >> --- >> lib/odp-util.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/lib/odp-util.c b/lib/odp-util.c >> index e893f46..e288ae8 100644 >> --- a/lib/odp-util.c >> +++ b/lib/odp-util.c >> @@ -2161,6 +2161,10 @@ parse_action_list(const char *s, const struct simap *port_names, >> n += retval; >> } >> >> + if (actions->size > UINT16_MAX) { >> + return -EFBIG; >> + } >> + >> return n; >> } >> > Hi Toms, > > Thanks for the patch. Question though, why EFBIG instead of E2BIG? This would appear to be a situation in > which too many arguments are sent (E2BIG) but then maybe it is from a file (EFBIG)? > > Also, I see this is a version 3 of this patch? What changed from version 1 to version 3? Commonly the > changes from each version of a patch are posted beneath the git separator '---'. Like below... > > Thanks, > > - Greg > > Signed-off-by: Martin Xu <martinxu9.ovs@gmail.com> <mailto:martinxu9.ovs@gmail.com> > CC: Flavio Leitner <fbl@sysclose.org> <mailto:martinxu9.ovs@gmail.com> > CC: Yi-Hung Wei <yihung.wei@gmail.com> <mailto:yihung.wei@gmail.com> > CC: Yifeng Sun <pkusunyifeng@gmail.com> <mailto:pkusunyifeng@gmail.com> > CC: Zak Whittington <zwhitt.vmware@gmail.com> <mailto:zwhitt.vmware@gmail.com> > CC: Ben Pfaff <blp@ovn.org> <mailto:blp@ovn.org> > --- > v1->v2: adds "Obsoletes" tag needed for upgrade after renaming > adds more reviewers > > Hi Greg, Thats not a case of too many arguments provided, but the the size of a single argument is too large, so I believe EFBIG is more appropriate. I guess its not worth creating v4 but ill keep in my mind off adding change log next time. Thanks, Tom
On 2/20/2019 1:53 PM, Cpp Code wrote: > >> On 20 Feb 2019, at 02:27, Gregory Rose <gvrose8192@gmail.com >> <mailto:gvrose8192@gmail.com>> wrote: >> >> >> On 2/19/2019 10:55 AM, Toms Atteka wrote: >>> If enough large input is passed to odp_actions_from_string it can >>> cause netlink attribute to overflow. >>> Check for buffer size was added to prevent entering this function >>> and returning appropriate error code. >>> >>> Basic manual testing was performed. >>> >>> Reported-by: >>> https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=12231 >>> Signed-off-by: Toms Atteka<cpp.code.lv@gmail.com> >>> --- >>> lib/odp-util.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/lib/odp-util.c b/lib/odp-util.c >>> index e893f46..e288ae8 100644 >>> --- a/lib/odp-util.c >>> +++ b/lib/odp-util.c >>> @@ -2161,6 +2161,10 @@ parse_action_list(const char *s, const struct simap *port_names, >>> n += retval; >>> } >>> >>> + if (actions->size > UINT16_MAX) { >>> + return -EFBIG; >>> + } >>> + >>> return n; >>> } >>> >> Hi Toms, >> >> Thanks for the patch. Question though, why EFBIG instead of E2BIG? >> This would appear to be a situation in >> which too many arguments are sent (E2BIG) but then maybe it is from a >> file (EFBIG)? >> >> Also, I see this is a version 3 of this patch? What changed from >> version 1 to version 3? Commonly the >> changes from each version of a patch are posted beneath the git >> separator '---'. Like below... >> >> Thanks, >> >> - Greg >> >> Signed-off-by: Martin Xu <martinxu9.ovs@gmail.com> >> CC: Flavio Leitner <fbl@sysclose.org> >> CC: Yi-Hung Wei <yihung.wei@gmail.com> CC: Yifeng Sun >> <pkusunyifeng@gmail.com> CC: Zak Whittington >> <zwhitt.vmware@gmail.com> CC: Ben Pfaff <blp@ovn.org> --- v1->v2: >> adds "Obsoletes" tag needed for upgrade after renaming adds more >> reviewers >> > > Hi Greg, > > > Thats not a case of too many arguments provided, but the the size of a > single argument is too large, so I believe EFBIG is more appropriate. > > I guess its not worth creating v4 but ill keep in my mind off adding > change log next time. > > Thanks, > Tom > OK, thanks for the explanation. You can add my Reviewed-by tag. Reviewed-by: Greg Rose <gvrose8192@gmail.com>
On Tue, Feb 19, 2019 at 10:55:02AM -0800, Toms Atteka wrote: > If enough large input is passed to odp_actions_from_string it can > cause netlink attribute to overflow. > Check for buffer size was added to prevent entering this function > and returning appropriate error code. > > Basic manual testing was performed. > > Reported-by: > https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=12231 > Signed-off-by: Toms Atteka <cpp.code.lv@gmail.com> Thanks, I applied this to master and backported it as far as needed.
diff --git a/lib/odp-util.c b/lib/odp-util.c index e893f46..e288ae8 100644 --- a/lib/odp-util.c +++ b/lib/odp-util.c @@ -2161,6 +2161,10 @@ parse_action_list(const char *s, const struct simap *port_names, n += retval; } + if (actions->size > UINT16_MAX) { + return -EFBIG; + } + return n; }
If enough large input is passed to odp_actions_from_string it can cause netlink attribute to overflow. Check for buffer size was added to prevent entering this function and returning appropriate error code. Basic manual testing was performed. Reported-by: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=12231 Signed-off-by: Toms Atteka <cpp.code.lv@gmail.com> --- lib/odp-util.c | 4 ++++ 1 file changed, 4 insertions(+)