Re: [PATCH] greybus: reject messages with claimed size smaller than header

[Alex Elder <elder@ieee.org>]

On 11/8/26 7:33 PM, Zhao Feng wrote:
> gb_connection_recv() performs two sanity checks on received
> messages: the receive buffer must hold at least a full message
> header, and it must hold at least the message size claimed in the
> header.  Neither check verifies that the claimed message size
> covers the header itself.
> 
> A message claiming a size smaller than sizeof(struct
> gb_operation_msg_hdr) (e.g. 7) passes both checks when 8 or more
> bytes were received, and the claimed size is then used as the
> message size:
> 
> - gb_operation_create_incoming() computes
>    request_size = msg_size - sizeof(header), which underflows to
>    SIZE_MAX.  The request handler then reads payload from a message
>    whose payload size is SIZE_MAX; syzbot reported this as a KASAN
>    global-buffer-overflow read in gb_svc_request_handler().
> - gb_connection_recv_response() similarly computes
>    payload_size = size - sizeof(*header) for accepted short
>    responses, which underflows the same way.
> 
> Reject such messages up front in gb_connection_recv().  A claimed
> size equal to the header size is valid (header only, no payload),
> so the comparison must be strict.

So the problem occurs when a message arrives that has a bogus
header.  We verify that the bytes received are enough to hold a
gb_operation_msg_hdr structure.  But the 2-byte size field within
that structure lies at the beginning, and there was previously
no check that its value made sense.

A valid header->size must be at least sizeof(header) (for a
zero-byte payload).  It looks like gb_connection_recv_response()
might handle a header whose size field is too small, but
gb_connection_recv_request() does not.

In any case, I think this fix looks good.

Reviewed-by: Alex Elder <elder@riscstar.com>

> 
> Reported-by: syzbot+693652ded53d3eb9cb91@syzkaller.appspotmail.com
> Closes: https://syzbot.org/bug?extid=693652ded53d3eb9cb91
> Cc: stable@vger.kernel.org
> Signed-off-by: Zhao Feng <zhaofeng2@cmss.chinamobile.com>
> ---
> No Fixes tag: the missing validation dates back to the original
> greybus import; the introducing commit could not be verified with
> the trees available to me.
> 
> Not runtime-tested (no greybus hardware).  The affected function is
> identical in current upstream master, so the patch applies there
> as-is.
> 
> This also answers the question raised on the syzbot thread about
> whether the underflow is reachable: it is, because the existing
> checks compare the *received* size against the claimed size, never
> the claimed size against the header size.
> 
>   drivers/greybus/operation.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/greybus/operation.c b/drivers/greybus/operation.c
> index 8459e9bc07..83b16c78f7 100644
> --- a/drivers/greybus/operation.c
> +++ b/drivers/greybus/operation.c
> @@ -1046,6 +1046,12 @@ void gb_connection_recv(struct gb_connection *connection,
>   	/* Use memcpy as data may be unaligned */
>   	memcpy(&header, data, sizeof(header));
>   	msg_size = le16_to_cpu(header.size);
> +	if (msg_size < sizeof(header)) {
> +		dev_err_ratelimited(dev,
> +				    "%s: malformed message received (%zu < %zu)\n",
> +				    connection->name, msg_size, sizeof(header));
> +		return;
> +	}
>   	if (size < msg_size) {
>   		dev_err_ratelimited(dev,
>   				    "%s: incomplete message 0x%04x of type 0x%02x received (%zu < %zu)\n",