1. Patchew
  2. Libvirt
  3. View series

[PATCH] network: ensure nparams is non-negative

Daniel P. Berrangé posted 1 patch 2 weeks, 2 days ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20240416101324.121619-1-berrange@redhat.com
src/libvirt-network.c | 2 ++
1 file changed, 2 insertions(+)
[PATCH] network: ensure nparams is non-negative
Posted by Daniel P. Berrangé 2 weeks, 2 days ago 
The typed parameter array must be either 0, or a positive
number.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 src/libvirt-network.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/libvirt-network.c b/src/libvirt-network.c
index ef17a8a04d..e467716b6a 100644
--- a/src/libvirt-network.c
+++ b/src/libvirt-network.c
@@ -1577,6 +1577,8 @@ virNetworkPortGetParameters(virNetworkPortPtr port,
     virCheckNetworkPortReturn(port, -1);
     conn = port->net->conn;
 
+    virCheckNonNegativeArgGoto(*nparams, error);
+
     if (conn->networkDriver && conn->networkDriver->networkPortGetParameters) {
         int ret;
         ret = conn->networkDriver->networkPortGetParameters(port, params, nparams, flags);
-- 
2.43.0
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: [PATCH] network: ensure nparams is non-negative
Posted by Ján Tomko 2 weeks, 2 days ago 
On a Tuesday in 2024, Daniel P. Berrangé wrote:
>The typed parameter array must be either 0, or a positive
>number.
>

Does this matter?

The API documentation says:
  * @nparams: pointer to received number of interface parameter

and it looks like we ignore the number as long as params is NULL.

Jano

>Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>---
> src/libvirt-network.c | 2 ++
> 1 file changed, 2 insertions(+)
>
>diff --git a/src/libvirt-network.c b/src/libvirt-network.c
>index ef17a8a04d..e467716b6a 100644
>--- a/src/libvirt-network.c
>+++ b/src/libvirt-network.c
>@@ -1577,6 +1577,8 @@ virNetworkPortGetParameters(virNetworkPortPtr port,
>     virCheckNetworkPortReturn(port, -1);
>     conn = port->net->conn;
>
>+    virCheckNonNegativeArgGoto(*nparams, error);
>+
>     if (conn->networkDriver && conn->networkDriver->networkPortGetParameters) {
>         int ret;
>         ret = conn->networkDriver->networkPortGetParameters(port, params, nparams, flags);
>-- 
>2.43.0
>_______________________________________________
>Devel mailing list -- devel@lists.libvirt.org
>To unsubscribe send an email to devel-leave@lists.libvirt.org
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: [PATCH] network: ensure nparams is non-negative
Posted by Daniel P. Berrangé 2 weeks, 2 days ago 
On Tue, Apr 16, 2024 at 12:58:53PM +0200, Ján Tomko wrote:
> On a Tuesday in 2024, Daniel P. Berrangé wrote:
> > The typed parameter array must be either 0, or a positive
> > number.
> > 
> 
> Does this matter?
> 
> The API documentation says:
>  * @nparams: pointer to received number of interface parameter
> 
> and it looks like we ignore the number as long as params is NULL.

This missing check is something I noticed when fixing the recent
CVE about RPC checking nparams. In all other APIs we have such
a virCheckNonNegativeArgGoto for '*nparams', this was the only
one that was missing.

I believe it is harmless in terms of risk to libvirt/libvirtd,
but it might lead to better detection/reporting of bugs in apps.

> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> > src/libvirt-network.c | 2 ++
> > 1 file changed, 2 insertions(+)
> > 
> > diff --git a/src/libvirt-network.c b/src/libvirt-network.c
> > index ef17a8a04d..e467716b6a 100644
> > --- a/src/libvirt-network.c
> > +++ b/src/libvirt-network.c
> > @@ -1577,6 +1577,8 @@ virNetworkPortGetParameters(virNetworkPortPtr port,
> >     virCheckNetworkPortReturn(port, -1);
> >     conn = port->net->conn;
> > 
> > +    virCheckNonNegativeArgGoto(*nparams, error);
> > +
> >     if (conn->networkDriver && conn->networkDriver->networkPortGetParameters) {
> >         int ret;
> >         ret = conn->networkDriver->networkPortGetParameters(port, params, nparams, flags);
> > -- 
> > 2.43.0
> > _______________________________________________
> > Devel mailing list -- devel@lists.libvirt.org
> > To unsubscribe send an email to devel-leave@lists.libvirt.org



With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: [PATCH] network: ensure nparams is non-negative
Posted by Ján Tomko 2 weeks, 1 day ago 
On a Tuesday in 2024, Daniel P. Berrangé wrote:
>On Tue, Apr 16, 2024 at 12:58:53PM +0200, Ján Tomko wrote:
>> On a Tuesday in 2024, Daniel P. Berrangé wrote:
>> > The typed parameter array must be either 0, or a positive
>> > number.
>> >
>>
>> Does this matter?
>>
>> The API documentation says:
>>  * @nparams: pointer to received number of interface parameter
>>
>> and it looks like we ignore the number as long as params is NULL.
>
>This missing check is something I noticed when fixing the recent
>CVE about RPC checking nparams. In all other APIs we have such
>a virCheckNonNegativeArgGoto for '*nparams', this was the only
>one that was missing.
>
>I believe it is harmless in terms of risk to libvirt/libvirtd,
>but it might lead to better detection/reporting of bugs in apps.
>

I was thinking someone initializing *nparams to -1 could be a valid use
according to the documentation (with params = NULL), but if we do it the
same for all other similar APIs, it's okay.

However, it seems 'nparams' being NULL is not valid whether it's
the user or libvirt allocating the array, so please also add:

virCheckNonNullArgGoto(nparams, error);

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano

>> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>> > ---
>> > src/libvirt-network.c | 2 ++
>> > 1 file changed, 2 insertions(+)
>> >
>> > diff --git a/src/libvirt-network.c b/src/libvirt-network.c
>> > index ef17a8a04d..e467716b6a 100644
>> > --- a/src/libvirt-network.c
>> > +++ b/src/libvirt-network.c
>> > @@ -1577,6 +1577,8 @@ virNetworkPortGetParameters(virNetworkPortPtr port,
>> >     virCheckNetworkPortReturn(port, -1);
>> >     conn = port->net->conn;
>> >
>> > +    virCheckNonNegativeArgGoto(*nparams, error);
>> > +
>> >     if (conn->networkDriver && conn->networkDriver->networkPortGetParameters) {
>> >         int ret;
>> >         ret = conn->networkDriver->networkPortGetParameters(port, params, nparams, flags);
>> > --
>> > 2.43.0
>> > _______________________________________________
>> > Devel mailing list -- devel@lists.libvirt.org
>> > To unsubscribe send an email to devel-leave@lists.libvirt.org
>
>
>
>With regards,
>Daniel
>-- 
>|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
>|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
>|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
>
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: [PATCH] network: ensure nparams is non-negative
Posted by Daniel P. Berrangé 2 weeks, 1 day ago 
On Wed, Apr 17, 2024 at 02:03:38PM +0200, Ján Tomko wrote:
> On a Tuesday in 2024, Daniel P. Berrangé wrote:
> > On Tue, Apr 16, 2024 at 12:58:53PM +0200, Ján Tomko wrote:
> > > On a Tuesday in 2024, Daniel P. Berrangé wrote:
> > > > The typed parameter array must be either 0, or a positive
> > > > number.
> > > >
> > > 
> > > Does this matter?
> > > 
> > > The API documentation says:
> > >  * @nparams: pointer to received number of interface parameter
> > > 
> > > and it looks like we ignore the number as long as params is NULL.
> > 
> > This missing check is something I noticed when fixing the recent
> > CVE about RPC checking nparams. In all other APIs we have such
> > a virCheckNonNegativeArgGoto for '*nparams', this was the only
> > one that was missing.
> > 
> > I believe it is harmless in terms of risk to libvirt/libvirtd,
> > but it might lead to better detection/reporting of bugs in apps.
> > 
> 
> I was thinking someone initializing *nparams to -1 could be a valid use
> according to the documentation (with params = NULL), but if we do it the
> same for all other similar APIs, it's okay.
> 
> However, it seems 'nparams' being NULL is not valid whether it's
> the user or libvirt allocating the array, so please also add:
> 
> virCheckNonNullArgGoto(nparams, error);

Opps yes, all the other APIs have this check, so should have
it here too.

> 
> Reviewed-by: Ján Tomko <jtomko@redhat.com>
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.