[char-misc-next V2] mei: hdcp: fix mei_hdcp_verify_mprime() input paramter

[char-misc-next V2] mei: hdcp: fix mei_hdcp_verify_mprime() input paramter

[Tomas Winkler]

wired_cmd_repeater_auth_stream_req_in has a variable
length array at the end. we use struct_size() overflow
macro to determine the size for the allocation and sending
size.

Fixes: c56967d674e3 (mei: hdcp: Replace one-element array with flexible-array member)
Fixes: c56967d674e3 (mei: hdcp: Replace one-element array with flexible-array member)
Cc: Ramalingam C <ramalingam.c@intel.com>
Cc: Gustavo A. R. Silva <gustavoars@kernel.org>
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
V2: Check for allocation failure.
 drivers/misc/mei/hdcp/mei_hdcp.c | 40 +++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 16 deletions(-)

diff --git a/drivers/misc/mei/hdcp/mei_hdcp.c b/drivers/misc/mei/hdcp/mei_hdcp.c
index d1d3e025ca0e..f1205e0060db 100644
--- a/drivers/misc/mei/hdcp/mei_hdcp.c
+++ b/drivers/misc/mei/hdcp/mei_hdcp.c
@@ -546,38 +546,46 @@ static int mei_hdcp_verify_mprime(struct device *dev,
 				  struct hdcp_port_data *data,
 				  struct hdcp2_rep_stream_ready *stream_ready)
 {
-	struct wired_cmd_repeater_auth_stream_req_in
-					verify_mprime_in = { { 0 } };
+	struct wired_cmd_repeater_auth_stream_req_in *verify_mprime_in;
 	struct wired_cmd_repeater_auth_stream_req_out
 					verify_mprime_out = { { 0 } };
 	struct mei_cl_device *cldev;
 	ssize_t byte;
+	size_t cmd_size;
 
 	if (!dev || !stream_ready || !data)
 		return -EINVAL;
 
 	cldev = to_mei_cl_device(dev);
 
-	verify_mprime_in.header.api_version = HDCP_API_VERSION;
-	verify_mprime_in.header.command_id = WIRED_REPEATER_AUTH_STREAM_REQ;
-	verify_mprime_in.header.status = ME_HDCP_STATUS_SUCCESS;
-	verify_mprime_in.header.buffer_len =
+	cmd_size = struct_size(verify_mprime_in, streams, data->k);
+	if (cmd_size == SIZE_MAX)
+		return -EINVAL;
+
+	verify_mprime_in = kzalloc(cmd_size, GFP_KERNEL);
+	if (!verify_mprime_in)
+		return -ENOMEM;
+
+	verify_mprime_in->header.api_version = HDCP_API_VERSION;
+	verify_mprime_in->header.command_id = WIRED_REPEATER_AUTH_STREAM_REQ;
+	verify_mprime_in->header.status = ME_HDCP_STATUS_SUCCESS;
+	verify_mprime_in->header.buffer_len =
 			WIRED_CMD_BUF_LEN_REPEATER_AUTH_STREAM_REQ_MIN_IN;
 
-	verify_mprime_in.port.integrated_port_type = data->port_type;
-	verify_mprime_in.port.physical_port = (u8)data->fw_ddi;
-	verify_mprime_in.port.attached_transcoder = (u8)data->fw_tc;
+	verify_mprime_in->port.integrated_port_type = data->port_type;
+	verify_mprime_in->port.physical_port = (u8)data->fw_ddi;
+	verify_mprime_in->port.attached_transcoder = (u8)data->fw_tc;
+
+	memcpy(verify_mprime_in->m_prime, stream_ready->m_prime, HDCP_2_2_MPRIME_LEN);
+	drm_hdcp_cpu_to_be24(verify_mprime_in->seq_num_m, data->seq_num_m);
 
-	memcpy(verify_mprime_in.m_prime, stream_ready->m_prime,
-	       HDCP_2_2_MPRIME_LEN);
-	drm_hdcp_cpu_to_be24(verify_mprime_in.seq_num_m, data->seq_num_m);
-	memcpy(verify_mprime_in.streams, data->streams,
+	memcpy(verify_mprime_in->streams, data->streams,
 	       array_size(data->k, sizeof(*data->streams)));
 
-	verify_mprime_in.k = cpu_to_be16(data->k);
+	verify_mprime_in->k = cpu_to_be16(data->k);
 
-	byte = mei_cldev_send(cldev, (u8 *)&verify_mprime_in,
-			      sizeof(verify_mprime_in));
+	byte = mei_cldev_send(cldev, (u8 *)&verify_mprime_in, cmd_size);
+	kfree(verify_mprime_in);
 	if (byte < 0) {
 		dev_dbg(dev, "mei_cldev_send failed. %zd\n", byte);
 		return byte;
-- 
2.25.4

Re: [char-misc-next V2] mei: hdcp: fix mei_hdcp_verify_mprime() input paramter

[Gustavo A. R. Silva]

Hi Tomas,

On 7/30/20 02:02, Tomas Winkler wrote:
> wired_cmd_repeater_auth_stream_req_in has a variable
> length array at the end. we use struct_size() overflow
> macro to determine the size for the allocation and sending
> size.
> 

My comments here:
https://lore.kernel.org/lkml/66c9950c-ef54-423e-467f-38a9f7afb384@embeddedor.com/

still stand...

> Fixes: c56967d674e3 (mei: hdcp: Replace one-element array with flexible-array member)
> Fixes: c56967d674e3 (mei: hdcp: Replace one-element array with flexible-array member)

You have repeated the same commit twice, above. Also, a Fixes tag for the commit from
Feb 2019 that I talk about here:
https://lore.kernel.org/lkml/66c9950c-ef54-423e-467f-38a9f7afb384@embeddedor.com/

should be included in this changelog text.

--
Gustavo

> Cc: Ramalingam C <ramalingam.c@intel.com>
> Cc: Gustavo A. R. Silva <gustavoars@kernel.org>
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> ---
> V2: Check for allocation failure.
>  drivers/misc/mei/hdcp/mei_hdcp.c | 40 +++++++++++++++++++-------------
>  1 file changed, 24 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/misc/mei/hdcp/mei_hdcp.c b/drivers/misc/mei/hdcp/mei_hdcp.c
> index d1d3e025ca0e..f1205e0060db 100644
> --- a/drivers/misc/mei/hdcp/mei_hdcp.c
> +++ b/drivers/misc/mei/hdcp/mei_hdcp.c
> @@ -546,38 +546,46 @@ static int mei_hdcp_verify_mprime(struct device *dev,
>  				  struct hdcp_port_data *data,
>  				  struct hdcp2_rep_stream_ready *stream_ready)
>  {
> -	struct wired_cmd_repeater_auth_stream_req_in
> -					verify_mprime_in = { { 0 } };
> +	struct wired_cmd_repeater_auth_stream_req_in *verify_mprime_in;
>  	struct wired_cmd_repeater_auth_stream_req_out
>  					verify_mprime_out = { { 0 } };
>  	struct mei_cl_device *cldev;
>  	ssize_t byte;
> +	size_t cmd_size;
>  
>  	if (!dev || !stream_ready || !data)
>  		return -EINVAL;
>  
>  	cldev = to_mei_cl_device(dev);
>  
> -	verify_mprime_in.header.api_version = HDCP_API_VERSION;
> -	verify_mprime_in.header.command_id = WIRED_REPEATER_AUTH_STREAM_REQ;
> -	verify_mprime_in.header.status = ME_HDCP_STATUS_SUCCESS;
> -	verify_mprime_in.header.buffer_len =
> +	cmd_size = struct_size(verify_mprime_in, streams, data->k);
> +	if (cmd_size == SIZE_MAX)
> +		return -EINVAL;
> +
> +	verify_mprime_in = kzalloc(cmd_size, GFP_KERNEL);
> +	if (!verify_mprime_in)
> +		return -ENOMEM;
> +
> +	verify_mprime_in->header.api_version = HDCP_API_VERSION;
> +	verify_mprime_in->header.command_id = WIRED_REPEATER_AUTH_STREAM_REQ;
> +	verify_mprime_in->header.status = ME_HDCP_STATUS_SUCCESS;
> +	verify_mprime_in->header.buffer_len =
>  			WIRED_CMD_BUF_LEN_REPEATER_AUTH_STREAM_REQ_MIN_IN;
>  
> -	verify_mprime_in.port.integrated_port_type = data->port_type;
> -	verify_mprime_in.port.physical_port = (u8)data->fw_ddi;
> -	verify_mprime_in.port.attached_transcoder = (u8)data->fw_tc;
> +	verify_mprime_in->port.integrated_port_type = data->port_type;
> +	verify_mprime_in->port.physical_port = (u8)data->fw_ddi;
> +	verify_mprime_in->port.attached_transcoder = (u8)data->fw_tc;
> +
> +	memcpy(verify_mprime_in->m_prime, stream_ready->m_prime, HDCP_2_2_MPRIME_LEN);
> +	drm_hdcp_cpu_to_be24(verify_mprime_in->seq_num_m, data->seq_num_m);
>  
> -	memcpy(verify_mprime_in.m_prime, stream_ready->m_prime,
> -	       HDCP_2_2_MPRIME_LEN);
> -	drm_hdcp_cpu_to_be24(verify_mprime_in.seq_num_m, data->seq_num_m);
> -	memcpy(verify_mprime_in.streams, data->streams,
> +	memcpy(verify_mprime_in->streams, data->streams,
>  	       array_size(data->k, sizeof(*data->streams)));
>  
> -	verify_mprime_in.k = cpu_to_be16(data->k);
> +	verify_mprime_in->k = cpu_to_be16(data->k);
>  
> -	byte = mei_cldev_send(cldev, (u8 *)&verify_mprime_in,
> -			      sizeof(verify_mprime_in));
> +	byte = mei_cldev_send(cldev, (u8 *)&verify_mprime_in, cmd_size);
> +	kfree(verify_mprime_in);
>  	if (byte < 0) {
>  		dev_dbg(dev, "mei_cldev_send failed. %zd\n", byte);
>  		return byte;
> 

RE: [char-misc-next V2] mei: hdcp: fix mei_hdcp_verify_mprime() input paramter

[Winkler, Tomas]

> Hi Tomas,
> 
> On 7/30/20 02:02, Tomas Winkler wrote:
> > wired_cmd_repeater_auth_stream_req_in has a variable length array at
> > the end. we use struct_size() overflow macro to determine the size for
> > the allocation and sending size.
> >
> 
> My comments here:
> https://lore.kernel.org/lkml/66c9950c-ef54-423e-467f-
> 38a9f7afb384@embeddedor.com/
Strange,  it haven't landed in my mailbox 


> still stand...


1. I don't think it should go to stable, true  there was bug but it wasn't triggered because there is only one stream.
2. In any case if we want to backport to stable, than all those fancy overflow macros are not there yet, need to rewrite the patch.
2. We cannot flex_array_size() as the streams in hdcp_port_data is not an array, I'm not going to change it in this patch. 

> 
> > Fixes: c56967d674e3 (mei: hdcp: Replace one-element array with
> > flexible-array member)
> > Fixes: c56967d674e3 (mei: hdcp: Replace one-element array with
> > flexible-array member)

> You have repeated the same commit twice, above.
Will fix 
 Also, a Fixes tag for the

> commit from Feb 2019 that I talk about here:
> https://lore.kernel.org/lkml/66c9950c-ef54-423e-467f-
> 38a9f7afb384@embeddedor.com/
Will add 
> should be included in this changelog text.
> 
> --
> Gustavo
> 
> > Cc: Ramalingam C <ramalingam.c@intel.com>
> > Cc: Gustavo A. R. Silva <gustavoars@kernel.org>
> > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > ---
> > V2: Check for allocation failure.
> >  drivers/misc/mei/hdcp/mei_hdcp.c | 40
> > +++++++++++++++++++-------------
> >  1 file changed, 24 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/misc/mei/hdcp/mei_hdcp.c
> > b/drivers/misc/mei/hdcp/mei_hdcp.c
> > index d1d3e025ca0e..f1205e0060db 100644
> > --- a/drivers/misc/mei/hdcp/mei_hdcp.c
> > +++ b/drivers/misc/mei/hdcp/mei_hdcp.c
> > @@ -546,38 +546,46 @@ static int mei_hdcp_verify_mprime(struct device
> *dev,
> >  				  struct hdcp_port_data *data,
> >  				  struct hdcp2_rep_stream_ready
> *stream_ready)  {
> > -	struct wired_cmd_repeater_auth_stream_req_in
> > -					verify_mprime_in = { { 0 } };
> > +	struct wired_cmd_repeater_auth_stream_req_in *verify_mprime_in;
> >  	struct wired_cmd_repeater_auth_stream_req_out
> >  					verify_mprime_out = { { 0 } };
> >  	struct mei_cl_device *cldev;
> >  	ssize_t byte;
> > +	size_t cmd_size;
> >
> >  	if (!dev || !stream_ready || !data)
> >  		return -EINVAL;
> >
> >  	cldev = to_mei_cl_device(dev);
> >
> > -	verify_mprime_in.header.api_version = HDCP_API_VERSION;
> > -	verify_mprime_in.header.command_id =
> WIRED_REPEATER_AUTH_STREAM_REQ;
> > -	verify_mprime_in.header.status = ME_HDCP_STATUS_SUCCESS;
> > -	verify_mprime_in.header.buffer_len =
> > +	cmd_size = struct_size(verify_mprime_in, streams, data->k);
> > +	if (cmd_size == SIZE_MAX)
> > +		return -EINVAL;
> > +
> > +	verify_mprime_in = kzalloc(cmd_size, GFP_KERNEL);
> > +	if (!verify_mprime_in)
> > +		return -ENOMEM;
> > +
> > +	verify_mprime_in->header.api_version = HDCP_API_VERSION;
> > +	verify_mprime_in->header.command_id =
> WIRED_REPEATER_AUTH_STREAM_REQ;
> > +	verify_mprime_in->header.status = ME_HDCP_STATUS_SUCCESS;
> > +	verify_mprime_in->header.buffer_len =
> >
> 	WIRED_CMD_BUF_LEN_REPEATER_AUTH_STREAM_REQ_MIN_IN;
> >
> > -	verify_mprime_in.port.integrated_port_type = data->port_type;
> > -	verify_mprime_in.port.physical_port = (u8)data->fw_ddi;
> > -	verify_mprime_in.port.attached_transcoder = (u8)data->fw_tc;
> > +	verify_mprime_in->port.integrated_port_type = data->port_type;
> > +	verify_mprime_in->port.physical_port = (u8)data->fw_ddi;
> > +	verify_mprime_in->port.attached_transcoder = (u8)data->fw_tc;
> > +
> > +	memcpy(verify_mprime_in->m_prime, stream_ready->m_prime,
> HDCP_2_2_MPRIME_LEN);
> > +	drm_hdcp_cpu_to_be24(verify_mprime_in->seq_num_m, data-
> >seq_num_m);
> >
> > -	memcpy(verify_mprime_in.m_prime, stream_ready->m_prime,
> > -	       HDCP_2_2_MPRIME_LEN);
> > -	drm_hdcp_cpu_to_be24(verify_mprime_in.seq_num_m, data-
> >seq_num_m);
> > -	memcpy(verify_mprime_in.streams, data->streams,
> > +	memcpy(verify_mprime_in->streams, data->streams,
> >  	       array_size(data->k, sizeof(*data->streams)));
> >
> > -	verify_mprime_in.k = cpu_to_be16(data->k);
> > +	verify_mprime_in->k = cpu_to_be16(data->k);
> >
> > -	byte = mei_cldev_send(cldev, (u8 *)&verify_mprime_in,
> > -			      sizeof(verify_mprime_in));
> > +	byte = mei_cldev_send(cldev, (u8 *)&verify_mprime_in, cmd_size);
> > +	kfree(verify_mprime_in);
> >  	if (byte < 0) {
> >  		dev_dbg(dev, "mei_cldev_send failed. %zd\n", byte);
> >  		return byte;
> >

RE: [char-misc-next V2] mei: hdcp: fix mei_hdcp_verify_mprime() input paramter

[Winkler, Tomas]

> > Hi Tomas,
> >
> > On 7/30/20 02:02, Tomas Winkler wrote:
> > > wired_cmd_repeater_auth_stream_req_in has a variable length array at
> > > the end. we use struct_size() overflow macro to determine the size
> > > for the allocation and sending size.
> > >
> >
> > My comments here:
> > https://lore.kernel.org/lkml/66c9950c-ef54-423e-467f-
> > 38a9f7afb384@embeddedor.com/
> Strange,  it haven't landed in my mailbox
> 
> 
> > still stand...
> 
> 
> 1. I don't think it should go to stable, true  there was bug but it wasn't
> triggered because there is only one stream.


I'm changing my mind on that, this is a pretty security issue, it should be fixed in stable. 

> 2. In any case if we want to backport to stable, than all those fancy overflow
> macros are not there yet, need to rewrite the patch.
> 2. We cannot flex_array_size() as the streams in hdcp_port_data is not an
> array, I'm not going to change it in this patch.

Last, forgot to thanks you for your persistent and thoroughly review. 
 
> >
> > > Fixes: c56967d674e3 (mei: hdcp: Replace one-element array with
> > > flexible-array member)
> > > Fixes: c56967d674e3 (mei: hdcp: Replace one-element array with
> > > flexible-array member)
> 
> > You have repeated the same commit twice, above.
> Will fix
>  Also, a Fixes tag for the
> 
> > commit from Feb 2019 that I talk about here:
> > https://lore.kernel.org/lkml/66c9950c-ef54-423e-467f-
> > 38a9f7afb384@embeddedor.com/
> Will add
> > should be included in this changelog text.
> >
> > --
> > Gustavo
> >
> > > Cc: Ramalingam C <ramalingam.c@intel.com>
> > > Cc: Gustavo A. R. Silva <gustavoars@kernel.org>
> > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > > ---
> > > V2: Check for allocation failure.
> > >  drivers/misc/mei/hdcp/mei_hdcp.c | 40
> > > +++++++++++++++++++-------------
> > >  1 file changed, 24 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/drivers/misc/mei/hdcp/mei_hdcp.c
> > > b/drivers/misc/mei/hdcp/mei_hdcp.c
> > > index d1d3e025ca0e..f1205e0060db 100644
> > > --- a/drivers/misc/mei/hdcp/mei_hdcp.c
> > > +++ b/drivers/misc/mei/hdcp/mei_hdcp.c
> > > @@ -546,38 +546,46 @@ static int mei_hdcp_verify_mprime(struct
> > > device
> > *dev,
> > >  				  struct hdcp_port_data *data,
> > >  				  struct hdcp2_rep_stream_ready
> > *stream_ready)  {
> > > -	struct wired_cmd_repeater_auth_stream_req_in
> > > -					verify_mprime_in = { { 0 } };
> > > +	struct wired_cmd_repeater_auth_stream_req_in *verify_mprime_in;
> > >  	struct wired_cmd_repeater_auth_stream_req_out
> > >  					verify_mprime_out = { { 0 } };
> > >  	struct mei_cl_device *cldev;
> > >  	ssize_t byte;
> > > +	size_t cmd_size;
> > >
> > >  	if (!dev || !stream_ready || !data)
> > >  		return -EINVAL;
> > >
> > >  	cldev = to_mei_cl_device(dev);
> > >
> > > -	verify_mprime_in.header.api_version = HDCP_API_VERSION;
> > > -	verify_mprime_in.header.command_id =
> > WIRED_REPEATER_AUTH_STREAM_REQ;
> > > -	verify_mprime_in.header.status = ME_HDCP_STATUS_SUCCESS;
> > > -	verify_mprime_in.header.buffer_len =
> > > +	cmd_size = struct_size(verify_mprime_in, streams, data->k);
> > > +	if (cmd_size == SIZE_MAX)
> > > +		return -EINVAL;
> > > +
> > > +	verify_mprime_in = kzalloc(cmd_size, GFP_KERNEL);
> > > +	if (!verify_mprime_in)
> > > +		return -ENOMEM;
> > > +
> > > +	verify_mprime_in->header.api_version = HDCP_API_VERSION;
> > > +	verify_mprime_in->header.command_id =
> > WIRED_REPEATER_AUTH_STREAM_REQ;
> > > +	verify_mprime_in->header.status = ME_HDCP_STATUS_SUCCESS;
> > > +	verify_mprime_in->header.buffer_len =
> > >
> > 	WIRED_CMD_BUF_LEN_REPEATER_AUTH_STREAM_REQ_MIN_IN;
> > >
> > > -	verify_mprime_in.port.integrated_port_type = data->port_type;
> > > -	verify_mprime_in.port.physical_port = (u8)data->fw_ddi;
> > > -	verify_mprime_in.port.attached_transcoder = (u8)data->fw_tc;
> > > +	verify_mprime_in->port.integrated_port_type = data->port_type;
> > > +	verify_mprime_in->port.physical_port = (u8)data->fw_ddi;
> > > +	verify_mprime_in->port.attached_transcoder = (u8)data->fw_tc;
> > > +
> > > +	memcpy(verify_mprime_in->m_prime, stream_ready->m_prime,
> > HDCP_2_2_MPRIME_LEN);
> > > +	drm_hdcp_cpu_to_be24(verify_mprime_in->seq_num_m, data-
> > >seq_num_m);
> > >
> > > -	memcpy(verify_mprime_in.m_prime, stream_ready->m_prime,
> > > -	       HDCP_2_2_MPRIME_LEN);
> > > -	drm_hdcp_cpu_to_be24(verify_mprime_in.seq_num_m, data-
> > >seq_num_m);
> > > -	memcpy(verify_mprime_in.streams, data->streams,
> > > +	memcpy(verify_mprime_in->streams, data->streams,
> > >  	       array_size(data->k, sizeof(*data->streams)));
> > >
> > > -	verify_mprime_in.k = cpu_to_be16(data->k);
> > > +	verify_mprime_in->k = cpu_to_be16(data->k);
> > >
> > > -	byte = mei_cldev_send(cldev, (u8 *)&verify_mprime_in,
> > > -			      sizeof(verify_mprime_in));
> > > +	byte = mei_cldev_send(cldev, (u8 *)&verify_mprime_in, cmd_size);
> > > +	kfree(verify_mprime_in);
> > >  	if (byte < 0) {
> > >  		dev_dbg(dev, "mei_cldev_send failed. %zd\n", byte);
> > >  		return byte;
> > >