linux-kernel - Re: Unsafe find

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]

Message-ID: <2024112853-conjure-posh-bf7a@gregkh>
Date: Thu, 28 Nov 2024 16:07:20 +0100
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Christian Brauner <brauner@...nel.org>
Cc: Scott Branden <scott.branden@...adcom.com>,
	Hans de Goede <hdegoede@...hat.com>,
	Larry Finger <Larry.Finger@...inger.net>,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	Broadcom internal kernel review list <bcm-kernel-feedback-list@...adcom.com>
Subject: Re: Unsafe find_vpid() calls

On Thu, Nov 28, 2024 at 03:58:06PM +0100, Christian Brauner wrote:
> Hey,
> 
> You have various calls to find_vpid() in your drivers that aren't
> protected by either tasklist_lock or rcu_read_lock(). Afair, this is
> unsafe as the struct pid might be freed beneath you. You should please
> fix those places to be protected by rcu_read_lock(). Something like the
> below or similar should work.
> 
> Thanks!
> Christian
> 
> diff --git a/drivers/misc/bcm-vk/bcm_vk_dev.c b/drivers/misc/bcm-vk/bcm_vk_dev.c
> index d4a96137728d..84cab909db71 100644
> --- a/drivers/misc/bcm-vk/bcm_vk_dev.c
> +++ b/drivers/misc/bcm-vk/bcm_vk_dev.c
> @@ -522,7 +522,9 @@ void bcm_vk_blk_drv_access(struct bcm_vk *vk)
>                                 dev_dbg(&vk->pdev->dev,
>                                         "Send kill signal to pid %d\n",
>                                         ctx->pid);
> +                               rcu_read_lock();
>                                 kill_pid(find_vpid(ctx->pid), SIGKILL, 1);
> +                               rcu_read_unlock();
>                         }
>                 }
>         }
> diff --git a/drivers/misc/bcm-vk/bcm_vk_tty.c b/drivers/misc/bcm-vk/bcm_vk_tty.c
> index 59bab76ff0a9..6bd93347938e 100644
> --- a/drivers/misc/bcm-vk/bcm_vk_tty.c
> +++ b/drivers/misc/bcm-vk/bcm_vk_tty.c
> @@ -326,8 +326,11 @@ void bcm_vk_tty_terminate_tty_user(struct bcm_vk *vk)
> 
>         for (i = 0; i < BCM_VK_NUM_TTY; ++i) {
>                 vktty = &vk->tty[i];
> -               if (vktty->pid)
> +               if (vktty->pid) {
> +                       rcu_read_lock();
>                         kill_pid(find_vpid(vktty->pid), SIGKILL, 1);
> +                       rcu_read_unlock();
> +               }
>         }
>  }
> 
> diff --git a/drivers/staging/rtl8712/rtl8712_cmd.c b/drivers/staging/rtl8712/rtl8712_cmd.c
> index bb7db96ed821..de13f4eab60f 100644
> --- a/drivers/staging/rtl8712/rtl8712_cmd.c
> +++ b/drivers/staging/rtl8712/rtl8712_cmd.c
> @@ -61,7 +61,9 @@ static void check_hw_pbc(struct _adapter *padapter)
>                  */
>                 if (padapter->pid == 0)
>                         return;
> +               rcu_read_lock();
>                 kill_pid(find_vpid(padapter->pid), SIGUSR1, 1);
> +               rcu_read_unlock();
>         }
>  }

Odds are all of these usages can just be removed entirely, I'll add it
to the "todo" list of mine...

thanks!

greg k-h