Re: [PULL v4 23/83] hw/cxl/cdat: CXL CDAT Data Object Exchange implementation

[Peter Maydell]

On Mon, 7 Nov 2022 at 22:49, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> From: Huai-Cheng Kuo <hchkuo@avery-design.com.tw>
>
> The Data Object Exchange implementation of CXL Coherent Device Attribute
> Table (CDAT). This implementation is referring to "Coherent Device
> Attribute Table Specification, Rev. 1.03, July. 2022" and "Compute
> Express Link Specification, Rev. 3.0, July. 2022"
>
> This patch adds core support that will be shared by both
> end-points and switch port emulation.

> +static void ct3_load_cdat(CDATObject *cdat, Error **errp)
> +{
> +    g_autofree CDATEntry *cdat_st = NULL;
> +    uint8_t sum = 0;
> +    int num_ent;
> +    int i = 0, ent = 1, file_size = 0;
> +    CDATSubHeader *hdr;
> +    FILE *fp = NULL;
> +
> +    /* Read CDAT file and create its cache */
> +    fp = fopen(cdat->filename, "r");
> +    if (!fp) {
> +        error_setg(errp, "CDAT: Unable to open file");
> +        return;
> +    }
> +
> +    fseek(fp, 0, SEEK_END);
> +    file_size = ftell(fp);

Coverity points out that ftell() can fail and return -1...

> +    fseek(fp, 0, SEEK_SET);
> +    cdat->buf = g_malloc0(file_size);

...which would cause an attempt to allocate a very large
amount of memory, since you aren't checking for errors.
CID 1508185.

Below, some other issues I saw in a quick scan through the code.

> +
> +    if (fread(cdat->buf, file_size, 1, fp) == 0) {
> +        error_setg(errp, "CDAT: File read failed");
> +        return;
> +    }

(The issues in this bit of code I've mentioned in a
different thread.)

> +
> +    fclose(fp);
> +
> +    if (file_size < sizeof(CDATTableHeader)) {
> +        error_setg(errp, "CDAT: File too short");
> +        return;
> +    }

> +    i = sizeof(CDATTableHeader);
> +    num_ent = 1;
> +    while (i < file_size) {
> +        hdr = (CDATSubHeader *)(cdat->buf + i);

If the file is not a complete number of records in
size, then this can index off the end of the buffer.
You should check for that.

> +        cdat_len_check(hdr, errp);
> +        i += hdr->length;
> +        num_ent++;
> +    }
> +    if (i != file_size) {
> +        error_setg(errp, "CDAT: File length missmatch");

Typo: "mismatch"

> +        return;
> +    }
> +
> +    cdat_st = g_malloc0(sizeof(*cdat_st) * num_ent);

To allocate an array of N lots of a structure, use
g_new0(), which will take care to avoid possible
overflow in the multiply.

> +    if (!cdat_st) {
> +        error_setg(errp, "CDAT: Failed to allocate entry array");

g_malloc0() and g_new0() can never fail, so you don't need
to check for a NULL pointer return.

> +        return;
> +    }
> +
> +    /* Set CDAT header, Entry = 0 */
> +    cdat_st[0].base = cdat->buf;
> +    cdat_st[0].length = sizeof(CDATTableHeader);
> +    i = 0;
> +
> +    while (i < cdat_st[0].length) {
> +        sum += cdat->buf[i++];
> +    }
> +
> +    /* Read CDAT structures */
> +    while (i < file_size) {
> +        hdr = (CDATSubHeader *)(cdat->buf + i);
> +        cdat_len_check(hdr, errp);

We already did this check the first time through the data...

> +
> +        cdat_st[ent].base = hdr;
> +        cdat_st[ent].length = hdr->length;
> +
> +        while (cdat->buf + i <
> +               (uint8_t *)cdat_st[ent].base + cdat_st[ent].length) {
> +            assert(i < file_size);
> +            sum += cdat->buf[i++];
> +        }
> +
> +        ent++;
> +    }
> +
> +    if (sum != 0) {
> +        warn_report("CDAT: Found checksum mismatch in %s", cdat->filename);
> +    }
> +    cdat->entry_len = num_ent;
> +    cdat->entry = g_steal_pointer(&cdat_st);
> +}
> +
> +void cxl_doe_cdat_init(CXLComponentState *cxl_cstate, Error **errp)
> +{
> +    CDATObject *cdat = &cxl_cstate->cdat;
> +
> +    if (cdat->filename) {
> +        ct3_load_cdat(cdat, errp);
> +    } else {
> +        ct3_build_cdat(cdat, errp);
> +    }
> +}

None of the callsites to this function check for it
failing. In particular they do not assume "if I call
this and it fails then I need to call cxl_doe_cdata_release()
to have it clean up". It would probably be less confusing
if the init function cleans up after itself, i.e. does not
leave allocated memory pointed to by cdat->buf and so on.

thanks
-- PMM