Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New fortify warning on ARCH=arm in drivers/iio/pressure/dlhl60d.c after -next commit 0f4459080884 #2000

Open
nathanchance opened this issue Feb 22, 2024 · 3 comments
Labels
[FIXED][LINUX] development cycle This bug was only present and fixed in a -next or -rc cycle loop unroller [WORKAROUND] Applied This bug has an applied workaround

Comments

@nathanchance
Copy link
Member

After commit 0f4459080884 ("fortify: Split reporting and avoid passing string pointer") in -next, I see a new fortify warning in drivers/iio/pressure/dlhl60d.c with a couple of distribution configurations (such as OpenSUSE's):

$ curl -LSso .config https://github.com/openSUSE/kernel-source/raw/master/config/armv7hl/default

$ make -skj"$(nproc)" ARCH=arm LLVM=1 olddefconfig drivers/iio/pressure/dlhl60d.o
In file included from drivers/iio/pressure/dlhl60d.c:11:
In file included from include/linux/module.h:17:
In file included from include/linux/kmod.h:9:
In file included from include/linux/umh.h:4:
In file included from include/linux/gfp.h:7:
In file included from include/linux/mmzone.h:8:
In file included from include/linux/spinlock.h:63:
In file included from include/linux/lockdep.h:14:
In file included from include/linux/smp.h:13:
In file included from include/linux/cpumask.h:12:
In file included from include/linux/bitmap.h:12:
In file included from include/linux/string.h:360:
include/linux/fortify-string.h:548:4: warning: call to '__write_overflow_field' declared with 'warning' attribute: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Wattribute-warning]
  548 |                         __write_overflow_field(p_size_field, size);
      |                         ^
1 warning generated.
# bad: [e31185ce00a96232308300008db193416ceb9769] Add linux-next specific files for 20240222
# good: [39133352cbed6626956d38ed72012f49b0421e7b] Merge tag 'for-linus' of git://git.kernel.org/pub/scm/virt/kvm/kvm
git bisect start 'e31185ce00a96232308300008db193416ceb9769' '39133352cbed6626956d38ed72012f49b0421e7b'
# good: [9d966fba1daf1f47534a747ad455d78f7a681480] Merge branch 'main' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git
git bisect good 9d966fba1daf1f47534a747ad455d78f7a681480
# good: [e40e65356855b1ab0e2c106fa54a4249249bd04c] Merge branch 'next' of git://github.com/cschaufler/smack-next
git bisect good e40e65356855b1ab0e2c106fa54a4249249bd04c
# good: [1505a31920eaf4ed023a84e38df0e754eaa50fdd] Merge branch 'tty-next' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git
git bisect good 1505a31920eaf4ed023a84e38df0e754eaa50fdd
# good: [31fd82901c598044a363ec0a8195453720222c53] Merge branch 'gpio/for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git
git bisect good 31fd82901c598044a363ec0a8195453720222c53
# good: [30d8a5235bfd01dd9b70a7e45f30d76c27a8c142] Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git
git bisect good 30d8a5235bfd01dd9b70a7e45f30d76c27a8c142
# good: [b64f3122070806db5166a6854439fb126afcc33a] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/krisman/unicode.git
git bisect good b64f3122070806db5166a6854439fb126afcc33a
# good: [f7d7bd92d6d33147a3bef8ab62a8f463edba20cc] Merge branch 'for-next/execve' of git://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git
git bisect good f7d7bd92d6d33147a3bef8ab62a8f463edba20cc
# good: [071ad962baf5e857fd965595421cf6fb588610ed] bitmap: Step down as a reviewer
git bisect good 071ad962baf5e857fd965595421cf6fb588610ed
# good: [7b3133aa4b9eba9cdf3905e3f7c8b6687ff4615b] hardening: drop obsolete DRM_LEGACY from config fragment
git bisect good 7b3133aa4b9eba9cdf3905e3f7c8b6687ff4615b
# good: [53003a74bdc005a4517de0ab0edca70ba28b0edc] refcount: Annotated intentional signed integer wrap-around
git bisect good 53003a74bdc005a4517de0ab0edca70ba28b0edc
# bad: [3b03afc4ebbb1f59721619e9f9ed45a2bb6ea967] fortify: Improve buffer overflow reporting
git bisect bad 3b03afc4ebbb1f59721619e9f9ed45a2bb6ea967
# bad: [d5ea5b5291a50c2be632373059ac7f535085b19e] fortify: Allow KUnit test to build without FORTIFY
git bisect bad d5ea5b5291a50c2be632373059ac7f535085b19e
# bad: [0f4459080884b688c4ec9c2b121f22c641401bc1] fortify: Split reporting and avoid passing string pointer
git bisect bad 0f4459080884b688c4ec9c2b121f22c641401bc1
# first bad commit: [0f4459080884b688c4ec9c2b121f22c641401bc1] fortify: Split reporting and avoid passing string pointer

This happens with LLVM 18, so this does not appear to be any changed compiler behavior (but I did not check older releases).

I have done no further investigation to see if this warning is legitimate or not.

@nathanchance nathanchance added the [BUG] Untriaged Something isn't working label Feb 22, 2024
@kees
Copy link

kees commented Feb 22, 2024

FWIW, I cannot reproduce this with GCC. The offending call is in dlh_trigger_handler():

                memcpy(tmp_buf + i,
                        &st->rx_buf[1] + chn * DLH_NUM_DATA_BYTES,
                        DLH_NUM_DATA_BYTES);

I'm checking how the fortify split could have possibly triggered this...

@kees
Copy link

kees commented Feb 22, 2024

This, again, appears to be a loop unrolling issue. :(

However, the iio code is also making a lot of assumptions. I've sent an RFC:
https://lore.kernel.org/lkml/20240222222335.work.759-kees@kernel.org

roxell pushed a commit to roxell/linux that referenced this issue Feb 26, 2024
3 bytes were being read but 4 were being written. Explicitly initialize
the unused bytes to 0 and refactor the loop to use direct array
indexing, which appears to silence a Clang false positive warning[1].

Indent improvement included for readability of the fixed code.

Link: ClangBuiltLinux#2000 [1]
Fixes: ac78c6a ("iio: pressure: Add driver for DLH pressure sensors")
Signed-off-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Link: https://lore.kernel.org/r/20240223172936.it.875-kees@kernel.org
Cc: <Stable@vger.kerenl.org>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
@nathanchance
Copy link
Member Author

@nathanchance nathanchance added [PATCH] Accepted A submitted patch has been accepted upstream [WORKAROUND] Applied This bug has an applied workaround [FIXED][LINUX] development cycle This bug was only present and fixed in a -next or -rc cycle and removed [BUG] Untriaged Something isn't working [PATCH] Accepted A submitted patch has been accepted upstream labels Feb 27, 2024
TheOrangeCat pushed a commit to TheOrangeCat/linux that referenced this issue Mar 20, 2024
3 bytes were being read but 4 were being written. Explicitly initialize
the unused bytes to 0 and refactor the loop to use direct array
indexing, which appears to silence a Clang false positive warning[1].

Indent improvement included for readability of the fixed code.

Link: ClangBuiltLinux/linux#2000 [1]
Fixes: ac78c6a ("iio: pressure: Add driver for DLH pressure sensors")
Signed-off-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Link: https://lore.kernel.org/r/20240223172936.it.875-kees@kernel.org
Cc: <Stable@vger.kerenl.org>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[FIXED][LINUX] development cycle This bug was only present and fixed in a -next or -rc cycle loop unroller [WORKAROUND] Applied This bug has an applied workaround
Projects
None yet
Development

No branches or pull requests

2 participants