| Message ID | 20240806134829.351703-3-chalapathi.v@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/ppc: SPI model - coverity fixes | expand |
On 6/8/24 15:48, Chalapathi V wrote: > In this commit the following coverity scan defect has been fixed > CID 1558831: Resource leaks (RESOURCE_LEAK) > Variable "rsp_payload" going out of scope leaks the storage it > points to. > > Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com> > --- > hw/ssi/pnv_spi.c | 1 + > 1 file changed, 1 insertion(+) Fixes: b4cb930e40 ("hw/ssi: Extend SPI model")
Back at this patch since Cédric asked me to look at it. On 6/8/24 15:48, Chalapathi V wrote: > In this commit the following coverity scan defect has been fixed > CID 1558831: Resource leaks (RESOURCE_LEAK) > Variable "rsp_payload" going out of scope leaks the storage it > points to. > > Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com> > --- > hw/ssi/pnv_spi.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c > index a33f682897..dbe06df224 100644 > --- a/hw/ssi/pnv_spi.c > +++ b/hw/ssi/pnv_spi.c > @@ -237,6 +237,7 @@ static void transfer(PnvSpi *s, PnvXferBuffer *payload) > } > if (rsp_payload != NULL) { > spi_response(s, s->N1_bits, rsp_payload); > + pnv_spi_xfer_buffer_free(rsp_payload); > } > } > Or bigger patch but simpler logic: -- >8 -- diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c index c1297ab733..aaedba84af 100644 --- a/hw/ssi/pnv_spi.c +++ b/hw/ssi/pnv_spi.c @@ -217,6 +217,9 @@ static void transfer(PnvSpi *s, PnvXferBuffer *payload) PnvXferBuffer *rsp_payload = NULL; rsp_payload = pnv_spi_xfer_buffer_new(); + if (!rsp_payload) { + return; + } for (int offset = 0; offset < payload->len; offset += s->transfer_len) { tx = 0; for (int i = 0; i < s->transfer_len; i++) { @@ -235,9 +238,8 @@ static void transfer(PnvSpi *s, PnvXferBuffer *payload) (rx >> (8 * (s->transfer_len - 1) - i * 8)) & 0xFF; } } - if (rsp_payload != NULL) { - spi_response(s, s->N1_bits, rsp_payload); - } + spi_response(s, s->N1_bits, rsp_payload); + pnv_spi_xfer_buffer_free(rsp_payload); } --- I note few things is odd here: 1/ pnv_spi_xfer_buffer_new() uses the GLib g_malloc0() call while pnv_spi_xfer_buffer_free() uses plan free(). 2/ pnv_spi_xfer_buffer_free() frees payload->data so doesn't match pnv_spi_xfer_buffer_new(). This is a bit disappointing.
On Wed, 7 Aug 2024 at 21:13, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > Back at this patch since Cédric asked me to look at it. > > On 6/8/24 15:48, Chalapathi V wrote: > > In this commit the following coverity scan defect has been fixed > > CID 1558831: Resource leaks (RESOURCE_LEAK) > > Variable "rsp_payload" going out of scope leaks the storage it > > points to. > > > > Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com> > > --- > > hw/ssi/pnv_spi.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c > > index a33f682897..dbe06df224 100644 > > --- a/hw/ssi/pnv_spi.c > > +++ b/hw/ssi/pnv_spi.c > > @@ -237,6 +237,7 @@ static void transfer(PnvSpi *s, PnvXferBuffer *payload) > > } > > if (rsp_payload != NULL) { > > spi_response(s, s->N1_bits, rsp_payload); > > + pnv_spi_xfer_buffer_free(rsp_payload); > > } > > } > > > > Or bigger patch but simpler logic: > > -- >8 -- > diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c > index c1297ab733..aaedba84af 100644 > --- a/hw/ssi/pnv_spi.c > +++ b/hw/ssi/pnv_spi.c > @@ -217,6 +217,9 @@ static void transfer(PnvSpi *s, PnvXferBuffer *payload) > PnvXferBuffer *rsp_payload = NULL; > > rsp_payload = pnv_spi_xfer_buffer_new(); > + if (!rsp_payload) { > + return; > + } pnv_spi_xfer_buffer_new() cannot fail (it calls g_malloc0, which aborts on not having enough memory) so we don't need to check it for NULL. > for (int offset = 0; offset < payload->len; offset += > s->transfer_len) { > tx = 0; > for (int i = 0; i < s->transfer_len; i++) { > @@ -235,9 +238,8 @@ static void transfer(PnvSpi *s, PnvXferBuffer *payload) > (rx >> (8 * (s->transfer_len - 1) - i * 8)) & 0xFF; > } > } > - if (rsp_payload != NULL) { > - spi_response(s, s->N1_bits, rsp_payload); > - } (So this NULL check was unnecessary in the old code.) > + spi_response(s, s->N1_bits, rsp_payload); > + pnv_spi_xfer_buffer_free(rsp_payload); > } > > --- > > I note few things is odd here: > > 1/ pnv_spi_xfer_buffer_new() uses the GLib g_malloc0() call > while pnv_spi_xfer_buffer_free() uses plan free(). This does work (glib guarantees now that g_malloc and friends can be paired with free), but it is not great style. > 2/ pnv_spi_xfer_buffer_free() frees payload->data so doesn't > match pnv_spi_xfer_buffer_new(). This part I think is OK -- the payload->data buffer is allocated with g_realloc(), so pnv_spi_xfer_buffer_new() creates a valid initial state (payload->data = NULL and payload->len = 0), pnv_spi_xfer_buffer_write_ptr() may allocate or extend the buffer (using g_realloc and updating the payload->len to match), and pnv_spi_xfer_buffer_free() will free the buffer (including handling the "buffer never allocated so payload->data = NULL" case, because free(NULL) is OK). What it doesn't get right I think is that when pnv_spi_xfer_buffer_write_ptr() extends the payload->data buffer it doesn't zero the newly added memory, so I think we might end up giving the guest back the contents of uninitialized memory. Another style issue is: PnvXferBuffer *payload = g_malloc0(sizeof(*payload)); g_new0(PnvXferBuffer, 1) is the better way to say "allocate me a zeroed out PnvXferBuffer", rather than manual use of sizeof. The various places which do if (*payload == NULL) { *payload = pnv_spi_xfer_buffer_new(); } also look odd to me -- I'm pretty sure that in operation_shiftn1() and operation_shiftn2() it is impossible for them to be called with a NULL payload. The PnvXferBuffer struct is only 12 bytes large (a length and a pointer), so it seems rather overkill to be allocating and freeing it -- is it possible to have it be a local variable instead? My other question here is whether we need to be doing dynamic memory allocation, extension and freeing of the payload->data at all. This is quite an unusual thing to need to do in a hardware device model, because usually the real hardware we're modelling doesn't have infinite resources, it has some fixed amount of memory that it has to work with. If the guest can effectively ask us to allocate arbitrary amounts of memory, do we have appropriate guards in place to forbid that? Is there a datasheet for this device? thanks -- PMM
diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c index a33f682897..dbe06df224 100644 --- a/hw/ssi/pnv_spi.c +++ b/hw/ssi/pnv_spi.c @@ -237,6 +237,7 @@ static void transfer(PnvSpi *s, PnvXferBuffer *payload) } if (rsp_payload != NULL) { spi_response(s, s->N1_bits, rsp_payload); + pnv_spi_xfer_buffer_free(rsp_payload); } }
In this commit the following coverity scan defect has been fixed CID 1558831: Resource leaks (RESOURCE_LEAK) Variable "rsp_payload" going out of scope leaks the storage it points to. Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com> --- hw/ssi/pnv_spi.c | 1 + 1 file changed, 1 insertion(+)