All of lore.kernel.org
 help / color / mirror / Atom feed
From: Hans Verkuil <hverkuil+cisco@kernel.org>
To: Pengpeng Hou <pengpeng@iscas.ac.cn>, Hans Verkuil <hverkuil@kernel.org>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>,
	linux-media@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] media: cec: extron: validate response prefixes
Date: Wed, 9 Sep 2026 11:05:00 +0200	[thread overview]
Message-ID: <a589c1f2-e823-485f-9768-a6e471ffeaca@kernel.org> (raw)
In-Reply-To: <20260830130356.159-1-pengpeng@iscas.ac.cn>

Hi Pengpeng Hou,

On 30/08/2026 15:03, Pengpeng Hou wrote:
> extron_interrupt() dispatches completed serial lines by comparing fixed
> prefixes and reading fixed offsets without first checking the current line
> length. Short lines can therefore consume stale bytes beyond the
> NUL-terminated message item.
> 
> Require each response type to contain the complete dispatch prefix before
> reading its fixed fields.
> 
> Fixes: 056f2821b631 ("media: cec: extron-da-hd-4k-plus: add the Extron DA HD 4K Plus CEC driver")
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> ---
>  .../usb/extron-da-hd-4k-plus/extron-da-hd-4k-plus.c    | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/cec/usb/extron-da-hd-4k-plus/extron-da-hd-4k-plus.c b/drivers/media/cec/usb/extron-da-hd-4k-plus/extron-da-hd-4k-plus.c
> index 3c6ce6f3d93e7..66780b3cf9188 100644
> --- a/drivers/media/cec/usb/extron-da-hd-4k-plus/extron-da-hd-4k-plus.c
> +++ b/drivers/media/cec/usb/extron-da-hd-4k-plus/extron-da-hd-4k-plus.c
> @@ -851,13 +851,13 @@ static irqreturn_t extron_interrupt(struct serio *serio, unsigned char data,
>  		if (debug)
>  			dev_info(extron->dev, "received %s\n", extron->data);
>  		extron->idx = 0;
> -		if (!memcmp(extron->data, "Sig", 3) &&
> +		if (extron->len >= 5 && !memcmp(extron->data, "Sig", 3) &&

All these extra checks makes the code fragile (i.e. easy to make mistakes), esp. if new
prefix checks are added in the future.

How about just replace:

                extron->data[extron->len] = 0;

with:

                /*
                 * Prevent the following prefix tests from using stale data
                 * from beyond extron->len
                 */
                memset(extron->data + extron->len, 0,
                       min(10, sizeof(extron->data) - extron->len));

This ensures that bytes extron->len to extron->len + 9 are all set to 0 and you won't hit
stale data.

Regards,

	Hans

>  		    extron->data[4] == '*') {
>  			extron_process_signal_change(extron, extron->data + 3);
> -		} else if (!memcmp(extron->data, "Hdcp", 4) &&
> +		} else if (extron->len >= 6 && !memcmp(extron->data, "Hdcp", 4) &&
>  			   extron->data[5] == '*') {
>  			extron_process_edid_change(extron, extron->data + 4);
> -		} else if (!memcmp(extron->data, "DcecI", 5) &&
> +		} else if (extron->len >= 6 && !memcmp(extron->data, "DcecI", 5) &&
>  			   extron->data[5] >= '1' &&
>  			   extron->data[5] < '1' + extron->num_in_ports) {
>  			unsigned int p = extron->data[5] - '1';
> @@ -865,7 +865,7 @@ static irqreturn_t extron_interrupt(struct serio *serio, unsigned char data,
>  			p += extron->num_out_ports;
>  			extron_process_tx_done(extron->ports[p],
>  					       extron->data[extron->len - 1]);
> -		} else if (!memcmp(extron->data, "Ceci", 4) &&
> +		} else if (extron->len >= 6 && !memcmp(extron->data, "Ceci", 4) &&
>  			   extron->data[4] >= '1' &&
>  			   extron->data[4] < '1' + extron->num_in_ports &&
>  			   extron->data[5] == '*') {
> @@ -874,14 +874,14 @@ static irqreturn_t extron_interrupt(struct serio *serio, unsigned char data,
>  			p += extron->num_out_ports;
>  			extron_process_received(extron->ports[p],
>  						extron->data + 6);
> -		} else if (!memcmp(extron->data, "DcecO", 5) &&
> +		} else if (extron->len >= 6 && !memcmp(extron->data, "DcecO", 5) &&
>  			   extron->data[5] >= '1' &&
>  			   extron->data[5] < '1' + extron->num_out_ports) {
>  			unsigned int p = extron->data[5] - '1';
>  
>  			extron_process_tx_done(extron->ports[p],
>  					       extron->data[extron->len - 1]);
> -		} else if (!memcmp(extron->data, "Ceco", 4) &&
> +		} else if (extron->len >= 6 && !memcmp(extron->data, "Ceco", 4) &&
>  			   extron->data[4] >= '1' &&
>  			   extron->data[4] < '1' + extron->num_out_ports &&
>  			   extron->data[5] == '*') {
> @@ -889,7 +889,7 @@ static irqreturn_t extron_interrupt(struct serio *serio, unsigned char data,
>  
>  			extron_process_received(extron->ports[p],
>  						extron->data + 6);
> -		} else if (!memcmp(extron->data, "Pceco", 5) &&
> +		} else if (extron->len >= 7 && !memcmp(extron->data, "Pceco", 5) &&
>  			   extron->data[5] >= '1' &&
>  			   extron->data[5] < '1' + extron->num_out_ports) {
>  			unsigned int p = extron->data[5] - '1';
> @@ -899,7 +899,7 @@ static irqreturn_t extron_interrupt(struct serio *serio, unsigned char data,
>  				   &tmp_pa[0], &tmp_pa[1]) == 2)
>  				extron_phys_addr_change(extron->ports[p],
>  							tmp_pa[0] << 8 | tmp_pa[1]);
> -		} else if (!memcmp(extron->data, "Pceci", 5) &&
> +		} else if (extron->len >= 7 && !memcmp(extron->data, "Pceci", 5) &&
>  			   extron->data[5] >= '1' &&
>  			   extron->data[5] < '1' + extron->num_in_ports) {
>  			unsigned int p = extron->data[5] - '1';
> @@ -910,7 +910,7 @@ static irqreturn_t extron_interrupt(struct serio *serio, unsigned char data,
>  				   &tmp_pa[0], &tmp_pa[1]) == 2)
>  				extron_phys_addr_change(extron->ports[p],
>  							tmp_pa[0] << 8 | tmp_pa[1]);
> -		} else if (!memcmp(extron->data, "EdidR", 5) &&
> +		} else if (extron->len >= 7 && !memcmp(extron->data, "EdidR", 5) &&
>  			   extron->data[5] >= '1' &&
>  			   extron->data[5] < '1' + extron->num_ports &&
>  			   extron->data[6] == '*') {
> 
> base-commit: 08dbfad3f5040f5bdb6c529da20d6d4e81fefd72


      reply	other threads:[~2026-09-09  9:05 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-08-30 13:03 [PATCH] media: cec: extron: validate response prefixes Pengpeng Hou
2026-09-09  9:05 ` Hans Verkuil [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=a589c1f2-e823-485f-9768-a6e471ffeaca@kernel.org \
    --to=hverkuil+cisco@kernel.org \
    --cc=hverkuil@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=mchehab@kernel.org \
    --cc=pengpeng@iscas.ac.cn \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.