Received-SPF: pass (google.com: domain of linux-kernel+bounces-91401-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Mon, 4 Mar 2024 15:10:39 -0800
From: Kees Cook <keescook@chromium.org>
To: Alex Elder <elder@ieee.org>
Cc: Alex Elder <elder@kernel.org>, Viresh Kumar <vireshk@kernel.org>,
	Johan Hovold <johan@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"Gustavo A . R . Silva" <gustavo@embeddedor.com>,
	greybus-dev@lists.linaro.org, linux-staging@lists.linux.dev,
	"Gustavo A. R. Silva" <gustavoars@kernel.org>,
	linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org
Subject: Re: [PATCH v2] greybus: Avoid fake flexible array for response data
Message-ID: <202403041507.BEF59739@keescook>
References: <20240304211940.it.083-kees@kernel.org>
 <1c5ab1e7-ac66-438c-bc49-0785810e9355@ieee.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1c5ab1e7-ac66-438c-bc49-0785810e9355@ieee.org>

On Mon, Mar 04, 2024 at 04:45:11PM -0600, Alex Elder wrote:
> On 3/4/24 3:19 PM, Kees Cook wrote:
> > FORTIFY_SOURCE has been ignoring 0-sized destinations while the kernel
> > code base has been converted to flexible arrays. In order to enforce
> > the 0-sized destinations (e.g. with __counted_by), the remaining 0-sized
> > destinations need to be handled. Instead of converting an empty struct
> > into using a flexible array, just directly use a pointer without any
> > additional indirection. Remove struct gb_bootrom_get_firmware_response
> > and struct gb_fw_download_fetch_firmware_response.
> > 
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> 
> Thanks for adding the comments!  This looks good to me.
> 
> Reviewed-by: Alex Elder <elder@linaro.org>
> 
> 
> 
> I want to call attention to a few other spots that should
> get a little more attention--related directly to what you're
> doing here.
> 
> I noticed that the GB_CONTROL_TYPE_GET_MANIFEST response
> structure also contains only a flexible array.  It might
> be good to add a similar comment in gb_interface_enable(),
> above this line:
>         manifest = kmalloc(size, GFP_KERNEL);
> The definition of the gb_control_get_manifest_response structure
> could probably be replaced with a comment.
> 
> 
> The response buffer for an I2C transfer consists only of incoming
> data.  There is already a comment in gb_i2c_operation_create()
> that says this:
>         /* Response consists only of incoming data */
> The definition of the gb_i2c_transfer_response structure should
> then go away, in favor of a comment saying this.
> 
> The response buffer for a SPI transfer consists only of incoming
> data.  It is used three times in "driver/staging/greybus/spilib.c":
> - calc_rx_xfer_size() subtracts the size of the response structure,
>   and that should be replaced by a comment (and the structure
>   definition should go away)
> - gb_spi_decode_response() takes the response structure as an
>   argument.  That could be replaced with a void pointer instead,
>   with a comment.
> - gb_spi_transfer_one_message() is what passes the response buffer
>   to gb_spi_decode_response(), and could be adjusted to reflect
>   that the response consists only of data--rather than a struct
>   containing only a flexible array.
> 
> 
> Kees:  I'm *not* asking you to deal with these, I'm just mentioning
> them to you.  My comments above (without someone else confirming)
> are not sufficient to dictate how to address these.

Okay, thanks! Yeah, I took a look at struct gb_i2c_transfer_response and
I think it might trip the memcpy checking too since it's zero sized, but
it's on the source side, which isn't as strictly checked.

I'll add a TODO item to track these, though.

-Kees

-- 
Kees Cook