| Message ID | 1540503710-23597-1-git-send-email-pkusunyifeng@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] NSH: Fix NSH-related length macros that cause stack overflow | expand |
On Thu, Oct 25, 2018 at 02:41:50PM -0700, Yifeng Sun wrote: > In the filed of ver_flags_ttl_len of struct nshhdr, there are only 6 > bits that are used to indicate header's total length in 4-byte words. > Therefore, the max value for total is 252 (63x4), instead of 256 used > in present code base. This patch fixes it. > > Reported-at: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=10855 > Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com> Thanks for the patch and the bug fix. Would you mind adding a few words to the commit message that explains how this can lead to stack overflow? Thanks, Ben.
On Fri, Oct 26, 2018 at 02:55:55PM -0700, Ben Pfaff wrote: > On Thu, Oct 25, 2018 at 02:41:50PM -0700, Yifeng Sun wrote: > > In the filed of ver_flags_ttl_len of struct nshhdr, there are only 6 > > bits that are used to indicate header's total length in 4-byte words. > > Therefore, the max value for total is 252 (63x4), instead of 256 used > > in present code base. This patch fixes it. > > > > Reported-at: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=10855 > > Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com> > > Thanks for the patch and the bug fix. > > Would you mind adding a few words to the commit message that explains > how this can lead to stack overflow? Oops, I accidentally applied this anyway. Never mind on the commit message update.
Thanks Ben for code review. I will leave explanation here for later reference. When md2 context is a string of size 248, the headers's total length would be 256 bytes. Because there is only 6 bits in ver_flags_ttl_len for length value, the value of ver_flags_ttl_len is a wrong value of 0. In format_odp_push_nsh_action(), the first line of code is: size_t mdlen = nsh_hdr_len(nsh_hdr) - NSH_BASE_HDR_LEN; nsh_hdr_len(nsh_hdr) returns 0. As a result, mdlen will be a very large value. Later in this function, when mdlen is used as the upper limit to access md2_ctx, stack overflow will happen. On Fri, Oct 26, 2018 at 3:05 PM Ben Pfaff <blp@ovn.org> wrote: > On Fri, Oct 26, 2018 at 02:55:55PM -0700, Ben Pfaff wrote: > > On Thu, Oct 25, 2018 at 02:41:50PM -0700, Yifeng Sun wrote: > > > In the filed of ver_flags_ttl_len of struct nshhdr, there are only 6 > > > bits that are used to indicate header's total length in 4-byte words. > > > Therefore, the max value for total is 252 (63x4), instead of 256 used > > > in present code base. This patch fixes it. > > > > > > Reported-at: > https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=10855 > > > Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com> > > > > Thanks for the patch and the bug fix. > > > > Would you mind adding a few words to the commit message that explains > > how this can lead to stack overflow? > > Oops, I accidentally applied this anyway. Never mind on the commit > message update. >
diff --git a/datapath/linux/compat/include/net/nsh.h b/datapath/linux/compat/include/net/nsh.h index ff3733d9032d..76894910cfcb 100644 --- a/datapath/linux/compat/include/net/nsh.h +++ b/datapath/linux/compat/include/net/nsh.h @@ -247,10 +247,10 @@ struct nshhdr { #define NSH_M_TYPE1_LEN 24 /* NSH header maximum Length. */ -#define NSH_HDR_MAX_LEN 256 +#define NSH_HDR_MAX_LEN 252 /* NSH context headers maximum Length. */ -#define NSH_CTX_HDRS_MAX_LEN 248 +#define NSH_CTX_HDRS_MAX_LEN 244 static inline struct nshhdr *nsh_hdr(struct sk_buff *skb) { diff --git a/include/openvswitch/nsh.h b/include/openvswitch/nsh.h index 55f59d636e5a..afed932fcb75 100644 --- a/include/openvswitch/nsh.h +++ b/include/openvswitch/nsh.h @@ -263,10 +263,10 @@ struct nsh_hdr { #define NSH_M_TYPE1_LEN 24 /* NSH header maximum Length. */ -#define NSH_HDR_MAX_LEN 256 +#define NSH_HDR_MAX_LEN 252 /* NSH context headers maximum Length. */ -#define NSH_CTX_HDRS_MAX_LEN 248 +#define NSH_CTX_HDRS_MAX_LEN 244 static inline uint16_t nsh_hdr_len(const struct nsh_hdr *nsh)
In the filed of ver_flags_ttl_len of struct nshhdr, there are only 6 bits that are used to indicate header's total length in 4-byte words. Therefore, the max value for total is 252 (63x4), instead of 256 used in present code base. This patch fixes it. Reported-at: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=10855 Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com> --- datapath/linux/compat/include/net/nsh.h | 4 ++-- include/openvswitch/nsh.h | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)