[PATCH net] net: qed: prevent buffer overflow when collecting debug data

[PATCH net] net: qed: prevent buffer overflow when collecting debug data

[Alexander Lobakin]

When generating debug dump, driver firstly collects all data in binary
form, and then performs per-feature formatting to human-readable if it
is supported.
The size of the new formatted data is often larger than the raw's. This
becomes critical when user requests dump via ethtool (-d/-w), as output
buffer size is strictly determined (by ethtool_ops::get_regs_len() etc),
as it may lead to out-of-bounds writes and memory corruption.

To not go past initial lengths, add a flag to return original,
non-formatted debug data, and set it in such cases. Also set data type
in regdump headers, so userland parsers could handle it.

Fixes: c965db444629 ("qed: Add support for debug data collection")
Signed-off-by: Alexander Lobakin <alobakin@marvell.com>
Signed-off-by: Igor Russkikh <irusskikh@marvell.com>
---
 drivers/net/ethernet/qlogic/qed/qed.h       |  2 ++
 drivers/net/ethernet/qlogic/qed/qed_debug.c | 13 ++++++++++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/qlogic/qed/qed.h b/drivers/net/ethernet/qlogic/qed/qed.h
index a49743d56b9c..6c2f9ff4a53e 100644
--- a/drivers/net/ethernet/qlogic/qed/qed.h
+++ b/drivers/net/ethernet/qlogic/qed/qed.h
@@ -876,6 +876,8 @@ struct qed_dev {
 	struct qed_dbg_feature dbg_features[DBG_FEATURE_NUM];
 	u8 engine_for_debug;
 	bool disable_ilt_dump;
+	bool				dbg_bin_dump;
+
 	DECLARE_HASHTABLE(connections, 10);
 	const struct firmware		*firmware;
 
diff --git a/drivers/net/ethernet/qlogic/qed/qed_debug.c b/drivers/net/ethernet/qlogic/qed/qed_debug.c
index 81e8fbe4a05b..cb80863d5a77 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_debug.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_debug.c
@@ -7506,6 +7506,12 @@ static enum dbg_status format_feature(struct qed_hwfn *p_hwfn,
 	if (p_hwfn->cdev->print_dbg_data)
 		qed_dbg_print_feature(text_buf, text_size_bytes);
 
+	/* Just return the original binary buffer if requested */
+	if (p_hwfn->cdev->dbg_bin_dump) {
+		vfree(text_buf);
+		return DBG_STATUS_OK;
+	}
+
 	/* Free the old dump_buf and point the dump_buf to the newly allocagted
 	 * and formatted text buffer.
 	 */
@@ -7733,7 +7739,9 @@ int qed_dbg_mcp_trace_size(struct qed_dev *cdev)
 #define REGDUMP_HEADER_SIZE_SHIFT		0
 #define REGDUMP_HEADER_SIZE_MASK		0xffffff
 #define REGDUMP_HEADER_FEATURE_SHIFT		24
-#define REGDUMP_HEADER_FEATURE_MASK		0x3f
+#define REGDUMP_HEADER_FEATURE_MASK		0x1f
+#define REGDUMP_HEADER_BIN_DUMP_SHIFT		29
+#define REGDUMP_HEADER_BIN_DUMP_MASK		0x1
 #define REGDUMP_HEADER_OMIT_ENGINE_SHIFT	30
 #define REGDUMP_HEADER_OMIT_ENGINE_MASK		0x1
 #define REGDUMP_HEADER_ENGINE_SHIFT		31
@@ -7771,6 +7779,7 @@ static u32 qed_calc_regdump_header(struct qed_dev *cdev,
 			  feature, feature_size);
 
 	SET_FIELD(res, REGDUMP_HEADER_FEATURE, feature);
+	SET_FIELD(res, REGDUMP_HEADER_BIN_DUMP, 1);
 	SET_FIELD(res, REGDUMP_HEADER_OMIT_ENGINE, omit_engine);
 	SET_FIELD(res, REGDUMP_HEADER_ENGINE, engine);
 
@@ -7794,6 +7803,7 @@ int qed_dbg_all_data(struct qed_dev *cdev, void *buffer)
 		omit_engine = 1;
 
 	mutex_lock(&qed_dbg_lock);
+	cdev->dbg_bin_dump = true;
 
 	org_engine = qed_get_debug_engine(cdev);
 	for (cur_engine = 0; cur_engine < cdev->num_hwfns; cur_engine++) {
@@ -7993,6 +8003,7 @@ int qed_dbg_all_data(struct qed_dev *cdev, void *buffer)
 		       QED_NVM_IMAGE_MDUMP, "QED_NVM_IMAGE_MDUMP", rc);
 	}
 
+	cdev->dbg_bin_dump = false;
 	mutex_unlock(&qed_dbg_lock);
 
 	return 0;
-- 
2.25.1

Re: [PATCH net] net: qed: prevent buffer overflow when collecting debug data

[David Miller]

From: Alexander Lobakin <alobakin@marvell.com>
Date: Fri, 3 Jul 2020 12:02:58 +0300

> When generating debug dump, driver firstly collects all data in binary
> form, and then performs per-feature formatting to human-readable if it
> is supported.
> The size of the new formatted data is often larger than the raw's. This
> becomes critical when user requests dump via ethtool (-d/-w), as output
> buffer size is strictly determined (by ethtool_ops::get_regs_len() etc),
> as it may lead to out-of-bounds writes and memory corruption.
> 
> To not go past initial lengths, add a flag to return original,
> non-formatted debug data, and set it in such cases. Also set data type
> in regdump headers, so userland parsers could handle it.
> 
> Fixes: c965db444629 ("qed: Add support for debug data collection")
> Signed-off-by: Alexander Lobakin <alobakin@marvell.com>
> Signed-off-by: Igor Russkikh <irusskikh@marvell.com>

This is now how ethtool register dumps work.

It does not provide "human readable" versions of register data.  Instead
it is supposed to be purely raw data and then userland utilities interpret
that data and can make it human readable based upon the driver name and
reg dump version.

Please fix your ethtool -d implementation to comply with this.

Thank you.

Re: [PATCH net] net: qed: prevent buffer overflow when collecting debug data

[Alexander Lobakin]

From:   David Miller <davem@davemloft.net>
Date:   Fri, 03 Jul 2020 12:59:33 -0700 (PDT)

> From: Alexander Lobakin <alobakin@marvell.com>
> Date: Fri, 3 Jul 2020 12:02:58 +0300
> 
> > When generating debug dump, driver firstly collects all data in binary
> > form, and then performs per-feature formatting to human-readable if it
> > is supported.
> > The size of the new formatted data is often larger than the raw's. This
> > becomes critical when user requests dump via ethtool (-d/-w), as output
> > buffer size is strictly determined (by ethtool_ops::get_regs_len() etc),
> > as it may lead to out-of-bounds writes and memory corruption.
> > 
> > To not go past initial lengths, add a flag to return original,
> > non-formatted debug data, and set it in such cases. Also set data type
> > in regdump headers, so userland parsers could handle it.
> > 
> > Fixes: c965db444629 ("qed: Add support for debug data collection")
> > Signed-off-by: Alexander Lobakin <alobakin@marvell.com>
> > Signed-off-by: Igor Russkikh <irusskikh@marvell.com>
> 
> This is now how ethtool register dumps work.
> 
> It does not provide "human readable" versions of register data.  Instead
> it is supposed to be purely raw data and then userland utilities interpret
> that data and can make it human readable based upon the driver name and
> reg dump version.
> 
> Please fix your ethtool -d implementation to comply with this.

This is exactly what this patch does: forces driver to dump raw binary
data. Current mainline version tries to perform formatting before passing
data up to ethtool infra.

> Thank you.

Thanks,
Al