Re: [PATCH v3 1/5] btrfs: extent_io: Do extra check for extent buffer read write functions

[Josef Bacik <josef@toxicpanda.com>]

On 8/9/20 8:09 AM, Qu Wenruo wrote:
> Although we have start, len check for extent buffer reader/write (e.g.
> read_extent_buffer()), those checks has its limitations:
> - No overflow check
>    Values like start = 1024 len = -1024 can still pass the basic
>     (start + len) > eb->len check.
> 
> - Checks are not consistent
>    For read_extent_buffer() we only check (start + len) against eb->len.
>    While for memcmp_extent_buffer() we also check start against eb->len.
> 
> - Different error reporting mechanism
>    We use WARN() in read_extent_buffer() but BUG() in
>    memcpy_extent_buffer().
> 
> - Still modify memory if the request is obviously wrong
>    In read_extent_buffer() even we find (start + len) > eb->len, we still
>    call memset(dst, 0, len), which can eaisly cause memory access error
>    if start + len overflows.
> 
> To address above problems, this patch creates a new common function to
> check such access, check_eb_range().
> - Add overflow check
>    This function checks start, start + len against eb->len and overflow
>    check.
> 
> - Unified checks
> 
> - Unified error reports
>    Will call WARN() if CONFIG_BTRFS_DEBUG is configured.
>    And also do btrfs_warn() message for non-debug build.
> 
> - Exit ASAP if check fails
>    No more possible memory corruption.
> 
> - Add extra comment for @start @len used in those functions
>    Even experienced developers sometimes get confused with the @start
>    @len with logical address in those functions.
>    I'm not sure what's the cause, maybe it's the extent_buffer::start
>    naming.
>    For now, just add some comment.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=202817
> [ Inspired by above report, the report itself is already addressed ]
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef