Fix signed integer overflow in MakeKernelParams8bit - #373
Conversation
|
Hi, @reillyeon. Can you help take a look or find a suitable reviewer? Thanks. |
| int start_row, int start_col, int end_row, | ||
| int end_col, Mat<DstScalar>* dst, | ||
| KernelParams8bit<LhsCols, RhsCols>* params) { | ||
| RUY_NO_SANITIZE_INTEGER_OVERFLOW void MakeKernelParams8bit( |
There was a problem hiding this comment.
This seems wrong because this function is computing buffer offsets which should be checked for overflow.
There was a problem hiding this comment.
Thanks, you're right that a function-level overflow suppression is wrong here. The UBSan report actually pointed at prod_zp_depth = lhs.zero_point * rhs.zero_point * depth . That value is a numeric accumulator constant, not a buffer offset, and it intentionally wraps to match the two's-complement arithmetic of the asm kernels.
So instead of exempting the whole function, I dropped the annotation and now compute prod_zp_depth with uint32_t casts so the wrap is well-defined. All the offset/pointer arithmetic keeps its overflow checks.
wangw-1991
changed the title
reillyeon
left a comment
reillyeon
left a comment
There was a problem hiding this comment.
I'm still working on finding a reviewer with permission to merge this.
|
OK, thanks for the review. |
|
I am sending the PR to Google's engineers to review. |
3 participants
Compute
prod_zp_depthviauint32_tcasts so the intended wraparound is well-defined rather than UB. Overflow checks on the surrounding offset/pointer computations are left untouched.See chromium issue: https://issues.chromium.org/issues/526978330 and https://issues.chromium.org/issues/520870343