[ovs-dev] [PATCH v2] flow: fix incorrect padding length checking and combine branch in ipv6_sanity_check
Ben Pfaff
blp at ovn.org
Wed Aug 28 21:58:13 UTC 2019
On Thu, Aug 22, 2019 at 06:09:34PM +0800, Yanqin Wei wrote:
> The padding length is (packet size - ipv6 header length - ipv6 plen). This
> patch fixes incorrect ipv6 size checking and improves it by combining branch.
>
> Reviewed-by: Gavin Hu <Gavin.Hu at arm.com>
> Signed-off-by: Yanqin Wei <Yanqin.Wei at arm.com>
> ---
> lib/flow.c | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/lib/flow.c b/lib/flow.c
> index e5b554b..1b21f51 100644
> --- a/lib/flow.c
> +++ b/lib/flow.c
> @@ -688,18 +688,16 @@ ipv4_get_nw_frag(const struct ip_header *nh)
> static inline bool
> ipv6_sanity_check(const struct ovs_16aligned_ip6_hdr *nh, size_t size)
> {
> - uint16_t plen;
> + int pad_len;
>
> if (OVS_UNLIKELY(size < sizeof *nh)) {
> return false;
> }
>
> - plen = ntohs(nh->ip6_plen);
> - if (OVS_UNLIKELY(plen + IPV6_HEADER_LEN > size)) {
> - return false;
> - }
> + pad_len = size - IPV6_HEADER_LEN - ntohs(nh->ip6_plen);
The types in the above calculation worry me. Writing the type of each
term:
int = size_t - int - uint16_t
The most likely type of the right side of the expression is size_t.
Assigning this to an 'int' might do "the right thing" for negative
values, but it's risky--especially since size_t and int might be
different widths. I think it would be safer to cast the first and third
terms to int, e.g.:
pad_len = (int) size - IPV6_HEADER_LEN - (int) ntohs(nh->ip6_plen);
> /* Jumbo Payload option not supported yet. */
> - if (OVS_UNLIKELY(size - plen > UINT8_MAX)) {
> + if (OVS_UNLIKELY(pad_len < 0 || pad_len > UINT8_MAX)) {
> return false;
> }
Thanks,
Ben.
More information about the dev
mailing list