linux-kernel - Re: [PATCH v3] usb: cdnsp: Fix issue with CV Bad Descriptor test

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]
Message-ID: <20250623082201.GA53043@nchen-desktop>
Date: Mon, 23 Jun 2025 16:22:01 +0800
From: "Peter Chen (CIX)" <peter.chen@...nel.org>
To: Pawel Laszczak <pawell@...ence.com>
Cc: "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: Re: [PATCH v3] usb: cdnsp: Fix issue with CV Bad Descriptor test

On 25-06-23 05:51:08, Pawel Laszczak wrote:
> >On 25-06-20 08:23:12, Pawel Laszczak wrote:
> >> The SSP2 controller has extra endpoint state preserve bit (ESP) which
> >> setting causes that endpoint state will be preserved during Halt
> >> Endpoint command. It is used only for EP0.
> >> Without this bit the Command Verifier "TD 9.10 Bad Descriptor Test"
> >> failed.
> >> Setting this bit doesn't have any impact for SSP controller.
> >>
> >> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence
> >> USBSSP DRD Driver")
> >> cc: stable@...r.kernel.org
> >> Signed-off-by: Pawel Laszczak <pawell@...ence.com>
> >> ---
> >> Changelog:
> >> v3:
> >> - removed else {}
> >>
> >> v2:
> >> - removed some typos
> >> - added pep variable initialization
> >> - updated TRB_ESP description
> >>
> >>  drivers/usb/cdns3/cdnsp-debug.h  |  5 +++--
> >>  drivers/usb/cdns3/cdnsp-ep0.c    | 18 +++++++++++++++---
> >>  drivers/usb/cdns3/cdnsp-gadget.h |  6 ++++++
> >>  drivers/usb/cdns3/cdnsp-ring.c   |  3 ++-
> >>  4 files changed, 26 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/usb/cdns3/cdnsp-debug.h
> >> b/drivers/usb/cdns3/cdnsp-debug.h index cd138acdcce1..86860686d836
> >> 100644
> >> --- a/drivers/usb/cdns3/cdnsp-debug.h
> >> +++ b/drivers/usb/cdns3/cdnsp-debug.h
> >> @@ -327,12 +327,13 @@ static inline const char *cdnsp_decode_trb(char
> >*str, size_t size, u32 field0,
> >>  	case TRB_RESET_EP:
> >>  	case TRB_HALT_ENDPOINT:
> >>  		ret = scnprintf(str, size,
> >> -				"%s: ep%d%s(%d) ctx %08x%08x slot %ld flags
> >%c",
> >> +				"%s: ep%d%s(%d) ctx %08x%08x slot %ld flags
> >%c %c",
> >>  				cdnsp_trb_type_string(type),
> >>  				ep_num, ep_id % 2 ? "out" : "in",
> >>  				TRB_TO_EP_INDEX(field3), field1, field0,
> >>  				TRB_TO_SLOT_ID(field3),
> >> -				field3 & TRB_CYCLE ? 'C' : 'c');
> >> +				field3 & TRB_CYCLE ? 'C' : 'c',
> >> +				field3 & TRB_ESP ? 'P' : 'p');
> >>  		break;
> >>  	case TRB_STOP_RING:
> >>  		ret = scnprintf(str, size,
> >> diff --git a/drivers/usb/cdns3/cdnsp-ep0.c
> >> b/drivers/usb/cdns3/cdnsp-ep0.c index f317d3c84781..5cd9b898ce97
> >> 100644
> >> --- a/drivers/usb/cdns3/cdnsp-ep0.c
> >> +++ b/drivers/usb/cdns3/cdnsp-ep0.c
> >> @@ -414,6 +414,7 @@ static int cdnsp_ep0_std_request(struct
> >> cdnsp_device *pdev,  void cdnsp_setup_analyze(struct cdnsp_device
> >> *pdev)  {
> >>  	struct usb_ctrlrequest *ctrl = &pdev->setup;
> >> +	struct cdnsp_ep *pep;
> >>  	int ret = -EINVAL;
> >>  	u16 len;
> >>
> >> @@ -427,10 +428,21 @@ void cdnsp_setup_analyze(struct cdnsp_device
> >*pdev)
> >>  		goto out;
> >>  	}
> >>
> >> +	pep = &pdev->eps[0];
> >> +
> >>  	/* Restore the ep0 to Stopped/Running state. */
> >> -	if (pdev->eps[0].ep_state & EP_HALTED) {
> >> -		trace_cdnsp_ep0_halted("Restore to normal state");
> >> -		cdnsp_halt_endpoint(pdev, &pdev->eps[0], 0);
> >> +	if (pep->ep_state & EP_HALTED) {
> >> +		if (GET_EP_CTX_STATE(pep->out_ctx) == EP_STATE_HALTED)
> >> +			cdnsp_halt_endpoint(pdev, pep, 0);

You mean above is only called for SSP? And below two lines needs to
be executed no matter cdnsp_halt_endpoint(pdev, pep, 0) is called?
	
pep->ep_state &= ~EP_HALTED;
pep->ep_state |= EP_STOPPED;

If it is the case, I am okay with this patch.

Peter

> >> +
> >> +		/*
> >> +		 * Halt Endpoint Command for SSP2 for ep0 preserve current
> >> +		 * endpoint state and driver has to synchronize the
> >> +		 * software endpoint state with endpoint output context
> >> +		 * state.
> >> +		 */
> >> +		pep->ep_state &= ~EP_HALTED;
> >> +		pep->ep_state |= EP_STOPPED;
> >
> >You do not reset endpoint by calling clear_halt, could we change ep_state
> >directly?
> 
> It's only "software" endpoint state and this code is related only with ep0.
> For SSP2 the state in pep->out_ctx - "hardware" endpoint state in this
> place will be in EP_STATE_STOPPED but "software" pep->ep_state
> will be EP_HALTED. 
> Driver only synchronizes pep->ep_state with this included in pep->out_ctx.
> 
> For SSP the state in pep->out_ctx - "hardware" endpoint state in this please
> will be in EP_STATE_HALTED, and "software" pep->ep_state will be
> EP_HALTED. For SSP driver will call cdnsp_halt_endpoint in which
> it changes the "hardware" and  "software" endpoint state
> to EP_STOPPED/EP_STATE_STOPPED.
> 
> So for SSP the extra code:
> 		pep->ep_state &= ~EP_HALTED;
> 		pep->ep_state |= EP_STOPPED;
> will not change anything
> 
> Pawel
> 
> >
> >Peter
> >>  	}
> >>
> >>  	/*
> >> diff --git a/drivers/usb/cdns3/cdnsp-gadget.h
> >> b/drivers/usb/cdns3/cdnsp-gadget.h
> >> index 2afa3e558f85..a91cca509db0 100644
> >> --- a/drivers/usb/cdns3/cdnsp-gadget.h
> >> +++ b/drivers/usb/cdns3/cdnsp-gadget.h
> >> @@ -987,6 +987,12 @@ enum cdnsp_setup_dev {
> >>  #define STREAM_ID_FOR_TRB(p)		((((p)) << 16) & GENMASK(31,
> >16))
> >>  #define SCT_FOR_TRB(p)			(((p) << 1) & 0x7)
> >>
> >> +/*
> >> + * Halt Endpoint Command TRB field.
> >> + * The ESP bit only exists in the SSP2 controller.
> >> + */
> >> +#define TRB_ESP				BIT(9)
> >> +
> >>  /* Link TRB specific fields. */
> >>  #define TRB_TC				BIT(1)
> >>
> >> diff --git a/drivers/usb/cdns3/cdnsp-ring.c
> >> b/drivers/usb/cdns3/cdnsp-ring.c index fd06cb85c4ea..d397d28efc6e
> >> 100644
> >> --- a/drivers/usb/cdns3/cdnsp-ring.c
> >> +++ b/drivers/usb/cdns3/cdnsp-ring.c
> >> @@ -2483,7 +2483,8 @@ void cdnsp_queue_halt_endpoint(struct
> >> cdnsp_device *pdev, unsigned int ep_index)  {
> >>  	cdnsp_queue_command(pdev, 0, 0, 0, TRB_TYPE(TRB_HALT_ENDPOINT)
> >|
> >>  			    SLOT_ID_FOR_TRB(pdev->slot_id) |
> >> -			    EP_ID_FOR_TRB(ep_index));
> >> +			    EP_ID_FOR_TRB(ep_index) |
> >> +			    (!ep_index ? TRB_ESP : 0));
> >>  }
> >>
> >>  void cdnsp_force_header_wakeup(struct cdnsp_device *pdev, int
> >> intf_num)
> >> --
> >> 2.43.0
> >>
> >
> >--
> >
> >Best regards,
> >Peter

-- 

Best regards,
Peter