New issue
Advanced search Search tips

Issue 749260 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Mac
Pri: 1
Type: Bug-Security



Sign in to add a comment

Crash in _sk_gather_bgra_avx

Project Member Reported by ClusterFuzz, Jul 26 2017

Issue description

Detailed report: https://clusterfuzz.com/testcase?key=6409534427103232

Fuzzer: inferno_twister_c
Job Type: linux_asan_chrome_media
Platform Id: linux

Crash Type: UNKNOWN READ
Crash Address: 0x7f9dbda98ff8
Crash State:
  _sk_gather_bgra_avx
  antifilldot8
  SkScan::AntiFillRect
  
Sanitizer: address (ASAN)

Recommended Security Severity: Medium

Regressed: https://clusterfuzz.com/revisions?job=linux_asan_chrome_media&range=488849:488981

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=6409534427103232


Issue filed automatically.

See https://github.com/google/clusterfuzz-tools for more information.
 

Comment 1 by vakh@chromium.org, Jul 27 2017

Components: Internals>Skia
Owner: mtklein@chromium.org
Status: Assigned (was: Untriaged)
Project Member

Comment 2 by sheriffbot@chromium.org, Jul 27 2017

Labels: M-61
Project Member

Comment 3 by sheriffbot@chromium.org, Jul 27 2017

Labels: ReleaseBlock-Stable
This is a serious security regression. If you are not able to fix this quickly, please revert the change that introduced it.

If this doesn't affect a release branch, or has not been properly classified for severity, please update the Security_Impact or Security_Severity labels, and remove the ReleaseBlock label. To disable this altogether, apply ReleaseBlock-NA.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 4 by sheriffbot@chromium.org, Jul 27 2017

Labels: Pri-1
Project Member

Comment 5 by sheriffbot@chromium.org, Jul 28 2017

Labels: -Security_Impact-Head Security_Impact-Beta

Comment 6 by awhalley@google.com, Jul 28 2017

Labels: -Security_Impact-Beta -M-61 Security_Impact-Head M-62
Labels: OS-Mac
Some notes to myself in case I don't finish this right now:

I've been unable to reproduce this with the minimized test case, but I can with the original.

~/c/src (clean|✔) $ cat out/Default/args.gn
# Build arguments go here.
# See "gn args <out_dir> --list" for available build arguments.
is_asan = true
is_component_build = false
is_debug = false
is_lsan = true
use_goma = true

export ASAN_OPTIONS="redzone=64:allow_user_segv_handler=0:symbolize=1:detect_stack_use_after_return=1:alloc_dealloc_mismatch=0:detect_leaks=0:print_scariness=1:check_malloc_usable_size=0:max_uar_stack_size_log=16:use_sigaltstack=1:handle_abort=1:strict_memcmp=0:detect_container_overflow=1:coverage=0:detect_odr_violation=0:allocator_may_return_null=1:handle_sigill=1:handle_segv=1:fast_unwind_on_fatal=1"

ninja -C out/Default chrome; and out/Default/Chromium.app/Contents/MacOS/Chromium --disable-gpu-rasterization --user-data-dir=/tmp/chromium --no-first-run --disable-in-process-stack-traces ~/Downloads/clusterfuzz-testcase-6409534427103232.xml
The crashing pipeline is

   seed_shader
   matrix_translate
   repeat_x
   clamp_y
   gather_bgra
   load_bgra_dst
   srcover
   store_bgra

So, a pretty normal drawRect() + image shader, all BGRA (kN32) as expected.
Looks like the index of the pixels we're loading in gather_bgra is underflowing (to 4294967294), despite being tiled by repeat_x/clamp_y.
x -2, y 0.5, stride 9, loading 0x14bb81000 + 4294967294
Cc: fmalita@chromium.org
Project Member

Comment 12 by bugdroid1@chromium.org, Aug 1 2017

The following revision refers to this bug:
  https://skia.googlesource.com/skia/+/249ee1f985b4bad78db2d5cfdd14ce38edb2c23a

commit 249ee1f985b4bad78db2d5cfdd14ce38edb2c23a
Author: Mike Klein <mtklein@chromium.org>
Date: Tue Aug 01 19:15:33 2017

clamp to 0 in repeat and mirror image tilers

If we were doing this math with real numbers or even just doubles, these
clamps wouldn't be necessary.  But we're favoring speed over accuracy
here when we emulate fmod() and some of those inaccuracies end up with
values outside the [0,tile) range, negative!

To keep the spirit of fast over 100% accurate, I've just added a safety
clamp to 0.  The case in the unit test now returns 0 where it should
really return something like 7 or 8, but at least we won't try to read
_way_ outside the image buffer.

BUG= chromium:749260 

Change-Id: Ifc5cfe69798beccbb2a16547510158576e06eb3a
Reviewed-on: https://skia-review.googlesource.com/29580
Reviewed-by: Florin Malita <fmalita@chromium.org>
Commit-Queue: Mike Klein <mtklein@chromium.org>

[modify] https://crrev.com/249ee1f985b4bad78db2d5cfdd14ce38edb2c23a/src/jumper/SkJumper_generated.S
[modify] https://crrev.com/249ee1f985b4bad78db2d5cfdd14ce38edb2c23a/src/jumper/SkJumper_stages.cpp
[modify] https://crrev.com/249ee1f985b4bad78db2d5cfdd14ce38edb2c23a/tests/SkRasterPipelineTest.cpp
[modify] https://crrev.com/249ee1f985b4bad78db2d5cfdd14ce38edb2c23a/src/jumper/SkJumper_generated_win.S

Status: Fixed (was: Assigned)
I can no longer reproduce after patching that in locally.  It should roll into Chrome later today.
Project Member

Comment 14 by bugdroid1@chromium.org, Aug 1 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/47461c01b360cd971a176b306d0d939ca72da2d3

commit 47461c01b360cd971a176b306d0d939ca72da2d3
Author: skia-deps-roller@chromium.org <skia-deps-roller@chromium.org>
Date: Tue Aug 01 21:57:17 2017

Roll src/third_party/skia/ 7516c2775..249ee1f98 (10 commits)

https://skia.googlesource.com/skia.git/+log/7516c2775c30..249ee1f985b4

$ git log 7516c2775..249ee1f98 --date=short --no-merges --format='%ad %ae %s'
2017-08-01 mtklein clamp to 0 in repeat and mirror image tilers
2017-08-01 brianosman Guard against D3D NaN/Infinity literals bug
2017-08-01 ethannicholas support for 'half' types in sksl, plus general numeric type improvements
2017-08-01 bungeman Assert text passed to canvas is initialized.
2017-08-01 mtklein Add Perf-Win2k8-Clang bots.
2017-08-01 liyuqian Revert "Revert "Revert "Add support for semaphores to be inserted on GrContext flush"""
2017-08-01 bsalomon Allow RegionOp to be used for stenciling
2017-08-01 brianosman Remove unused code for index 8
2017-08-01 robertphillips Roll ANGLE
2017-08-01 egdaniel Revert "Revert "Add support for semaphores to be inserted on GrContext flush""

Created with:
  roll-dep src/third_party/skia
BUG= 749260 , 750070 , 750071 , 750072 


Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, see:
http://www.chromium.org/developers/tree-sheriffs/sheriff-details-chromium#TOC-Failures-due-to-DEPS-rolls


CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
TBR=liyuqian@chromium.org

Change-Id: I7026762d2d4d4c8423e909c82f6f0216f5aaebb1
Reviewed-on: https://chromium-review.googlesource.com/596829
Reviewed-by: Skia Deps Roller <skia-deps-roller@chromium.org>
Commit-Queue: Skia Deps Roller <skia-deps-roller@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491127}
[modify] https://crrev.com/47461c01b360cd971a176b306d0d939ca72da2d3/DEPS

Project Member

Comment 15 by ClusterFuzz, Aug 2 2017

ClusterFuzz has detected this issue as fixed in range 491089:491219.

Detailed report: https://clusterfuzz.com/testcase?key=6409534427103232

Fuzzer: inferno_twister_c
Job Type: linux_asan_chrome_media
Platform Id: linux

Crash Type: UNKNOWN READ
Crash Address: 0x7f9dbda98ff8
Crash State:
  _sk_gather_bgra_avx
  antifilldot8
  SkScan::AntiFillRect
  
Sanitizer: address (ASAN)

Recommended Security Severity: Medium

Regressed: https://clusterfuzz.com/revisions?job=linux_asan_chrome_media&range=488849:488981
Fixed: https://clusterfuzz.com/revisions?job=linux_asan_chrome_media&range=491089:491219

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=6409534427103232


See https://github.com/google/clusterfuzz-tools for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.
Project Member

Comment 16 by ClusterFuzz, Aug 2 2017

Labels: ClusterFuzz-Verified
Status: Verified (was: Fixed)
ClusterFuzz testcase 6409534427103232 is verified as fixed, so closing issue as verified.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.
Project Member

Comment 17 by sheriffbot@chromium.org, Aug 2 2017

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Labels: -ReleaseBlock-Stable
Project Member

Comment 19 by sheriffbot@chromium.org, Nov 8 2017

Labels: -Restrict-View-SecurityNotify allpublic
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Sign in to add a comment