| Message ID | 20240731143617.3391947-5-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | block: Miscellaneous minor Coverity fixes | expand |
Peter Maydell <peter.maydell@linaro.org> writes: > In ide_atapi_cmd_reply_end() we calculate a 16-bit size, and then > assign its two halves to s->lcyl and s->hcyl like this: > > s->lcyl = size; > s->hcyl = size >> 8; > > Coverity warns that the first line here can overflow the > 8-bit s->lcyl variable. This is true, and in this case we're > deliberately only after the low 8 bits of the value. The > code is clearer to both humans and Coverity if we're explicit > that we only wanted the low 8 bits, though. > Resolves? > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > hw/ide/atapi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c > index fcb6cca1573..e82959dc2d3 100644 > --- a/hw/ide/atapi.c > +++ b/hw/ide/atapi.c > @@ -265,7 +265,7 @@ void ide_atapi_cmd_reply_end(IDEState *s) /* a new transfer is needed */ s->nsector = (s->nsector & ~7) | ATAPI_INT_REASON_IO; ide_bus_set_irq(s->bus); byte_count_limit = atapi_byte_count_limit(s); trace_ide_atapi_cmd_reply_end_bcl(s, byte_count_limit); size = s->packet_transfer_size; if (size > byte_count_limit) { /* byte count limit must be even if this case */ if (byte_count_limit & 1) > byte_count_limit--; > size = byte_count_limit; > } > - s->lcyl = size; > + s->lcyl = size & 0xff; > s->hcyl = size >> 8; > s->elementary_transfer_size = size; > /* we cannot transmit more than one sector at a time */ @size is int. I wonder why it's fine with size >> 8... ah, atapi_byte_count_limit() returns uint16_t, and Coverity is smart enough to data-flow this via @byte_count_limit into @size. Reviewed-by: Markus Armbruster <armbru@redhat.com>
On Wed, 31 Jul 2024 at 15:47, Markus Armbruster <armbru@redhat.com> wrote: > > Peter Maydell <peter.maydell@linaro.org> writes: > > > In ide_atapi_cmd_reply_end() we calculate a 16-bit size, and then > > assign its two halves to s->lcyl and s->hcyl like this: > > > > s->lcyl = size; > > s->hcyl = size >> 8; > > > > Coverity warns that the first line here can overflow the > > 8-bit s->lcyl variable. This is true, and in this case we're > > deliberately only after the low 8 bits of the value. The > > code is clearer to both humans and Coverity if we're explicit > > that we only wanted the low 8 bits, though. > > > > Resolves? Whoops. Resolves: Coverity CID 1547621 > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > --- > > hw/ide/atapi.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c > > index fcb6cca1573..e82959dc2d3 100644 > > --- a/hw/ide/atapi.c > > +++ b/hw/ide/atapi.c > > @@ -265,7 +265,7 @@ void ide_atapi_cmd_reply_end(IDEState *s) > /* a new transfer is needed */ > s->nsector = (s->nsector & ~7) | ATAPI_INT_REASON_IO; > ide_bus_set_irq(s->bus); > byte_count_limit = atapi_byte_count_limit(s); > trace_ide_atapi_cmd_reply_end_bcl(s, byte_count_limit); > size = s->packet_transfer_size; > if (size > byte_count_limit) { > /* byte count limit must be even if this case */ > if (byte_count_limit & 1) > > byte_count_limit--; > > size = byte_count_limit; > > } > > - s->lcyl = size; > > + s->lcyl = size & 0xff; > > s->hcyl = size >> 8; > > s->elementary_transfer_size = size; > > /* we cannot transmit more than one sector at a time */ > > @size is int. I wonder why it's fine with size >> 8... ah, > atapi_byte_count_limit() returns uint16_t, and Coverity is smart enough > to data-flow this via @byte_count_limit into @size. > > Reviewed-by: Markus Armbruster <armbru@redhat.com> thanks -- PMM
Am 31.07.2024 um 16:36 hat Peter Maydell geschrieben: > In ide_atapi_cmd_reply_end() we calculate a 16-bit size, and then > assign its two halves to s->lcyl and s->hcyl like this: > > s->lcyl = size; > s->hcyl = size >> 8; > > Coverity warns that the first line here can overflow the > 8-bit s->lcyl variable. This is true, and in this case we're > deliberately only after the low 8 bits of the value. The > code is clearer to both humans and Coverity if we're explicit > that we only wanted the low 8 bits, though. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c index fcb6cca1573..e82959dc2d3 100644 --- a/hw/ide/atapi.c +++ b/hw/ide/atapi.c @@ -265,7 +265,7 @@ void ide_atapi_cmd_reply_end(IDEState *s) byte_count_limit--; size = byte_count_limit; } - s->lcyl = size; + s->lcyl = size & 0xff; s->hcyl = size >> 8; s->elementary_transfer_size = size; /* we cannot transmit more than one sector at a time */
In ide_atapi_cmd_reply_end() we calculate a 16-bit size, and then assign its two halves to s->lcyl and s->hcyl like this: s->lcyl = size; s->hcyl = size >> 8; Coverity warns that the first line here can overflow the 8-bit s->lcyl variable. This is true, and in this case we're deliberately only after the low 8 bits of the value. The code is clearer to both humans and Coverity if we're explicit that we only wanted the low 8 bits, though. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/ide/atapi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)