Re: [alsa-devel] [PATCH v7 08/24] ASoC: qdsp6: q6core: Add q6core driver

Thanks Banajit for the review!

On 04/05/18 20:04, Banajit Goswami wrote:

...

+static int q6core_callback(struct apr_device *adev, struct apr_resp_pkt *data) +{

• struct q6core *core = dev_get_drvdata(&adev->dev);
• struct aprv2_ibasic_rsp_result_t *result;
• struct apr_hdr *hdr = &data->hdr;
• result = data->payload;
• switch (hdr->opcode) {
• case APR_BASIC_RSP_RESULT:{

•    result = data->payload;
  

•    switch (result->opcode) {
  

•    case AVCS_GET_VERSIONS:
  

•        if (result->status == ADSP_EUNSUPPORTED)
  

•            core->get_version_supported = false;
  

•        core->resp_received = true;
  

•        break;
  

•    case AVCS_CMD_GET_FWK_VERSION:
  

•        if (result->status == ADSP_EUNSUPPORTED)
  

•            core->fwk_version_supported = false;
  

•        core->resp_received = true;
  

•        break;
  

•    case AVCS_CMD_ADSP_EVENT_GET_STATE:
  

•        if (result->status == ADSP_EUNSUPPORTED)
  

•            core->get_state_supported = false;
  

•        core->resp_received = true;
  

•        break;
  

•    }
  

•    break;
  

• }
• case AVCS_CMDRSP_GET_FWK_VERSION: {

•    struct avcs_cmdrsp_get_fwk_version *fwk;
  

•    int bytes;
  

•    fwk = data->payload;
  

•    core->fwk_version_supported = true;
  

•    bytes = sizeof(*fwk) + fwk->num_services *
  

•            sizeof(fwk->svc_api_info[0]);
  

•    core->fwk_version = kzalloc(bytes, GFP_ATOMIC);
  

•    if (!core->fwk_version)
  

•        return -ENOMEM;
  

When the above allocation fails, core->fwk_version_supported will be still true, and q6core_get_fwk_versions() will return 0 (timeout as core->resp_received will not be set to true). This can cause a NULL pointer dereference inside the if() loop pointed below (added comment). Please move the line to set core->fwk_version_supported flag to after memset() to copy fwk version info.

Yes, makes sense, I fixed this and other comments in v8.

thanks, srini

...

•    memcpy(core->fwk_version, data->payload, bytes);
  

•    core->resp_received = true;
  

•    break;
  

• }
• case AVCS_GET_VERSIONS_RSP: {

•    struct avcs_cmdrsp_get_version *v;
  

•    int len;
  

•    v = data->payload;
  

•    core->get_version_supported = true;