conf: Check for bandwidth limits during parsing

[PATCH] conf: Check for bandwidth limits during parsing

Posted by Michal Privoznik 2 months ago

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@redhat.com>
---
 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");
+
     if (!rc_average && !rc_floor) {
         virReportError(VIR_ERR_XML_DETAIL, "%s",
                        _("Missing mandatory average or floor attributes"));
@@ -71,6 +86,8 @@ virNetDevBandwidthParseRate(xmlNodePtr node,
     return 0;
 }
 
+#undef CHECK_LIMIT
+
 /**
  * virNetDevBandwidthParse:
  * @bandwidth: parsed bandwidth
diff --git a/src/conf/schemas/networkcommon.rng b/src/conf/schemas/networkcommon.rng
index 6df6d43f54..2b3f902ffe 100644
--- a/src/conf/schemas/networkcommon.rng
+++ b/src/conf/schemas/networkcommon.rng
@@ -180,7 +180,7 @@
   </define>
 
   <define name="speed">
-    <data type="unsignedInt">
+    <data type="unsignedLong">
       <param name="pattern">[0-9]+</param>
       <param name="minInclusive">1</param>
     </data>
-- 
2.44.2

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

Posted by Peter Krempa 1 month, 1 week ago

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@redhat.com>
> ---
>  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.

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

Posted by Michal Prívozník 3 weeks, 4 days ago

On 7/29/24 15:31, Peter Krempa wrote:
> 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@redhat.com>
>> ---
>>  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.
> 

One could argue that:

a) these limits couldn't ever be set successfully,
b) nobody really sets 16EB/s rates (the limit introduced here).

But okay. Let me post v2.

Michal