New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 854924 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

ec: Pass all host tests with AddressSanitizer enabled

Project Member Reported by drinkcat@chromium.org, Jun 21 2018

Issue description

1. Make sure EC tests can build using LLVM
2. Enable -fsanitizer
3. make runtests should pass
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 21 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/ec/+/5d34998aeb78534ae7659778788cf6d2aec3d742

commit 5d34998aeb78534ae7659778788cf6d2aec3d742
Author: Nicolas Boichat <drinkcat@chromium.org>
Date: Thu Jun 21 15:14:25 2018

test/host_command: Do not overflow req_buf

The test attempts to access req_buf outside of its bounds during
test_hostcmd_too_long, let's increase the buffer size.

BRANCH=none
BUG= chromium:854924 
TEST=make V=1 TEST_ASAN=y run-host_command -j

Change-Id: Ibacc080c9e961ad4eb56c17908e704796404a9ca
Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1109614
Reviewed-by: Vincent Palatin <vpalatin@chromium.org>

[modify] https://crrev.com/5d34998aeb78534ae7659778788cf6d2aec3d742/test/host_command.c

Project Member

Comment 2 by bugdroid1@chromium.org, Jun 21 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/ec/+/161501e092f9509aba66a1bd0217ef6661314dfe

commit 161501e092f9509aba66a1bd0217ef6661314dfe
Author: Nicolas Boichat <drinkcat@chromium.org>
Date: Thu Jun 21 15:14:25 2018

chip/host/gpio.c: Fix out of bounds access

Check if signal is within bounds before accessing
gpio_irq_handlers[signal].

BRANCH=none
BUG= chromium:854924 
TEST=make TEST_ASAN=y runtests -j

Change-Id: Ia1ff9b34943ff596d27b2c746937f31623f58f96
Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1109615
Reviewed-by: Vincent Palatin <vpalatin@chromium.org>

[modify] https://crrev.com/161501e092f9509aba66a1bd0217ef6661314dfe/chip/host/gpio.c

Project Member

Comment 3 by bugdroid1@chromium.org, Jun 21 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/ec/+/aef1ae66381a414a98dcba8efe55990bdd8d5324

commit aef1ae66381a414a98dcba8efe55990bdd8d5324
Author: Nicolas Boichat <drinkcat@chromium.org>
Date: Thu Jun 21 15:14:29 2018

test/flash: Switch to EC_CMD_FLASH_INFO version 1 command

test_flash_info: the response to EC_CMD_FLASH_INFO is now, by
default, a ec_response_flash_info_1 structure, not just
ec_response_flash_info (version 0).

BRANCH=none
BUG= chromium:854924 
TEST=make TEST_ASAN=y run-flash -j

Change-Id: Iebe8d90c3bdee70c481e31d41f173bf1b9a094ad
Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1109657
Reviewed-by: Vincent Palatin <vpalatin@chromium.org>

[modify] https://crrev.com/aef1ae66381a414a98dcba8efe55990bdd8d5324/test/flash.c

Project Member

Comment 4 by bugdroid1@chromium.org, Jun 21 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/ec/+/b8fc9a03a8e41602469f4476a9db9e81031f2187

commit b8fc9a03a8e41602469f4476a9db9e81031f2187
Author: Nicolas Boichat <drinkcat@chromium.org>
Date: Thu Jun 21 15:14:32 2018

test/sha256: Fix parameter to memcmp

clang spots an issue with the parameter, we really want to compare
the whole SHA256 digest.

test/sha256.c:167:33: error: 'memcmp' call operates on objects of type
   'const uint8_t' (aka 'const unsigned char') while the size is based
      on a different type 'const uint8_t *' (aka 'const unsigned char *')
           [-Werror,-Wsizeof-pointer-memaccess]
        if (memcmp(tmp, output, sizeof(output)) != 0) {
                        ~~~~~~         ^~~~~~
test/sha256.c:167:33: note: did you mean to provide an explicit length?
        if (memcmp(tmp, output, sizeof(output)) != 0) {

BRANCH=none
BUG= chromium:854924 
TEST=make CC=clang run-sha256 -j

Change-Id: I7ca875a3981f987b60d62a12be7a4ca8b870b376
Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1109659
Reviewed-by: Vincent Palatin <vpalatin@chromium.org>

[modify] https://crrev.com/b8fc9a03a8e41602469f4476a9db9e81031f2187/test/sha256.c

Project Member

Comment 5 by bugdroid1@chromium.org, Jun 21 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/ec/+/300d9537615249c541d7cba287da919c6c774547

commit 300d9537615249c541d7cba287da919c6c774547
Author: Nicolas Boichat <drinkcat@chromium.org>
Date: Thu Jun 21 15:14:28 2018

chip/host/system: Fix parameter to scanf

value is an integer, so we should use %u, not %lu.

BRANCH=none
BUG= chromium:854924 
TEST=make CC=clang run-flash -j (issue only shows up with clang)

Change-Id: Ifc13792dfed548f897c451790c73ad47aa8496ba
Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1109660
Reviewed-by: Vincent Palatin <vpalatin@chromium.org>

[modify] https://crrev.com/300d9537615249c541d7cba287da919c6c774547/chip/host/system.c

Project Member

Comment 6 by bugdroid1@chromium.org, Jun 22 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/ec/+/8032e90ccbceaf799eeabf6709ab66981b6cf720

commit 8032e90ccbceaf799eeabf6709ab66981b6cf720
Author: Nicolas Boichat <drinkcat@chromium.org>
Date: Fri Jun 22 09:39:42 2018

test/rma_auth: Pad authcode before passing it to rma_try_authcode

rma_try_authcode expects a buffer that is at least RMA_AUTHCODE_CHARS
long, so copy the input string to a buffer before calling the
function, else AddressSanitizer will complain.

BRANCH=none
BUG= chromium:854924 
TEST=make TEST_ASAN=y run-rma_auth -j

Change-Id: Iff2b195a7c7b01b925df6d9f53e0055f98f59ded
Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1109658
Reviewed-by: Randall Spangler <rspangler@chromium.org>

[modify] https://crrev.com/8032e90ccbceaf799eeabf6709ab66981b6cf720/include/rma_auth.h
[modify] https://crrev.com/8032e90ccbceaf799eeabf6709ab66981b6cf720/test/rma_auth.c

Project Member

Comment 7 by bugdroid1@chromium.org, Jun 22 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/ec/+/5bad4a8c77cb01469818c8ae3c45d7acf79f336a

commit 5bad4a8c77cb01469818c8ae3c45d7acf79f336a
Author: Nicolas Boichat <drinkcat@chromium.org>
Date: Fri Jun 22 15:21:26 2018

nvmem_vars: Make sure tuple structure is within bounds

The code uses a 0-byte to mark the end of the nvmem variables
(which corresponds to tuple->key_len), check for that explicitly,
then check if struct tuple fits within the nvmem.

BRANCH=none
BUG= chromium:854924 
TEST=make TEST_ASAN=y run-nvmem_vars -j

Change-Id: I7a974c64dec26c72de955f673d69a0712b023cb2
Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1109616
Reviewed-by: Vadim Bendebury <vbendeb@chromium.org>

[modify] https://crrev.com/5bad4a8c77cb01469818c8ae3c45d7acf79f336a/common/nvmem_vars.c

Project Member

Comment 8 by bugdroid1@chromium.org, Jun 28 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/ec/+/dcfbe0be69d3445edfd45fb036bfae2581b6fdd5

commit dcfbe0be69d3445edfd45fb036bfae2581b6fdd5
Author: Nicolas Boichat <drinkcat@chromium.org>
Date: Thu Jun 28 14:06:46 2018

ec: Make it possible to build tests using clang

We might want to try out address sanitizer/fuzzer on some host
tests: make it possible to build host tests using clang.

Board builds are broken, and there is no intention to fix them,
at least for now.

BRANCH=none
BUG= chromium:854924 
TEST=make buildall -j
TEST=make CC=clang runtests -j

Change-Id: Id49a1b8537bc403d53437a2245f4fab6ceae89ac
Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1107522
Reviewed-by: Vincent Palatin <vpalatin@chromium.org>

[modify] https://crrev.com/dcfbe0be69d3445edfd45fb036bfae2581b6fdd5/common/lightbar.c
[modify] https://crrev.com/dcfbe0be69d3445edfd45fb036bfae2581b6fdd5/include/common.h
[modify] https://crrev.com/dcfbe0be69d3445edfd45fb036bfae2581b6fdd5/common/mkbp_event.c
[modify] https://crrev.com/dcfbe0be69d3445edfd45fb036bfae2581b6fdd5/common/usb_pd_protocol.c
[modify] https://crrev.com/dcfbe0be69d3445edfd45fb036bfae2581b6fdd5/Makefile.toolchain
[modify] https://crrev.com/dcfbe0be69d3445edfd45fb036bfae2581b6fdd5/common/charge_manager.c
[modify] https://crrev.com/dcfbe0be69d3445edfd45fb036bfae2581b6fdd5/include/compile_time_macros.h
[modify] https://crrev.com/dcfbe0be69d3445edfd45fb036bfae2581b6fdd5/common/usb_pd_policy.c
[modify] https://crrev.com/dcfbe0be69d3445edfd45fb036bfae2581b6fdd5/Makefile.rules

Project Member

Comment 9 by bugdroid1@chromium.org, Jun 28 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/ec/+/6c6888037c8d82228b480eeba0eaf1b0aa83e9f8

commit 6c6888037c8d82228b480eeba0eaf1b0aa83e9f8
Author: Nicolas Boichat <drinkcat@chromium.org>
Date: Thu Jun 28 14:06:46 2018

ec: Make it possible to run tests with AddressSanitizer enabled

Automatically use CC=clang if TEST_ASAN is specified.

Also, add a __no_sanitize_address attribute macro to prevent ASan
from adding guards around host_command, mkbp_event, and hook
"arrays" that are generated at link-time.

Also, set ASAN_OPTIONS env variable in run_host_test.

BRANCH=none
BUG= chromium:854924 
TEST=make TEST_ASAN=y runtests -j

Change-Id: Iaf0ec405022760d757a8a9d62a5022460d1b16e1
Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1109661
Reviewed-by: Vincent Palatin <vpalatin@chromium.org>

[modify] https://crrev.com/6c6888037c8d82228b480eeba0eaf1b0aa83e9f8/util/run_host_test
[modify] https://crrev.com/6c6888037c8d82228b480eeba0eaf1b0aa83e9f8/include/mkbp_event.h
[modify] https://crrev.com/6c6888037c8d82228b480eeba0eaf1b0aa83e9f8/include/console.h
[modify] https://crrev.com/6c6888037c8d82228b480eeba0eaf1b0aa83e9f8/include/common.h
[modify] https://crrev.com/6c6888037c8d82228b480eeba0eaf1b0aa83e9f8/include/host_command.h
[modify] https://crrev.com/6c6888037c8d82228b480eeba0eaf1b0aa83e9f8/Makefile.toolchain
[modify] https://crrev.com/6c6888037c8d82228b480eeba0eaf1b0aa83e9f8/include/test_util.h
[modify] https://crrev.com/6c6888037c8d82228b480eeba0eaf1b0aa83e9f8/include/hooks.h
[modify] https://crrev.com/6c6888037c8d82228b480eeba0eaf1b0aa83e9f8/include/extension.h

Project Member

Comment 10 by bugdroid1@chromium.org, Jun 28 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/ec/+/6f38ed23f3d467937f15cf0474b8f323564bcc0d

commit 6f38ed23f3d467937f15cf0474b8f323564bcc0d
Author: Nicolas Boichat <drinkcat@chromium.org>
Date: Thu Jun 28 14:06:45 2018

Makefile.toolchain: Switch from cpp to $(CC) -E

Makes it more straightforward to switch to clang preprocessor later
on.

This requires a few other modifications in the Makefiles (gcc -E does
not read from stdin by default, and does not process files that do not
end in .c/.h).

BRANCH=none
BUG= chromium:854924 
TEST=make buildall -j
TEST=Before and after the change, all ec.bin binaries are identical:
   git checkout m/master
   make REAL_SIGNER=dummy buildall -j
   mv build build-master
   find build-master -type d | sed -e 's/-master//' | \
         xargs -I{} mkdir -p {}
   find build-master -name ec_version.h | sed -e 's|build-master/||' | \
         xargs -I{} cp build-master/{} build/{}
   git checkout this-cl
    => comment out .PHONY: $(out)/ec_version.h in Makefiles.rules
   make REAL_SIGNER=dummy buildall -j
   find build-master -name ec.bin | sed -e 's|build-master/||' | \
         xargs -I{} diff build-master/{} build/{}
    => No difference

Change-Id: If07f033dc7e9a73245499c656562bb906dcd4130
Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1117721
Reviewed-by: Vincent Palatin <vpalatin@chromium.org>

[modify] https://crrev.com/6f38ed23f3d467937f15cf0474b8f323564bcc0d/Makefile.toolchain
[modify] https://crrev.com/6f38ed23f3d467937f15cf0474b8f323564bcc0d/Makefile
[modify] https://crrev.com/6f38ed23f3d467937f15cf0474b8f323564bcc0d/Makefile.rules

Status: Verified (was: Started)
Project Member

Comment 12 by bugdroid1@chromium.org, Jul 6

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/ec/+/66b400fa25992943f1278426b54f3109fb033407

commit 66b400fa25992943f1278426b54f3109fb033407
Author: Nicolas Boichat <drinkcat@chromium.org>
Date: Fri Jul 06 17:09:04 2018

test/pinweaver: Fix clang compilation error

When building with clang -Waddress-of-packed-member, it throws a
warning:
test/pinweaver.c:2032:5: error: taking address of packed member 'data_length'
    of class or structure 'pw_request_header_t' may result in an
      unaligned pointer value [-Werror,-Waddress-of-packed-member]
                        &buf.request.header.data_length);

This is because struct pw_test_data_t is defined as __packed (actually,
its members are), which also implies a base alignment of 1 byte.

Tell the compiler that the structure must be aligned on a 4-byte
boundary, which fixes the issue above.

BRANCH=none
BUG= chromium:854924 
TEST=make buildall -j
TEST=make CC=clang CFLAGS_y=-Waddress-of-packed-member run-pinweaver -j

Change-Id: I498ff311438303b3f648e370af580075dab613a9
Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1111760
Commit-Ready: Allen Webb <allenwebb@google.com>
Reviewed-by: Allen Webb <allenwebb@google.com>

[modify] https://crrev.com/66b400fa25992943f1278426b54f3109fb033407/test/pinweaver.c

Project Member

Comment 13 by bugdroid1@chromium.org, Jul 21

Labels: merge-merged-firmware-cr50-9308.B
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/ec/+/f2de3676a5c82fd427d573a6857cb6931ccbced1

commit f2de3676a5c82fd427d573a6857cb6931ccbced1
Author: Nicolas Boichat <drinkcat@chromium.org>
Date: Sat Jul 21 00:25:16 2018

test/rma_auth: Pad authcode before passing it to rma_try_authcode

rma_try_authcode expects a buffer that is at least RMA_AUTHCODE_CHARS
long, so copy the input string to a buffer before calling the
function, else AddressSanitizer will complain.

BRANCH=none
BUG= chromium:854924 
TEST=make TEST_ASAN=y run-rma_auth -j

Change-Id: Iff2b195a7c7b01b925df6d9f53e0055f98f59ded
Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1109658
Reviewed-by: Randall Spangler <rspangler@chromium.org>
(cherry picked from commit 8032e90ccbceaf799eeabf6709ab66981b6cf720)
Reviewed-on: https://chromium-review.googlesource.com/1145843
Tested-by: Vadim Bendebury <vbendeb@chromium.org>
Commit-Queue: Vadim Bendebury <vbendeb@chromium.org>
Reviewed-by: Vadim Bendebury <vbendeb@chromium.org>

[modify] https://crrev.com/f2de3676a5c82fd427d573a6857cb6931ccbced1/include/rma_auth.h
[modify] https://crrev.com/f2de3676a5c82fd427d573a6857cb6931ccbced1/test/rma_auth.c

Project Member

Comment 14 by bugdroid1@chromium.org, Jul 21

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/ec/+/0a11f94e36fa8886770258f1bca56fb410bde7c5

commit 0a11f94e36fa8886770258f1bca56fb410bde7c5
Author: Nicolas Boichat <drinkcat@chromium.org>
Date: Sat Jul 21 00:25:17 2018

nvmem_vars: Make sure tuple structure is within bounds

The code uses a 0-byte to mark the end of the nvmem variables
(which corresponds to tuple->key_len), check for that explicitly,
then check if struct tuple fits within the nvmem.

BRANCH=none
BUG= chromium:854924 
TEST=make TEST_ASAN=y run-nvmem_vars -j

Change-Id: I7a974c64dec26c72de955f673d69a0712b023cb2
Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1109616
Reviewed-by: Vadim Bendebury <vbendeb@chromium.org>
(cherry picked from commit 5bad4a8c77cb01469818c8ae3c45d7acf79f336a)
Reviewed-on: https://chromium-review.googlesource.com/1145844
Tested-by: Vadim Bendebury <vbendeb@chromium.org>
Commit-Queue: Vadim Bendebury <vbendeb@chromium.org>

[modify] https://crrev.com/0a11f94e36fa8886770258f1bca56fb410bde7c5/common/nvmem_vars.c

Project Member

Comment 15 by bugdroid1@chromium.org, Aug 3

Labels: merge-merged-firmware-cr50-mp-release-9308.87.B
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/ec/+/8a2415859efdbd361dfb0c6f0f9486c9ae55ba34

commit 8a2415859efdbd361dfb0c6f0f9486c9ae55ba34
Author: Nicolas Boichat <drinkcat@chromium.org>
Date: Fri Aug 03 19:50:47 2018

test/rma_auth: Pad authcode before passing it to rma_try_authcode

rma_try_authcode expects a buffer that is at least RMA_AUTHCODE_CHARS
long, so copy the input string to a buffer before calling the
function, else AddressSanitizer will complain.

BRANCH=none
BUG= chromium:854924 
TEST=make TEST_ASAN=y run-rma_auth -j

Change-Id: Iff2b195a7c7b01b925df6d9f53e0055f98f59ded
Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1109658
Reviewed-by: Randall Spangler <rspangler@chromium.org>
(cherry picked from commit 8032e90ccbceaf799eeabf6709ab66981b6cf720)
Reviewed-on: https://chromium-review.googlesource.com/1145843
Tested-by: Vadim Bendebury <vbendeb@chromium.org>
Commit-Queue: Vadim Bendebury <vbendeb@chromium.org>
Reviewed-by: Vadim Bendebury <vbendeb@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1162610
Commit-Queue: Mary Ruthven <mruthven@chromium.org>
Tested-by: Mary Ruthven <mruthven@chromium.org>
Reviewed-by: Mary Ruthven <mruthven@chromium.org>

[modify] https://crrev.com/8a2415859efdbd361dfb0c6f0f9486c9ae55ba34/include/rma_auth.h
[modify] https://crrev.com/8a2415859efdbd361dfb0c6f0f9486c9ae55ba34/test/rma_auth.c

Project Member

Comment 16 by bugdroid1@chromium.org, Aug 3

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/ec/+/373934cd1462dbb8059832baa0793a3549e54c2a

commit 373934cd1462dbb8059832baa0793a3549e54c2a
Author: Nicolas Boichat <drinkcat@chromium.org>
Date: Fri Aug 03 19:50:48 2018

nvmem_vars: Make sure tuple structure is within bounds

The code uses a 0-byte to mark the end of the nvmem variables
(which corresponds to tuple->key_len), check for that explicitly,
then check if struct tuple fits within the nvmem.

BRANCH=none
BUG= chromium:854924 
TEST=make TEST_ASAN=y run-nvmem_vars -j

Change-Id: I7a974c64dec26c72de955f673d69a0712b023cb2
Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1109616
Reviewed-by: Vadim Bendebury <vbendeb@chromium.org>
(cherry picked from commit 5bad4a8c77cb01469818c8ae3c45d7acf79f336a)
Reviewed-on: https://chromium-review.googlesource.com/1145844
Tested-by: Vadim Bendebury <vbendeb@chromium.org>
Commit-Queue: Vadim Bendebury <vbendeb@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1162611
Commit-Queue: Mary Ruthven <mruthven@chromium.org>
Tested-by: Mary Ruthven <mruthven@chromium.org>
Reviewed-by: Mary Ruthven <mruthven@chromium.org>

[modify] https://crrev.com/373934cd1462dbb8059832baa0793a3549e54c2a/common/nvmem_vars.c

Sign in to add a comment