linux-kernel - Re: [PATCH v4 1/2] drivers: qcom: add command DB driver

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:   Tue, 06 Mar 2018 14:08:06 -0800
From:   Stephen Boyd <sboyd@...nel.org>
To:     Lina Iyer <ilina@...eaurora.org>
Cc:     andy.gross@...aro.org, david.brown@...aro.org,
        linux-arm-msm@...r.kernel.org, linux-soc@...r.kernel.org,
        rnayak@...eaurora.org, bjorn.andersson@...aro.org,
        linux-kernel@...r.kernel.org,
        Mahesh Sivasubramanian <msivasub@...eaurora.org>
Subject: Re: [PATCH v4 1/2] drivers: qcom: add command DB driver

Quoting Lina Iyer (2018-03-06 08:21:40)
> On Mon, Mar 05 2018 at 11:42 -0700, Stephen Boyd wrote:
> >Quoting Lina Iyer (2018-02-26 09:58:01)
> 
> >> +}
> >> +EXPORT_SYMBOL(cmd_db_ready);
> >> +
> >> +static u64 cmd_db_get_u64_id(const char *id)
> >> +{
> >> +       u64 rsc_id = 0;
> >> +       u8 *ch = (u8 *)&rsc_id;
> >> +       int i;
> >> +
> >> +       for (i = 0; i < sizeof(rsc_id) && id[i]; i++)
> >> +               ch[i] = id[i];
> >
> >This could be a strncpy now? Didn't I already ask this? I'll have to
> >look back at the other emails it seems. Actually, it looks like a cast
> >of a string to an integer,
> Yes, you did ask. The cast helps do an integer comparison as opposed to
> string comparision on the database.
> 
> >which is then reversed to look up the same
> >string. Not sure what the use is for this.
> >
> Where do you see that?

Hmm no idea. I must have seen something wrong. Ignore this one.

> 
> >> +
> >> +       return rsc_id;
> >> +}
> >> +
> >> +static int cmd_db_get_header(u64 query, struct entry_header *eh,
> >> +                           struct rsc_hdr *rh)
> >> +{
> >> +       struct rsc_hdr *rsc_hdr;
> >> +       struct entry_header *ent;
> >> +       int ret, i, j;
> >> +
> >> +       ret = cmd_db_ready();
> >> +       if (ret)
> >> +               return ret;
> >> +
> >> +       if (!eh || !rh)
> >> +               return -EINVAL;
> >> +
> >> +       for (i = 0; i < MAX_SLV_ID; i++) {
> >> +               rsc_hdr = &cmd_db_header->header[i];
> >> +               if (!rsc_hdr->slv_id)
> >> +                       break;
> >> +
> >> +               ent = ENTRY_HEADER(rsc_hdr);
> >> +               for (j = 0; j < rsc_hdr->cnt; j++, ent++) {
> >> +                       if (ent->id == query)
> >> +                               break;
> >> +               }
> >> +
> >> +               if (j < rsc_hdr->cnt) {
> >> +                       memcpy(eh, ent, sizeof(*ent));
> >> +                       memcpy(rh, rsc_hdr, sizeof(*rh));
> >
> >I suppose it's OK to punt on the endian issues for now if it's too hard.
> >Eventually we'll want to handle it though and it shouldn't make the code
> >any worse when endianness is the same. Can it be done now?
> >
> Why is the change in endian needed? We can always add that in later if
> we decide to change the default endian. I would prefer that we punt it
> for now.

Ok!

> 
> >> +               ret = PTR_ERR(start_addr);
> >> +               goto done;
> >> +       }
> >> +
> >> +       cmd_db_header = start_addr;
> >> +       if (cmd_db_header->magic_num != CMD_DB_MAGIC) {
> >
> >memcmp?
> >
> Why? It could be a simple comparison.

Endian stuff again

> 
> >> +               ret = -EINVAL;
> >> +               dev_err(&pdev->dev, "Invalid Command DB Magic\n");
> >> +               goto done;
> >> +       }
> >> +
> >> +done:
> >> +       devm_memunmap(&pdev->dev, dict);
> >
> >I'm lost why we use devm_*() for this mapping.
> >
> Must have picked it up from an example. Curious, why not though?

devm is usually used for long lived things that need to be released on
driver removal. At least that's the way I view it. For short lived
things that are created and then freed in the same scope I usually avoid
devm.