On Tue, 11 Apr 2023 16:52:58 +0100
Peter Maydell <peter.mayd...@linaro.org> wrote:
> On Mon, 7 Nov 2022 at 22:49, Michael S. Tsirkin <m...@redhat.com> wrote:
> >
> > From: Huai-Cheng Kuo <hch...@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 Peter,
I'll wait for the other thread to resolve the follow up with
a patch set cleaning up remaining issues you've pointed out.
Jonathan
>
> thanks
> -- PMM
