linux-kernel - Re: [PATCH v2 3/4] usb: dbgp: Fix return values for reset prep and startup

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] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 14 May 2021 11:33:45 -0400
From:   Boris Ostrovsky <boris.ostrovsky@...cle.com>
To:     Connor Davis <connojdavis@...il.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Juergen Gross <jgross@...e.com>,
        Stefano Stabellini <sstabellini@...nel.org>
Cc:     Douglas Anderson <dianders@...omium.org>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Chunfeng Yun <chunfeng.yun@...iatek.com>,
        Petr Mladek <pmladek@...e.com>,
        Sumit Garg <sumit.garg@...aro.org>,
        Lee Jones <lee.jones@...aro.org>, linux-usb@...r.kernel.org,
        linux-kernel@...r.kernel.org, xen-devel@...ts.xenproject.org
Subject: Re: [PATCH v2 3/4] usb: dbgp: Fix return values for reset prep and
 startup


On 5/13/21 8:56 PM, Connor Davis wrote:
> Callers of dbgp_reset_prep treat a 0 return value as "stop using
> the debug port", which means they don't make any subsequent calls to
> dbgp_reset_prep or dbgp_external_startup.
>
> To ensure the callers' interpretation is correct, first return -EPERM
> from xen_dbgp_op if !xen_initial_domain(). This ensures that
> both xen_dbgp_reset_prep and xen_dbgp_external_startup return 0
> iff the PHYSDEVOP_DBGP_RESET_{PREPARE,DONE} hypercalls succeed. Also
> update xen_dbgp_reset_prep and xen_dbgp_external_startup to return
> -EPERM when !CONFIG_XEN_DOM0 for consistency.
>
> Next, return nonzero from dbgp_reset_prep if xen_dbgp_reset_prep returns
> 0. The nonzero value ensures that callers of dbgp_reset_prep will
> subsequently call dbgp_external_startup when it is safe to do so.
>
> Also invert the return values from dbgp_external_startup for
> consistency with dbgp_reset_prep (this inversion has no functional
> change since no callers actually check the value).
>
> Signed-off-by: Connor Davis <connojdavis@...il.com>


For Xen bits:


Reviewed-by: Boris Ostrovsky <boris.ostrovsky@...cle.com>


For the rest it seems to me that error code passing could be improved: if it's 0 or 1 it should be bool. Or pass actual error code, with zero for no-error case, such as ...


> ---
>  drivers/usb/early/ehci-dbgp.c |  9 ++++++---
>  drivers/xen/dbgp.c            |  2 +-
>  include/linux/usb/ehci-dbgp.h | 14 +++++++++-----
>  3 files changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/usb/early/ehci-dbgp.c b/drivers/usb/early/ehci-dbgp.c
> index 45b42d8f6453..ff993d330c01 100644
> --- a/drivers/usb/early/ehci-dbgp.c
> +++ b/drivers/usb/early/ehci-dbgp.c
> @@ -970,8 +970,8 @@ int dbgp_reset_prep(struct usb_hcd *hcd)
>  	int ret = xen_dbgp_reset_prep(hcd);
>  	u32 ctrl;
>  
> -	if (ret)
> -		return ret;
> +	if (!ret)
> +		return 1;


... here or ...


>  
>  	dbgp_not_safe = 1;
>  	if (!ehci_debug)
> @@ -995,7 +995,10 @@ EXPORT_SYMBOL_GPL(dbgp_reset_prep);
>  
>  int dbgp_external_startup(struct usb_hcd *hcd)
>  {
> -	return xen_dbgp_external_startup(hcd) ?: _dbgp_external_startup();
> +	if (!xen_dbgp_external_startup(hcd))
> +		return 1;


... here.


-boris