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

[Jonathan Cameron via <qemu-devel@nongnu.org>]

On Tue, 11 Apr 2023 16:52:58 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> 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.)

I'll carry a patch locally using g_file_get_contents()
that gets rid of this mess. My assumption being that similar
will emerge from other thread and I can drop my patch.

> 
> > +
> > +    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"
> 

That's fixed in the tree.

> > +        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.

dropped.  I'll remember this one day ;)

> 
> > +        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...
dropped
> 
> > +
> > +        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.
Agreed.  Will make it cleanup and add the error checks at the two
callers.

Thanks,

Jonathan

> 
> thanks
> -- PMM