On 12/15/2023 1:55 PM, Sivaramakrishnan, VenkatX wrote: > Hi Ferruh, > >> -----Original Message----- >> From: Ferruh Yigit <[email protected]> >> Sent: Friday, December 15, 2023 7:22 PM >> To: Sivaramakrishnan, VenkatX <[email protected]>; Hemant >> Agrawal <[email protected]>; Sachin Saxena >> <[email protected]>; Zyta Szpak <[email protected]>; Liron Himi >> <[email protected]>; Chaoyong He <[email protected]>; Gagandeep >> Singh <[email protected]>; Jerin Jacob <[email protected]>; Maciej Czekaj >> <[email protected]> >> Cc: [email protected]; Power, Ciara <[email protected]>; >> [email protected]; [email protected]; [email protected]; >> [email protected]; [email protected] >> Subject: Re: [PATCH v2] net/tap: fix buffer overflow for ptypes list >> >> On 12/15/2023 1:38 PM, Sivaramakrishnan Venkat wrote: >>> Incorrect ptypes list causes buffer overflow for Address Sanitizer >>> run. The last element in the ptypes lists to be "RTE_PTYPE_UNKNOWN" >>> for rte_eth_dev_get_supported_ptypes(). >>> In rte_eth_dev_get_supported_ptypes(),the loop iterates until it finds >>> "RTE_PTYPE_UNKNOWN" to detect last element of the ptypes array. >>> Fix the ptypes list for drivers. >>> >>> Fixes: 0849ac3b6122 ("net/tap: add packet type management") >>> Fixes: a7bdc3bd4244 ("net/dpaa: support packet type parsing") >>> Fixes: 4ccc8d770d3b ("net/mvneta: add PMD skeleton") >>> Fixes: f3f0d77db6b0 ("net/mrvl: support packet type parsing") >>> Fixes: 78a38edf66de ("ethdev: query supported packet types") >>> Fixes: 659b494d3d88 ("net/pfe: add packet types and basic statistics") >>> Fixes: 398a1be14168 ("net/thunderx: remove generic passX references") >>> Cc: [email protected] >>> Cc: [email protected] >>> Cc: [email protected] >>> Cc: [email protected] >>> Cc: [email protected] >>> Cc: [email protected] >>> Cc: [email protected] >>> >>> Signed-off-by: Sivaramakrishnan Venkat >>> <[email protected]> >>> >> >> Thanks Sivaramakrishnan for fixing all drivers. >> >> Acked-by: Ferruh Yigit <[email protected]> >> >> >> >> Is there any chance you can add relevant unit test to >> 'app/test/test_ethdev_api.c', this way it helps us prevent doing same >> mistake in >> the future? >> > Currently, the application didn't crash for an invalid ptypes list. > It is a silent buffer overflow that was only detected by running ASAN. > Could you please provide your inputs/ideas to implement a unit test for > invalid ptypes list. >
I was thinking just call the API and detect the crash, but if it doesn't cause crash it won't help much. This is .dev_supported_ptypes_get() design problem, it is relying on driver set array ending with 'RTE_PTYPE_UNKNOWN' but there is no way to verify it. Also this requirement is not documented very well. Please scratch the ask to add unit test. Perhaps we can change the '.dev_supported_ptypes_get()', this should be possible without impacting the user, just by updating drivers. '.dev_supported_ptypes_get()' can be updated as: typedef const uint32_t * (*eth_dev_supported_ptypes_get_t)(struct rte_eth_dev *dev, uint32_t num); 'num' is simply size of returned 'ptypes' array. This eliminates need to have 'RTE_PTYPE_UNKNOWN' as last item, and overall change is not so big. What do you think, does new dev_ops fingerprint make sense to you?
