linux-kernel - Re: [PATCH v3 1/8] scpi: Add cmd indirection table to prepare for legacy commands

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 19 Sep 2016 17:03:50 +0200
From:   Neil Armstrong <narmstrong@...libre.com>
To:     Sudeep Holla <sudeep.holla@....com>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Cc:     linux-amlogic@...ts.infradead.org, khilman@...libre.com,
        heiko@...ech.de, wxt@...k-chips.com, frank.wang@...k-chips.com
Subject: Re: [PATCH v3 1/8] scpi: Add cmd indirection table to prepare for
 legacy commands

On 09/19/2016 04:41 PM, Sudeep Holla wrote:
> Hi Neil,
> 
> On 07/09/16 16:34, Neil Armstrong wrote:
>> Add indirection table to permit multiple command values for legacy support.
>>
> 
> I wrote the most of the patch and you changed the author too ;)

Sorry, forgot this ! v4 will have it !
> 
>> Signed-off-by: Neil Armstrong <narmstrong@...libre.com>
>> ---
>>  drivers/firmware/arm_scpi.c | 145 ++++++++++++++++++++++++++++++++++++++------
>>  1 file changed, 127 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
>> index 4388937..9a87687 100644
>> --- a/drivers/firmware/arm_scpi.c
>> +++ b/drivers/firmware/arm_scpi.c
> 
> [..]
> 
>> @@ -161,6 +194,7 @@ struct scpi_drvinfo {
>>      u32 protocol_version;
>>      u32 firmware_version;
>>      int num_chans;
>> +    int *scpi_cmds;
>>      atomic_t next_chan;
>>      struct scpi_ops *scpi_ops;
>>      struct scpi_chan *channels;
>> @@ -390,6 +424,19 @@ static u32 scpi_get_version(void)
>>      return scpi_info->protocol_version;
>>  }
>>
>> +static inline int check_cmd(unsigned int offset)
>> +{
>> +    if (offset >= CMD_MAX_COUNT ||
> 
> If we call scpi_send_message internally(as it's static) why is this
> check needed ?
> 
> 
>> +        !scpi_info ||
>> +        !scpi_info->scpi_cmds)
> 
> Will be even reach to this point if above is true ?
> 
>> +        return -EINVAL;
>> +
>> +    if (scpi_info->scpi_cmds[offset] < 0)
>> +        return -EOPNOTSUPP;
> 
> IMO just above couple of lines in the beginning of scpi_send_message
> will suffice. You can just add this to my original patch.

Will do.

> 
>>  static int
>>  scpi_clk_get_range(u16 clk_id, unsigned long *min, unsigned long *max)
>>  {
>> @@ -397,8 +444,13 @@ scpi_clk_get_range(u16 clk_id, unsigned long *min, unsigned long *max)
>>      struct clk_get_info clk;
>>      __le16 le_clk_id = cpu_to_le16(clk_id);
>>
>> -    ret = scpi_send_message(SCPI_CMD_GET_CLOCK_INFO, &le_clk_id,
>> -                sizeof(le_clk_id), &clk, sizeof(clk));
>> +    ret = check_cmd(CMD_GET_CLOCK_INFO);
>> +    if (ret)
>> +        return ret;
>> +
> 
> It's totally unnecessary to add check in each and every function calling
> scpi_send_message, why not add it to scpi_send_message instead.
> 

This was my first thought, I should have stayed at this !

Thanks,
Neil