| Message ID | 1453183077-50542-1-git-send-email-czylin@uwaterloo.ca (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 19.01.16 at 06:57, <czylin@uwaterloo.ca> 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@suse.com> I don't think so. > Signed-off-by: Chester Lin <czylin@uwaterloo.ca> > --- > 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
Quoting Jan Beulich <JBeulich@suse.com>: >>>> On 19.01.16 at 06:57, <czylin@uwaterloo.ca> 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@suse.com> > > I don't think so. > >> Signed-off-by: Chester Lin <czylin@uwaterloo.ca> >> --- >> 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@lists.xen.org > http://lists.xen.org/xen-devel > Sorry for the confusion regarding the suggested-by tag. Thank you for reviewing our patches. We agree that it would make more sense to suppress the error in Coverity. As such, we will not be sending this patch for further review. Regards, Chester Lin
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; if ( bar_idx >= uart_param[p].max_bars )
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@suse.com> Signed-off-by: Chester Lin <czylin@uwaterloo.ca> --- xen/drivers/char/ns16550.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)