[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PULL v4 23/83] hw/cxl/cdat: CXL CDAT Data Object Exchange implement
From: |
Peter Maydell |
Subject: |
Re: [PULL v4 23/83] hw/cxl/cdat: CXL CDAT Data Object Exchange implementation |
Date: |
Tue, 11 Apr 2023 16:52:58 +0100 |
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
- Re: [PULL v4 23/83] hw/cxl/cdat: CXL CDAT Data Object Exchange implementation,
Peter Maydell <=