Re: [PATCH v1] x86: Fix strncat-avx2.S when `src` has no null-term [BZ #30065]

["H.J. Lu" <hjl.tools@gmail.com>]

On Tue, Jan 31, 2023 at 3:09 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
>
> On Tue, Jan 31, 2023 at 5:02 PM H.J. Lu <hjl.tools@gmail.com> wrote:
> >
> > On Tue, Jan 31, 2023 at 2:52 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
> > >
> > > On Tue, Jan 31, 2023 at 4:43 PM H.J. Lu <hjl.tools@gmail.com> wrote:
> > > >
> > > > On Tue, Jan 31, 2023 at 2:37 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
> > > > >
> > > > > On Tue, Jan 31, 2023 at 4:29 PM H.J. Lu <hjl.tools@gmail.com> wrote:
> > > > > >
> > > > > > On Tue, Jan 31, 2023 at 1:37 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
> > > > > > >
> > > > > > > Two issue:
> > > > > > >
> > > > > > > 1) Zero-length check is doing:
> > > > > > > ```
> > > > > > >     test    %rdx, %rdx
> > > > > > >     jl      L(zero_len)
> > > > > > > ```
> > > > > > > which doesn't actually check zero (was at some point `decq` and the
> > > > > > > flag never got updated).
> > > > > > >
> > > > > > > The fix is just make the flag `jle` i.e:
> > > > > > > ```
> > > > > > >     test    %rdx, %rdx
> > > > > > >     jle     L(zero_len)
> > > > > > > ```
> > > > > > >
> > > > > > > 2) Length check in page-cross case checking if we should continue is
> > > > > > > doing:
> > > > > > > ```
> > > > > > >     cmpq    %r8, %rdx
> > > > > > >     jb      L(page_cross_small)
> > > > > > > ```
> > > > > > > which means we will continue searching for null-term if length ends at
> > > > > > > the end of a page and there was no null-term in `src`.
> > > > > >
> > > > > > It is not purely about null-term.   strncat shouldn't read beyond the limit.
> > > > > > In this case, src may point to PROT_NONE memory.
> > > > > >
> > > > >
> > > > > If there was a null-term the later check:
> > > > > ```
> > > > > shll $CHAR_SIZE, %ecx
> > > > > jz L(page_cross_continue)
> > > > > ```
> > > > > would catch it, this can only occur if there is no null-term.
> > > >
> > > > But it is incorrect to read beyond the limit even if there is a null-term.
> > > It's a page-aligned read where len != 0 so the original read should be fine.
> > >
> > > The `jb L(page_cross_small)`
> > > falls through to the null-term check, so if there was a null-term we won't
> > > read anymore bytes.
> >
> > There is no need to search null-term for zero length case.
> >
> This isn't the zero length case, this is the page-cross case. Length
> is != 0 here.

In the zero length case:

        test    %rdx, %rdx
        jl      L(zero_len)

it crashes when src points to an unreadable memory and the limit is 0.
strncat shouldn't read beyond the limit.  The change is OK.   But the
commit log should mention "avoid reading beyond the limit."

> > > >
> > > > > > > The fix is to make the flag:
> > > > > > > ```
> > > > > > >     cmpq    %r8, %rdx
> > > > > > >     jbe     L(page_cross_small)
> > > > > > > ```
> > > > > > > ---
> > > > > > >  string/test-strncat.c                   | 25 ++++++++++++++++++++++++-
> > > > > > >  sysdeps/x86_64/multiarch/strncat-avx2.S |  4 ++--
> > > > > > >  2 files changed, 26 insertions(+), 3 deletions(-)
> > > > > > >
> > > > > > > diff --git a/string/test-strncat.c b/string/test-strncat.c
> > > > > > > index e03d329e1c..c0cde206ee 100644
> > > > > > > --- a/string/test-strncat.c
> > > > > > > +++ b/string/test-strncat.c
> > > > > > > @@ -28,6 +28,7 @@
> > > > > > >  # define CHAR char
> > > > > > >  # define UCHAR unsigned char
> > > > > > >  # define SIMPLE_STRNCAT simple_strncat
> > > > > > > +# define STRNLEN strnlen
> > > > > > >  # define STRLEN strlen
> > > > > > >  # define MEMSET memset
> > > > > > >  # define MEMCPY memcpy
> > > > > > > @@ -40,6 +41,7 @@
> > > > > > >  # define CHAR wchar_t
> > > > > > >  # define UCHAR wchar_t
> > > > > > >  # define SIMPLE_STRNCAT simple_wcsncat
> > > > > > > +# define STRNLEN wcsnlen
> > > > > > >  # define STRLEN wcslen
> > > > > > >  # define MEMSET wmemset
> > > > > > >  # define MEMCPY wmemcpy
> > > > > > > @@ -78,7 +80,7 @@ do_one_test (impl_t *impl, CHAR *dst, const CHAR *src, size_t n)
> > > > > > >        return;
> > > > > > >      }
> > > > > > >
> > > > > > > -  size_t len = STRLEN (src);
> > > > > > > +  size_t len = STRNLEN (src, n);
> > > > > > >    if (MEMCMP (dst + k, src, len + 1 > n ? n : len + 1) != 0)
> > > > > >
> > > > > > >      {
> > > > > > >        error (0, 0, "Incorrect concatenation in function %s",
> > > > > > > @@ -95,6 +97,26 @@ do_one_test (impl_t *impl, CHAR *dst, const CHAR *src, size_t n)
> > > > > > >      }
> > > > > > >  }
> > > > > > >
> > > > > > > +static void
> > > > > > > +do_test_src_no_nullterm_bz30065 (void)
> > > > > > > +{
> > > > > > > +  /* NB: "src does not need to be null-terminated if it contains n or more
> > > > > > > +   * bytes." */
> > > > > > > +  CHAR *s1, *s2;
> > > > > > > +  size_t bound = page_size / sizeof (CHAR);
> > > > > > > +  s1 = (CHAR *) (buf1 + BUF1PAGES * page_size);
> > > > > > > +  s2 = (CHAR *) buf2;
> > > > > > > +  MEMSET (s1 - bound, -1, bound);
> > > > > > > +  for (size_t n = 0; n < bound; ++n)
> > > > > > > +    {
> > > > > > > +      FOR_EACH_IMPL (impl, 0)
> > > > > > > +       {
> > > > > > > +         s2[0] = '\0';
> > > > > > > +         do_one_test (impl, s2, s1 - n, n);
> > > > > > > +       }
> > > > > > > +    }
> > > > > > > +}
> > > > > > > +
> > > > > > >  static void
> > > > > > >  do_test (size_t align1, size_t align2, size_t len1, size_t len2,
> > > > > > >          size_t n, int max_char)
> > > > > > > @@ -372,6 +394,7 @@ test_main (void)
> > > > > > >
> > > > > > >    do_random_tests ();
> > > > > > >    do_overflow_tests ();
> > > > > > > +  do_test_src_no_nullterm_bz30065 ();
> > > > > > >    return ret;
> > > > > > >  }
> > > > > > >
> > > > > > > diff --git a/sysdeps/x86_64/multiarch/strncat-avx2.S b/sysdeps/x86_64/multiarch/strncat-avx2.S
> > > > > > > index b380e8e11c..c2ff202238 100644
> > > > > > > --- a/sysdeps/x86_64/multiarch/strncat-avx2.S
> > > > > > > +++ b/sysdeps/x86_64/multiarch/strncat-avx2.S
> > > > > > > @@ -66,7 +66,7 @@ ENTRY(STRNCAT)
> > > > > > >         salq    $2, %rdx
> > > > > > >  # else
> > > > > > >         test    %rdx, %rdx
> > > > > > > -       jl      L(zero_len)
> > > > > > > +       jle     L(zero_len)
> > > > > > >  # endif
> > > > > > >         vpxor   %VZERO_128, %VZERO_128, %VZERO_128
> > > > > > >
> > > > > > > @@ -387,7 +387,7 @@ L(page_cross):
> > > > > > >         subl    %esi, %r8d
> > > > > > >         andl    $(VEC_SIZE - 1), %r8d
> > > > > > >         cmpq    %r8, %rdx
> > > > > > > -       jb      L(page_cross_small)
> > > > > > > +       jbe     L(page_cross_small)
> > > > > > >
> > > > > > >         /* Optimizing more aggressively for space as this is very cold
> > > > > > >            code. This saves 2x cache lines.  */
> > > > > > > --
> > > > > > > 2.34.1
> > > > > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > > H.J.
> > > >
> > > >
> > > >
> > > > --
> > > > H.J.
> >
> >
> >
> > --
> > H.J.



-- 
H.J.