by Gustavo A. R. Silva

[permalink] [raw]

Subject: Re: [char-misc-next] Revert "mei: hdcp: Replace one-element array with flexible-array member"

On 7/28/20 18:16, Gustavo A. R. Silva wrote:
>
>
> On 7/28/20 18:04, Gustavo A. R. Silva wrote:
>> Tomas,
>>
>> Please, see some comments below...
>>
>> On 7/28/20 17:29, Winkler, Tomas wrote:
>>>>>>>
>>>>>>> Hi Tomas,
>>>>>>>
>>>>>>> On 7/28/20 16:41, Tomas Winkler wrote:
>>>>>>>> Greg please revert, this commit it changes size of struct
>>>>>>>> wired_cmd_repeater_auth_stream_req_in, this is not what firmware
>>>>>>>> is expecting.
>>>>>>>

Instead of reverting that commit, I think this patch should be applied
to fix it, if Tomas consider this is the appropiate solution:

diff --git a/drivers/misc/mei/hdcp/mei_hdcp.c b/drivers/misc/mei/hdcp/mei_hdcp.c
index d1d3e025ca0e..852374565cba 100644
--- a/drivers/misc/mei/hdcp/mei_hdcp.c
+++ b/drivers/misc/mei/hdcp/mei_hdcp.c
@@ -572,12 +572,12 @@ static int mei_hdcp_verify_mprime(struct device *dev,
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,
- array_size(data->k, sizeof(*data->streams)));
+ flex_array_size(&verify_mprime_in, streams, data->k));

verify_mprime_in.k = cpu_to_be16(data->k);

byte = mei_cldev_send(cldev, (u8 *)&verify_mprime_in,
- sizeof(verify_mprime_in));
+ struct_size(&verify_mprime_in, streams, data->k));
if (byte < 0) {
dev_dbg(dev, "mei_cldev_send failed. %zd\n", byte);
return byte;

I'll write up a proper patch after getting some feedback.

Thanks
--
Gustavo

>>>>>>> Could you elaborate on what's the firmware expecting, exactly?
>>>>>> struct wired_cmd_repeater_auth_stream_req_in {
>>>>>> - struct hdcp2_streamid_type streams[1];
>>>>>> + struct hdcp2_streamid_type streams[];
>>>>>> }
>>>>>>
>>>>>> But then you have, which you haven't changed to + 1 byte =
>>>>>> mei_cldev_send(cldev, (u8 *)&verify_mprime_in,
>>>>>> sizeof(verify_mprime_in));
>>>>>>
>>
>> I don't think the fix for this is to add 1 byte, if any, it seems
>> to be sizeof(*data->streams), or sizeof(struct hdcp2_streamid_type)
>> what needs to be added.
>>
>> But it might be better to code something like this, instead:
>>
>> diff --git a/drivers/misc/mei/hdcp/mei_hdcp.c b/drivers/misc/mei/hdcp/mei_hdcp.c
>> index e6c3dc595617..7fe63c915548 100644
>> --- a/drivers/misc/mei/hdcp/mei_hdcp.c
>> +++ b/drivers/misc/mei/hdcp/mei_hdcp.c
>> @@ -572,12 +572,12 @@ static int mei_hdcp_verify_mprime(struct device *dev,
>> 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,
>> - (data->k * sizeof(struct hdcp2_streamid_type)));
>> + (data->k * sizeof(*data->streams)));
>>
>> verify_mprime_in.k = cpu_to_be16(data->k);
>>
>> byte = mei_cldev_send(cldev, (u8 *)&verify_mprime_in,
>> - sizeof(verify_mprime_in));
>> + struct_size(&verify_mprime_in, streams, data->k));
>> if (byte < 0) {
>> dev_dbg(dev, "mei_cldev_send failed. %zd\n", byte);
>> return byte;
>>
>> struct_size(&verify_mprime_in, streams, data->k) will give us the size, in bytes,
>> of struct wired_cmd_repeater_auth_stream_req_in plus the size in bytes for
>> the streams[] flexible-array, which is determined by struct hdcp2_streamid_type and
>> data->k.
>>
>> What do you think?
>>
>
> This is even better:
>
> diff --git a/drivers/misc/mei/hdcp/mei_hdcp.c b/drivers/misc/mei/hdcp/mei_hdcp.c
> index e6c3dc595617..852374565cba 100644
> --- a/drivers/misc/mei/hdcp/mei_hdcp.c
> +++ b/drivers/misc/mei/hdcp/mei_hdcp.c
> @@ -572,12 +572,12 @@ static int mei_hdcp_verify_mprime(struct device *dev,
> 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,
> - (data->k * sizeof(struct hdcp2_streamid_type)));
> + flex_array_size(&verify_mprime_in, streams, data->k));
>
> verify_mprime_in.k = cpu_to_be16(data->k);
>
> byte = mei_cldev_send(cldev, (u8 *)&verify_mprime_in,
> - sizeof(verify_mprime_in));
> + struct_size(&verify_mprime_in, streams, data->k));
> if (byte < 0) {
> dev_dbg(dev, "mei_cldev_send failed. %zd\n", byte);
> return byte;
>
> the flex_array_size() is a new helper that was designed for this situations. :)
> It calculates the size of a flexible array member within an enclosing structure,
> which is exactly the case.
>
> Thanks
> --
> Gustavo
>
>> See more comments below...
>>
>>>>>
>>>>> I see, this is the kind of feedback I need from people that knows the
>>>>> code better. Thanks!
>>>>>
>>>>>> But that's not the major point. Point is that we should be able to
>>>>>> review and test the code before it is merged. You haven't run it, right?
>>>>>> There is MAINTAINERS file for a reason.
>>>>>>
>>>>>
>>>>> I'm using this command:
>>>>>
>>>>> $ scripts/get_maintainer.pl --nokeywords --nogit --nogit-fallback
>>>>>
>>>>> and this is the output for both files:
>>>>>
>>>>> $ scripts/get_maintainer.pl --nokeywords --nogit --nogit-fallback
>>>>> drivers/misc/mei/hdcp/mei_hdcp.c Arnd Bergmann <[email protected]>
>>>>> (supporter:CHAR and MISC DRIVERS) Greg Kroah-Hartman
>>>>> <[email protected]> (supporter:CHAR and MISC DRIVERS) linux-
>>>>> [email protected] (open list) $ scripts/get_maintainer.pl
>>>>> --nokeywords -- nogit --nogit-fallback
>>>>> drivers/misc/mei/hdcp/mei_hdcp.h Arnd Bergmann <[email protected]>
>>>>> (supporter:CHAR and MISC DRIVERS) Greg Kroah-Hartman
>>>>> <[email protected]> (supporter:CHAR and MISC DRIVERS) linux-
>>>>> [email protected] (open list)
>>>>
>>>>
>>>>> For some reason you don't appear on the list above. Do you see
>>>>> anything wrong with the command I'm running to get the maintainers and
>>>> lists?
>>>>
>>>> Not sure, it should be caught by drivers/misc/mei/* ?
>>>> Maybe it is not recursive? Need to check the script, frankly I usually check this
>>>> manually.
>>>>
>>>> INTEL MANAGEMENT ENGINE (mei)
>>>> M: Tomas Winkler <[email protected]>
>>>> L: [email protected]
>>>> S: Supported
>>>> F: Documentation/driver-api/mei/*
>>>> F: drivers/misc/mei/*
>>>> F: drivers/watchdog/mei_wdt.c
>>>> F: include/linux/mei_cl_bus.h
>>>> F: include/uapi/linux/mei.h
>>>> F: samples/mei/*
>>>>
>>>
>>> It's not recursive, when I add drivers/misc/mei/hdcp/* . It works correctly
>>> I will post a patch.
>>>
>>
>> Great. I'm glad we got to the root cause of this issue. :)
>>
>> Thanks
>> --
>> Gustavo
>>
>>> Thanks
>>> Tomas
>>>
>>>
>>>>
>>>>>
>>>>> Thanks
>>>>> --
>>>>> Gustavo
>>>>>
>>>>>>>
>>>>>>>> I really do not appreciate that the code is bypassing driver
>>>>>>>> maintaner review, I think this is a minimum we can ask for, this
>>>>>>>> is not for a first time.
>>>>>>>>
>>>>>>>> This reverts commit c56967d674e361ebe716e66992e3c5332b25ac1f.
>>>>>>>>
>>>>>>>> Cc: Gustavo A. R. Silva <[email protected]>
>>>>>>>> Signed-off-by: Tomas Winkler <[email protected]>
>>>>>>>> ---
>>>>>>>> drivers/misc/mei/hdcp/mei_hdcp.c | 2 +-
>>>>>>>> drivers/misc/mei/hdcp/mei_hdcp.h | 2 +-
>>>>>>>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/misc/mei/hdcp/mei_hdcp.c
>>>>>>>> b/drivers/misc/mei/hdcp/mei_hdcp.c
>>>>>>>> index d1d3e025ca0e..e6c3dc595617 100644
>>>>>>>> --- a/drivers/misc/mei/hdcp/mei_hdcp.c
>>>>>>>> +++ b/drivers/misc/mei/hdcp/mei_hdcp.c
>>>>>>>> @@ -572,7 +572,7 @@ static int mei_hdcp_verify_mprime(struct
>>>>>>>> device
>>>>>>> *dev,
>>>>>>>> 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,
>>>>>>>> - array_size(data->k, sizeof(*data->streams)));
>>>>>>>> + (data->k * sizeof(struct hdcp2_streamid_type)));
>>>>>>>>
>>>>>>>> verify_mprime_in.k = cpu_to_be16(data->k);
>>>>>>>>
>>>>>>>> diff --git a/drivers/misc/mei/hdcp/mei_hdcp.h
>>>>>>>> b/drivers/misc/mei/hdcp/mei_hdcp.h
>>>>>>>> index 834757f5e072..18ffc773fa18 100644
>>>>>>>> --- a/drivers/misc/mei/hdcp/mei_hdcp.h
>>>>>>>> +++ b/drivers/misc/mei/hdcp/mei_hdcp.h
>>>>>>>> @@ -358,7 +358,7 @@ struct
>>>> wired_cmd_repeater_auth_stream_req_in
>>>>> {
>>>>>>>> u8
>>>>>>> seq_num_m[HDCP_2_2_SEQ_NUM_LEN];
>>>>>>>> u8 m_prime[HDCP_2_2_MPRIME_LEN];
>>>>>>>> __be16 k;
>>>>>>>> - struct hdcp2_streamid_type streams[];
>>>>>>>> + struct hdcp2_streamid_type streams[1];
>>>>>>>> } __packed;
>>>>>>>>
>>>>>>>> struct wired_cmd_repeater_auth_stream_req_out {
>>>>>>>>