netdev - Re: [PATCH net-next] net: ipconfig: avoid use-after-free in ic_close

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 22 Mar 2021 13:01:26 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Heiko Thiery <heiko.thiery@...il.com>
Cc:     andrew@...n.ch, davem@...emloft.net, f.fainelli@...il.com,
        kuba@...nel.org, netdev@...r.kernel.org, vivien.didelot@...il.com,
        yoshfuji@...ux-ipv6.org, michael@...le.cc
Subject: Re: [PATCH net-next] net: ipconfig: avoid use-after-free in
 ic_close_devs

Hi Heiko,

On Mon, Mar 22, 2021 at 11:57:33AM +0100, Heiko Thiery wrote:
> Hi Vladimir and all,
> 
> > Due to the fact that ic_dev->dev is kept open in ic_close_dev, I had
> > thought that ic_dev will not be freed either. But that is not the case,
> > but instead "everybody dies" when ipconfig cleans up, and just the
> > net_device behind ic_dev->dev remains allocated but not ic_dev itself.
> > 
> > This is a problem because in ic_close_devs, for every net device that
> > we're about to close, we compare it against the list of lower interfaces
> > of ic_dev, to figure out whether we should close it or not. But since
> > ic_dev itself is subject to freeing, this means that at some point in
> > the middle of the list of ipconfig interfaces, ic_dev will have been
> > freed, and we would be still attempting to iterate through its list of
> > lower interfaces while checking whether to bring down the remaining
> > ipconfig interfaces.
> > 
> > There are multiple ways to avoid the use-after-free: we could delay
> > freeing ic_dev until the very end (outside the while loop). Or an even
> > simpler one: we can observe that we don't need ic_dev when iterating
> > through its lowers, only ic_dev->dev, structure which isn't ever freed.
> > So, by keeping ic_dev->dev in a variable assigned prior to freeing
> > ic_dev, we can avoid all use-after-free issues.
> > 
> > Fixes: 46acf7bdbc72 ("Revert "net: ipv4: handle DSA enabled master network devices"")
> > Reported-by: kernel test robot <oliver.sang@...el.com>
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> > ---
> >  net/ipv4/ipconfig.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/net/ipv4/ipconfig.c b/net/ipv4/ipconfig.c
> > index f9ab1fb219ec..47db1bfdaaa0 100644
> > --- a/net/ipv4/ipconfig.c
> > +++ b/net/ipv4/ipconfig.c
> > @@ -309,6 +309,7 @@ static int __init ic_open_devs(void)
> >   */
> >  static void __init ic_close_devs(void)
> >  {
> > +	struct net_device *selected_dev = ic_dev->dev;
> 
> This will causes a kernel panic when ic_dev is not valid.
> 
> See log output here: https://lavalab.kontron.com/scheduler/job/12453
> 
> >  	struct ic_device *d, *next;
> >  	struct net_device *dev;
> >  
> > @@ -322,7 +323,7 @@ static void __init ic_close_devs(void)
> >  		next = d->next;
> >  		dev = d->dev;
> >  
> > -		netdev_for_each_lower_dev(ic_dev->dev, lower_dev, iter) {
> > +		netdev_for_each_lower_dev(selected_dev, lower_dev, iter) {
> >  			if (dev == lower_dev) {
> >  				bring_down = false;
> >  				break;
> > -- 
> > 2.25.1
> 
> 
> I can fix my issue with this:
> 
> diff --git a/net/ipv4/ipconfig.c b/net/ipv4/ipconfig.c
> index 47db1bfdaaa0..6f1df4336153 100644
> --- a/net/ipv4/ipconfig.c
> +++ b/net/ipv4/ipconfig.c
> @@ -309,7 +309,6 @@ static int __init ic_open_devs(void)
>   */
>  static void __init ic_close_devs(void)
>  {
> -       struct net_device *selected_dev = ic_dev->dev;
>         struct ic_device *d, *next;
>         struct net_device *dev;
>  
> @@ -323,10 +322,13 @@ static void __init ic_close_devs(void)
>                 next = d->next;
>                 dev = d->dev;
>  
> -               netdev_for_each_lower_dev(selected_dev, lower_dev, iter) {
> -                       if (dev == lower_dev) {
> -                               bring_down = false;
> -                               break;
> +               if (ic_dev) {
> +                       struct net_device *selected_dev = ic_dev->dev;
> +                       netdev_for_each_lower_dev(selected_dev, lower_dev, iter) {
> +                               if (dev == lower_dev) {
> +                                       bring_down = false;
> +                                       break;
> +                               }
>                         }
>                 }
>                 if (bring_down) {
> 
> 
> What do you think? Is this valid?
> 
> Thank you.
> -- 
> Heiko

I guess you're the right person to give me a Reviewed-by/Tested-by here:
https://patchwork.kernel.org/project/netdevbpf/patch/20210322002637.3412657-1-olteanv@gmail.com/