Re: [Xen-devel] [PATCH v2 3/5] n16550: add sanity check for reg_shift

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.01.16 at 06:57, <czylin@xxxxxxxxxxxx> wrote:
> Fix CID 1343302 by adding checking a check on the value of reg_shift.
> This patch also rolls the multiplication by 8 into the shift.
> No functional changes.
> 
> Suggested-by: Jan Beulich <JBeulich@xxxxxxxx>

I don't think so.

> Signed-off-by: Chester Lin <czylin@xxxxxxxxxxxx>
> ---
>  xen/drivers/char/ns16550.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index bc24015..55cfc45 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -913,7 +913,8 @@ pci_uart_config(struct ns16550 *uart, bool_t skip_amt, 
> unsigned int bar_idx)
>                           * Force length of mmio region to be at least
>                           * 8 bytes times (1 << reg_shift)
>                           */
> -                        if ( size < (0x8 * (1 << uart_param[p].reg_shift)) )
> +                        if ( uart_param[p].reg_shift > 27 ||
> +                             size < (1 << (uart_param[p].reg_shift + 3)) )
>                              continue;

Instead I think Coverity complaining is mad, and adding a
comparison here just clutters the code. The only thing I could
imagine I might have suggested would be to put an ASSERT()
here.

In any event should is the replacement of the multiplication
by an addition not what I think I had also mentioned before:
The expression, if changed in the first place, should use 8 as
the left operand of the shift.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel