| Message ID | oPimw5pKWjOolRVo_7-wrc75fq9RQccGIZFwWLTLupIprA9PaVm6IDzlE5M_MEZLCWtT31P55BAwc1VgCpEGyB21qyGLDRekNlK6ueu4MaA=@szczek.dev (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/i386/pc: Fix enum value assertion | expand |
On 8/21/24 08:52, Kamil Szczęk wrote: > Coverity reported: > >>> CID 1559533: Integer handling issues (CONSTANT_EXPRESSION_RESULT) > >>> "pcms->vmport >= 0" is always true regardless of the values of > >>> its operands. This occurs as the logical first operand of "&&". > > Signed-off-by: Kamil Szczęk <kamil@szczek.dev> > Reported-By: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > hw/i386/pc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 7779c88a91..523dfe3f3f 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1221,7 +1221,7 @@ void pc_basic_device_init(struct PCMachineState *pcms, > isa_realize_and_unref(pcms->pcspk, isa_bus, &error_fatal); > } > > - assert(pcms->vmport >= 0 && pcms->vmport < ON_OFF_AUTO__MAX); > + assert(pcms->vmport < ON_OFF_AUTO__MAX); Given the usage here > if (pcms->vmport == ON_OFF_AUTO_AUTO) { and later, here > pc_superio_init(isa_bus, create_fdctrl, pcms->i8042_enabled, > pcms->vmport != ON_OFF_AUTO_ON, &error_fatal); absolutely nothing goes off the rails no matter the value of vmport. It is not used to index an array, which might be out of bounds. It it not a security issue. There's no need or benefit for the assert at all. r~
On Wednesday, August 21st, 2024 at 00:59, Richard Henderson <richard.henderson@linaro.org> wrote: > On 8/21/24 08:52, Kamil Szczęk wrote: > > > Coverity reported: > > > > > > > CID 1559533: Integer handling issues (CONSTANT_EXPRESSION_RESULT) > > > > > "pcms->vmport >= 0" is always true regardless of the values of > > > > > its operands. This occurs as the logical first operand of "&&". > > > > Signed-off-by: Kamil Szczęk kamil@szczek.dev > > Reported-By: Philippe Mathieu-Daudé philmd@linaro.org > > --- > > hw/i386/pc.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > index 7779c88a91..523dfe3f3f 100644 > > --- a/hw/i386/pc.c > > +++ b/hw/i386/pc.c > > @@ -1221,7 +1221,7 @@ void pc_basic_device_init(struct PCMachineState *pcms, > > isa_realize_and_unref(pcms->pcspk, isa_bus, &error_fatal); > > } > > > > - assert(pcms->vmport >= 0 && pcms->vmport < ON_OFF_AUTO__MAX); > > + assert(pcms->vmport < ON_OFF_AUTO__MAX); > > > Given the usage here > > > if (pcms->vmport == ON_OFF_AUTO_AUTO) { > > > and later, here > > > pc_superio_init(isa_bus, create_fdctrl, pcms->i8042_enabled, > > pcms->vmport != ON_OFF_AUTO_ON, &error_fatal); > > > absolutely nothing goes off the rails no matter the value of vmport. > > It is not used to index an array, which might be out of bounds. > It it not a security issue. > There's no need or benefit for the assert at all. > Agreed, v2 posted.
diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 7779c88a91..523dfe3f3f 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1221,7 +1221,7 @@ void pc_basic_device_init(struct PCMachineState *pcms, isa_realize_and_unref(pcms->pcspk, isa_bus, &error_fatal); } - assert(pcms->vmport >= 0 && pcms->vmport < ON_OFF_AUTO__MAX); + assert(pcms->vmport < ON_OFF_AUTO__MAX); if (pcms->vmport == ON_OFF_AUTO_AUTO) { pcms->vmport = (xen_enabled() || !pcms->i8042_enabled) ? ON_OFF_AUTO_OFF : ON_OFF_AUTO_ON;