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
Date: Mon, 4 Mar 2024 15:10:39 -0800 [thread overview]
Message-ID: <202403041507.BEF59739@keescook> (raw)
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
prev parent reply other threads:[~2024-03-04 23:10 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-04 21:19 [PATCH v2] greybus: Avoid fake flexible array for response data Kees Cook
2024-03-04 22:45 ` Alex Elder
2024-03-04 23:10 ` Kees Cook [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=202403041507.BEF59739@keescook \
--to=keescook@chromium.org \
--cc=elder@ieee.org \
--cc=elder@kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=greybus-dev@lists.linaro.org \
--cc=gustavo@embeddedor.com \
--cc=gustavoars@kernel.org \
--cc=johan@kernel.org \
--cc=linux-hardening@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-staging@lists.linux.dev \
--cc=vireshk@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).