Re: [PATCH] conf: Check for bandwidth limits during parsing

On Thu, Jul 04, 2024 at 13:02:46 +0200, Michal Privoznik wrote: > The 'tc' program stores speeds in 64bit integers (unit is bytes > per second) and sizes in uints (unit is bytes). We use different > units: kilobytes per second and kibibytes and therefore we can > parse values larger than 'tc' can handle. Reject those values > right away. > > And while at it, fix the schema which assumed speed values fit > into uint. > > Resolves: https://issues.redhat.com/browse/RHEL-45200 > Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; > --- > src/conf/netdev_bandwidth_conf.c | 17 +++++++++++++++++ > src/conf/schemas/networkcommon.rng | 2 +- > 2 files changed, 18 insertions(+), 1 deletion(-) > > diff --git a/src/conf/netdev_bandwidth_conf.c b/src/conf/netdev_bandwidth_conf.c > index 9faa46a27f..f3f0b2209a 100644 > --- a/src/conf/netdev_bandwidth_conf.c > +++ b/src/conf/netdev_bandwidth_conf.c > @@ -24,6 +24,16 @@ > > #define VIR_FROM_THIS VIR_FROM_NONE > > +#define CHECK_LIMIT(val, limit, name) \ > + do { \ > + if ((val) > (limit)) { \ > + virReportError(VIR_ERR_OVERFLOW, \ > + _("value '%1$llu' is too big for '%2$s' parameter, maximum is '%3$llu'"), \ > + val, name, (unsigned long long) limit); \ > + return -1; \ > + } \ > + } while (0) > + > static int > virNetDevBandwidthParseRate(xmlNodePtr node, > virNetDevBandwidthRate *rate, > @@ -50,6 +60,11 @@ virNetDevBandwidthParseRate(xmlNodePtr node, > &rate->floor)) < 0) > return -1; > > + CHECK_LIMIT(rate->average, 1ULL << 54, "average"); > + CHECK_LIMIT(rate->peak, 1ULL << 54, "peak"); > + CHECK_LIMIT(rate->burst, UINT_MAX >> 10, "burst"); > + CHECK_LIMIT(rate->floor, 1ULL << 54, "floor"); This is making the parser stricter than it used to be, which could result into failures when parsing existing libvirt-stored configs. 'virNetDevBandwidthParseRate()' is called from 'virNetDevBandwidthParse()' which in turn is used e.g. in 'virDomainNetDefParseXML()' thus in worst case could lead into losing defined VMs. As of such I don't think this validation can be in the parser.