| Message ID | 1532496652-26364-1-git-send-email-hongbo.zhang@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] hw/arm: check fw_cfg return value before using it | expand |
On 25 July 2018 at 06:30, Hongbo Zhang <hongbo.zhang@linaro.org> wrote: > The fw_cfg value returned from fw_cfg_find() may be NULL, so check it > before using. > > Signed-off-by: Hongbo Zhang <hongbo.zhang@linaro.org> Hi -- this patch series seems to be missing its cover letter. Please could you include a cover letter for any patchset with more than one patch -- some of our automated patch email handling tools rely on it. thanks -- PMM
On 25 July 2018 at 17:00, Peter Maydell <peter.maydell@linaro.org> wrote: > On 25 July 2018 at 06:30, Hongbo Zhang <hongbo.zhang@linaro.org> wrote: >> The fw_cfg value returned from fw_cfg_find() may be NULL, so check it >> before using. >> >> Signed-off-by: Hongbo Zhang <hongbo.zhang@linaro.org> > > Hi -- this patch series seems to be missing its cover letter. > Please could you include a cover letter for any patchset > with more than one patch -- some of our automated patch > email handling tools rely on it. > Get it. 2/2 needs a v3 at least, so will remember to include the cover letter in v3 then. Thanks. > thanks > -- PMM
On 25 July 2018 at 06:30, Hongbo Zhang <hongbo.zhang@linaro.org> wrote: > The fw_cfg value returned from fw_cfg_find() may be NULL, so check it > before using. > > Signed-off-by: Hongbo Zhang <hongbo.zhang@linaro.org> > --- > hw/arm/boot.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/hw/arm/boot.c b/hw/arm/boot.c > index e09201c..43b217f 100644 > --- a/hw/arm/boot.c > +++ b/hw/arm/boot.c > @@ -930,6 +930,7 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info) > hwaddr entry; > static const ARMInsnFixup *primary_loader; > AddressSpace *as = arm_boot_address_space(cpu, info); > + FWCfgState *fw_cfg; > > /* CPU objects (unlike devices) are not automatically reset on system > * reset, so we must always register a handler to do so. If we're > @@ -960,11 +961,10 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info) > info->dtb_start = info->loader_start; > } > > - if (info->kernel_filename) { > - FWCfgState *fw_cfg; > + fw_cfg = fw_cfg_find(); > + if (info->kernel_filename && fw_cfg) { > bool try_decompressing_kernel; This can only happen if: * the user provided a firmware blob * the user provided a -kernel option * the board does not have a fw_cfg so we can't pass the kernel to the firmware blob right? I think in this situation we should exit with a helpful error message to the user (telling them that this board model does not support using the -kernel option when a guest firmware image is being booted), rather than silently ignoring the option. thanks -- PMM
On 31 July 2018 at 02:07, Peter Maydell <peter.maydell@linaro.org> wrote: > On 25 July 2018 at 06:30, Hongbo Zhang <hongbo.zhang@linaro.org> wrote: >> The fw_cfg value returned from fw_cfg_find() may be NULL, so check it >> before using. >> >> Signed-off-by: Hongbo Zhang <hongbo.zhang@linaro.org> >> --- >> hw/arm/boot.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/hw/arm/boot.c b/hw/arm/boot.c >> index e09201c..43b217f 100644 >> --- a/hw/arm/boot.c >> +++ b/hw/arm/boot.c >> @@ -930,6 +930,7 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info) >> hwaddr entry; >> static const ARMInsnFixup *primary_loader; >> AddressSpace *as = arm_boot_address_space(cpu, info); >> + FWCfgState *fw_cfg; >> >> /* CPU objects (unlike devices) are not automatically reset on system >> * reset, so we must always register a handler to do so. If we're >> @@ -960,11 +961,10 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info) >> info->dtb_start = info->loader_start; >> } >> >> - if (info->kernel_filename) { >> - FWCfgState *fw_cfg; >> + fw_cfg = fw_cfg_find(); >> + if (info->kernel_filename && fw_cfg) { >> bool try_decompressing_kernel; > > This can only happen if: > * the user provided a firmware blob > * the user provided a -kernel option > * the board does not have a fw_cfg so we can't pass the kernel to > the firmware blob > right? > Yes. > I think in this situation we should exit with a helpful error > message to the user (telling them that this board model does > not support using the -kernel option when a guest firmware image > is being booted), rather than silently ignoring the option. > The suggestion sounds better. Thanks. > thanks > -- PMM
diff --git a/hw/arm/boot.c b/hw/arm/boot.c index e09201c..43b217f 100644 --- a/hw/arm/boot.c +++ b/hw/arm/boot.c @@ -930,6 +930,7 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info) hwaddr entry; static const ARMInsnFixup *primary_loader; AddressSpace *as = arm_boot_address_space(cpu, info); + FWCfgState *fw_cfg; /* CPU objects (unlike devices) are not automatically reset on system * reset, so we must always register a handler to do so. If we're @@ -960,11 +961,10 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info) info->dtb_start = info->loader_start; } - if (info->kernel_filename) { - FWCfgState *fw_cfg; + fw_cfg = fw_cfg_find(); + if (info->kernel_filename && fw_cfg) { bool try_decompressing_kernel; - fw_cfg = fw_cfg_find(); try_decompressing_kernel = arm_feature(&cpu->env, ARM_FEATURE_AARCH64);
The fw_cfg value returned from fw_cfg_find() may be NULL, so check it before using. Signed-off-by: Hongbo Zhang <hongbo.zhang@linaro.org> --- hw/arm/boot.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)