Re: [PATCH] attr: fix msan issue in read_attr_from_index

From: Jeff King <peff@peff.net>
Date: Jun 18, 2024, 23:39 UTC
Message-ID: <20240618233935.GB188880@coredump.intra.peff.net>
In-Reply-To: <pull.1747.git.1718654424683.gitgitgadget@gmail.com>

On Mon, Jun 17, 2024 at 08:00:24PM +0000, Kyle Lippincott via GitGitGadget wrote:

Show 12 quoted lines

> The issue exists because `size` is an output parameter from
> `read_blob_data_from_index`, but it's only modified if
> `read_blob_data_from_index` returns non-NULL. The read of `size` when
> calling `read_attr_from_buf` unconditionally may read from an
> uninitialized value. `read_attr_from_buf` checks that `buf` is non-NULL
> before reading from `size`, but by then it's already too late: the
> uninitialized read will have happened already. Furthermore, there's no
> guarantee that the compiler won't reorder things so that it checks
> `size` before checking `!buf`.
> 
> Make the call to `read_attr_from_buf` conditional on `buf` being
> non-NULL, ensuring that `size` is not read if it's never set.

Yeah, this is the same one I mentioned when bisecting in the other thread[1]. But I got confused by applying my fixup patch at various points in the bisection, and thought it _used_ to be a problem, and isn't anymore. It's the other way around. It was introduced by c793f9cb08, which moved the NULL check into the helper.

That patch is from Taylor, but I'm listed as a co-author, and I'm almost certain moving that NULL check was my suggestion. So it's doubly bad that I didn't figure out what was going on earlier. ;)

Possible UB aside, I doubt this can trigger bad behavior in practice. But I also wouldn't call it a false positive in MSan. We really are reading the uninitialized value and passing it. Your fix here is the obviously correct thing to do.

-Peff

[1] https://lore.kernel.org/git/20240608081855.GA2390433@coredump.intra.peff.net/