linux-hardening - Re: [PATCH v2] greybus: Avoid fake flexible array for response data

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <202403041507.BEF59739@keescook>
Date: Mon, 4 Mar 2024 15:10:39 -0800
From: Kees Cook <keescook@...omium.org>
To: Alex Elder <elder@...e.org>
Cc: Alex Elder <elder@...nel.org>, Viresh Kumar <vireshk@...nel.org>,
	Johan Hovold <johan@...nel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"Gustavo A . R . Silva" <gustavo@...eddedor.com>,
	greybus-dev@...ts.linaro.org, linux-staging@...ts.linux.dev,
	"Gustavo A. R. Silva" <gustavoars@...nel.org>,
	linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH v2] greybus: Avoid fake flexible array for response data

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@...omium.org>
> 
> Thanks for adding the comments!  This looks good to me.
> 
> Reviewed-by: Alex Elder <elder@...aro.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