* [PATCH] drm/dp: Actually read Adjust Request Post Cursor2 register
@ 2021-12-03 8:43 ` Kees Cook
0 siblings, 0 replies; 16+ messages in thread
From: Kees Cook @ 2021-12-03 8:43 UTC (permalink / raw)
To: Maarten Lankhorst
Cc: Kees Cook, Maxime Ripard, Thomas Zimmermann, David Airlie,
Daniel Vetter, Philipp Zabel, Thierry Reding, Lyude Paul,
linux-kernel, dri-devel, linux-hardening
The link_status array was not large enough to read the Adjust Request
Post Cursor2 register. Adjust the size to include it. Found with a
-Warray-bounds build:
drivers/gpu/drm/drm_dp_helper.c: In function 'drm_dp_get_adjust_request_post_cursor':
drivers/gpu/drm/drm_dp_helper.c:59:27: error: array subscript 10 is outside array bounds of 'const u8[6]' {aka 'const unsigned char[6]'} [-Werror=array-bounds]
59 | return link_status[r - DP_LANE0_1_STATUS];
| ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/drm_dp_helper.c:147:51: note: while referencing 'link_status'
147 | u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZE],
| ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Fixes: 79465e0ffeb9 ("drm/dp: Add helper to get post-cursor adjustments")
Signed-off-by: Kees Cook <keescook@chromium.org>
---
include/drm/drm_dp_helper.h | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 472dac376284..277643d2fe2c 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -1517,7 +1517,15 @@ enum drm_dp_phy {
#define DP_MST_LOGICAL_PORT_0 8
#define DP_LINK_CONSTANT_N_VALUE 0x8000
-#define DP_LINK_STATUS_SIZE 6
+/*
+ * DPCD registers in link_status:
+ * Link Status: 0x202 through 0x204
+ * Sink Status: 0x205
+ * Adjust Request: 0x206 through 0x207
+ * Training Score: 0x208 through 0x20b
+ * AR Post Cursor2: 0x20c
+ */
+#define DP_LINK_STATUS_SIZE 11
bool drm_dp_channel_eq_ok(const u8 link_status[DP_LINK_STATUS_SIZE],
int lane_count);
bool drm_dp_clock_recovery_ok(const u8 link_status[DP_LINK_STATUS_SIZE],
--
2.30.2
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH] drm/dp: Actually read Adjust Request Post Cursor2 register
@ 2021-12-03 8:43 ` Kees Cook
0 siblings, 0 replies; 16+ messages in thread
From: Kees Cook @ 2021-12-03 8:43 UTC (permalink / raw)
To: Maarten Lankhorst
Cc: David Airlie, linux-kernel, linux-hardening, dri-devel,
Thomas Zimmermann, Thierry Reding, Kees Cook
The link_status array was not large enough to read the Adjust Request
Post Cursor2 register. Adjust the size to include it. Found with a
-Warray-bounds build:
drivers/gpu/drm/drm_dp_helper.c: In function 'drm_dp_get_adjust_request_post_cursor':
drivers/gpu/drm/drm_dp_helper.c:59:27: error: array subscript 10 is outside array bounds of 'const u8[6]' {aka 'const unsigned char[6]'} [-Werror=array-bounds]
59 | return link_status[r - DP_LANE0_1_STATUS];
| ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/drm_dp_helper.c:147:51: note: while referencing 'link_status'
147 | u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZE],
| ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Fixes: 79465e0ffeb9 ("drm/dp: Add helper to get post-cursor adjustments")
Signed-off-by: Kees Cook <keescook@chromium.org>
---
include/drm/drm_dp_helper.h | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 472dac376284..277643d2fe2c 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -1517,7 +1517,15 @@ enum drm_dp_phy {
#define DP_MST_LOGICAL_PORT_0 8
#define DP_LINK_CONSTANT_N_VALUE 0x8000
-#define DP_LINK_STATUS_SIZE 6
+/*
+ * DPCD registers in link_status:
+ * Link Status: 0x202 through 0x204
+ * Sink Status: 0x205
+ * Adjust Request: 0x206 through 0x207
+ * Training Score: 0x208 through 0x20b
+ * AR Post Cursor2: 0x20c
+ */
+#define DP_LINK_STATUS_SIZE 11
bool drm_dp_channel_eq_ok(const u8 link_status[DP_LINK_STATUS_SIZE],
int lane_count);
bool drm_dp_clock_recovery_ok(const u8 link_status[DP_LINK_STATUS_SIZE],
--
2.30.2
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] drm/dp: Actually read Adjust Request Post Cursor2 register
2021-12-03 8:43 ` Kees Cook
@ 2021-12-08 11:19 ` Jani Nikula
-1 siblings, 0 replies; 16+ messages in thread
From: Jani Nikula @ 2021-12-08 11:19 UTC (permalink / raw)
To: Kees Cook, Maarten Lankhorst
Cc: David Airlie, linux-kernel, linux-hardening, dri-devel,
Thomas Zimmermann, Thierry Reding, Kees Cook
On Fri, 03 Dec 2021, Kees Cook <keescook@chromium.org> wrote:
> The link_status array was not large enough to read the Adjust Request
> Post Cursor2 register. Adjust the size to include it. Found with a
> -Warray-bounds build:
>
> drivers/gpu/drm/drm_dp_helper.c: In function 'drm_dp_get_adjust_request_post_cursor':
> drivers/gpu/drm/drm_dp_helper.c:59:27: error: array subscript 10 is outside array bounds of 'const u8[6]' {aka 'const unsigned char[6]'} [-Werror=array-bounds]
> 59 | return link_status[r - DP_LANE0_1_STATUS];
> | ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
> drivers/gpu/drm/drm_dp_helper.c:147:51: note: while referencing 'link_status'
> 147 | u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZE],
> | ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Fixes: 79465e0ffeb9 ("drm/dp: Add helper to get post-cursor adjustments")
> Signed-off-by: Kees Cook <keescook@chromium.org>
Using DP_ADJUST_REQUEST_POST_CURSOR2 has been deprecated since DP 1.3
published in 2014, and Tegra is the only user of
drm_dp_get_adjust_request_post_cursor().
Instead of bumping the link status read size from 6 to 11 for all
drivers I'd much rather see some other (maybe Tegra specific) solution
to this.
BR,
Jani.
> ---
> include/drm/drm_dp_helper.h | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 472dac376284..277643d2fe2c 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -1517,7 +1517,15 @@ enum drm_dp_phy {
> #define DP_MST_LOGICAL_PORT_0 8
>
> #define DP_LINK_CONSTANT_N_VALUE 0x8000
> -#define DP_LINK_STATUS_SIZE 6
> +/*
> + * DPCD registers in link_status:
> + * Link Status: 0x202 through 0x204
> + * Sink Status: 0x205
> + * Adjust Request: 0x206 through 0x207
> + * Training Score: 0x208 through 0x20b
> + * AR Post Cursor2: 0x20c
> + */
> +#define DP_LINK_STATUS_SIZE 11
> bool drm_dp_channel_eq_ok(const u8 link_status[DP_LINK_STATUS_SIZE],
> int lane_count);
> bool drm_dp_clock_recovery_ok(const u8 link_status[DP_LINK_STATUS_SIZE],
--
Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] drm/dp: Actually read Adjust Request Post Cursor2 register
@ 2021-12-08 11:19 ` Jani Nikula
0 siblings, 0 replies; 16+ messages in thread
From: Jani Nikula @ 2021-12-08 11:19 UTC (permalink / raw)
To: Kees Cook, Maarten Lankhorst
Cc: Kees Cook, David Airlie, linux-kernel, dri-devel,
linux-hardening, Thomas Zimmermann, Thierry Reding
On Fri, 03 Dec 2021, Kees Cook <keescook@chromium.org> wrote:
> The link_status array was not large enough to read the Adjust Request
> Post Cursor2 register. Adjust the size to include it. Found with a
> -Warray-bounds build:
>
> drivers/gpu/drm/drm_dp_helper.c: In function 'drm_dp_get_adjust_request_post_cursor':
> drivers/gpu/drm/drm_dp_helper.c:59:27: error: array subscript 10 is outside array bounds of 'const u8[6]' {aka 'const unsigned char[6]'} [-Werror=array-bounds]
> 59 | return link_status[r - DP_LANE0_1_STATUS];
> | ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
> drivers/gpu/drm/drm_dp_helper.c:147:51: note: while referencing 'link_status'
> 147 | u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZE],
> | ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Fixes: 79465e0ffeb9 ("drm/dp: Add helper to get post-cursor adjustments")
> Signed-off-by: Kees Cook <keescook@chromium.org>
Using DP_ADJUST_REQUEST_POST_CURSOR2 has been deprecated since DP 1.3
published in 2014, and Tegra is the only user of
drm_dp_get_adjust_request_post_cursor().
Instead of bumping the link status read size from 6 to 11 for all
drivers I'd much rather see some other (maybe Tegra specific) solution
to this.
BR,
Jani.
> ---
> include/drm/drm_dp_helper.h | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 472dac376284..277643d2fe2c 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -1517,7 +1517,15 @@ enum drm_dp_phy {
> #define DP_MST_LOGICAL_PORT_0 8
>
> #define DP_LINK_CONSTANT_N_VALUE 0x8000
> -#define DP_LINK_STATUS_SIZE 6
> +/*
> + * DPCD registers in link_status:
> + * Link Status: 0x202 through 0x204
> + * Sink Status: 0x205
> + * Adjust Request: 0x206 through 0x207
> + * Training Score: 0x208 through 0x20b
> + * AR Post Cursor2: 0x20c
> + */
> +#define DP_LINK_STATUS_SIZE 11
> bool drm_dp_channel_eq_ok(const u8 link_status[DP_LINK_STATUS_SIZE],
> int lane_count);
> bool drm_dp_clock_recovery_ok(const u8 link_status[DP_LINK_STATUS_SIZE],
--
Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] drm/dp: Actually read Adjust Request Post Cursor2 register
2021-12-08 11:19 ` Jani Nikula
@ 2021-12-09 5:54 ` Kees Cook
-1 siblings, 0 replies; 16+ messages in thread
From: Kees Cook @ 2021-12-09 5:54 UTC (permalink / raw)
To: Jani Nikula
Cc: Maarten Lankhorst, David Airlie, linux-kernel, linux-hardening,
dri-devel, Thomas Zimmermann, Thierry Reding
On Wed, Dec 08, 2021 at 01:19:28PM +0200, Jani Nikula wrote:
> On Fri, 03 Dec 2021, Kees Cook <keescook@chromium.org> wrote:
> > The link_status array was not large enough to read the Adjust Request
> > Post Cursor2 register. Adjust the size to include it. Found with a
> > -Warray-bounds build:
> >
> > drivers/gpu/drm/drm_dp_helper.c: In function 'drm_dp_get_adjust_request_post_cursor':
> > drivers/gpu/drm/drm_dp_helper.c:59:27: error: array subscript 10 is outside array bounds of 'const u8[6]' {aka 'const unsigned char[6]'} [-Werror=array-bounds]
> > 59 | return link_status[r - DP_LANE0_1_STATUS];
> > | ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
> > drivers/gpu/drm/drm_dp_helper.c:147:51: note: while referencing 'link_status'
> > 147 | u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZE],
> > | ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > Fixes: 79465e0ffeb9 ("drm/dp: Add helper to get post-cursor adjustments")
> > Signed-off-by: Kees Cook <keescook@chromium.org>
>
> Using DP_ADJUST_REQUEST_POST_CURSOR2 has been deprecated since DP 1.3
> published in 2014, and Tegra is the only user of
> drm_dp_get_adjust_request_post_cursor().
>
> Instead of bumping the link status read size from 6 to 11 for all
> drivers I'd much rather see some other (maybe Tegra specific) solution
> to this.
Hmmm... Well given this is currently non-functional on Tegra (and is an
OOB memory read), how about just removing it entirely?
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 23f9073bc473..c9528aa62c9c 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -144,16 +144,6 @@ u8 drm_dp_get_adjust_tx_ffe_preset(const u8 link_status[DP_LINK_STATUS_SIZE],
}
EXPORT_SYMBOL(drm_dp_get_adjust_tx_ffe_preset);
-u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZE],
- unsigned int lane)
-{
- unsigned int offset = DP_ADJUST_REQUEST_POST_CURSOR2;
- u8 value = dp_link_status(link_status, offset);
-
- return (value >> (lane << 1)) & 0x3;
-}
-EXPORT_SYMBOL(drm_dp_get_adjust_request_post_cursor);
-
static int __8b10b_clock_recovery_delay_us(const struct drm_dp_aux *aux, u8 rd_interval)
{
if (rd_interval > 4)
diff --git a/drivers/gpu/drm/tegra/dp.c b/drivers/gpu/drm/tegra/dp.c
index 70dfb7d1dec5..bb5bfa93950f 100644
--- a/drivers/gpu/drm/tegra/dp.c
+++ b/drivers/gpu/drm/tegra/dp.c
@@ -559,8 +559,7 @@ static void drm_dp_link_get_adjustments(struct drm_dp_link *link,
drm_dp_get_adjust_request_pre_emphasis(status, i) >>
DP_TRAIN_PRE_EMPHASIS_SHIFT;
- adjust->post_cursor[i] =
- drm_dp_get_adjust_request_post_cursor(status, i);
+ adjust->post_cursor[i] = 0;
}
}
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 30359e434c3f..28378db676c8 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -1528,8 +1528,6 @@ u8 drm_dp_get_adjust_request_pre_emphasis(const u8 link_status[DP_LINK_STATUS_SI
int lane);
u8 drm_dp_get_adjust_tx_ffe_preset(const u8 link_status[DP_LINK_STATUS_SIZE],
int lane);
-u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZE],
- unsigned int lane);
#define DP_BRANCH_OUI_HEADER_SIZE 0xc
#define DP_RECEIVER_CAP_SIZE 0xf
Or maybe do a long link status read in Tegra only?
--
Kees Cook
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] drm/dp: Actually read Adjust Request Post Cursor2 register
@ 2021-12-09 5:54 ` Kees Cook
0 siblings, 0 replies; 16+ messages in thread
From: Kees Cook @ 2021-12-09 5:54 UTC (permalink / raw)
To: Jani Nikula
Cc: David Airlie, linux-kernel, dri-devel, linux-hardening,
Thomas Zimmermann, Thierry Reding
On Wed, Dec 08, 2021 at 01:19:28PM +0200, Jani Nikula wrote:
> On Fri, 03 Dec 2021, Kees Cook <keescook@chromium.org> wrote:
> > The link_status array was not large enough to read the Adjust Request
> > Post Cursor2 register. Adjust the size to include it. Found with a
> > -Warray-bounds build:
> >
> > drivers/gpu/drm/drm_dp_helper.c: In function 'drm_dp_get_adjust_request_post_cursor':
> > drivers/gpu/drm/drm_dp_helper.c:59:27: error: array subscript 10 is outside array bounds of 'const u8[6]' {aka 'const unsigned char[6]'} [-Werror=array-bounds]
> > 59 | return link_status[r - DP_LANE0_1_STATUS];
> > | ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
> > drivers/gpu/drm/drm_dp_helper.c:147:51: note: while referencing 'link_status'
> > 147 | u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZE],
> > | ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > Fixes: 79465e0ffeb9 ("drm/dp: Add helper to get post-cursor adjustments")
> > Signed-off-by: Kees Cook <keescook@chromium.org>
>
> Using DP_ADJUST_REQUEST_POST_CURSOR2 has been deprecated since DP 1.3
> published in 2014, and Tegra is the only user of
> drm_dp_get_adjust_request_post_cursor().
>
> Instead of bumping the link status read size from 6 to 11 for all
> drivers I'd much rather see some other (maybe Tegra specific) solution
> to this.
Hmmm... Well given this is currently non-functional on Tegra (and is an
OOB memory read), how about just removing it entirely?
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 23f9073bc473..c9528aa62c9c 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -144,16 +144,6 @@ u8 drm_dp_get_adjust_tx_ffe_preset(const u8 link_status[DP_LINK_STATUS_SIZE],
}
EXPORT_SYMBOL(drm_dp_get_adjust_tx_ffe_preset);
-u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZE],
- unsigned int lane)
-{
- unsigned int offset = DP_ADJUST_REQUEST_POST_CURSOR2;
- u8 value = dp_link_status(link_status, offset);
-
- return (value >> (lane << 1)) & 0x3;
-}
-EXPORT_SYMBOL(drm_dp_get_adjust_request_post_cursor);
-
static int __8b10b_clock_recovery_delay_us(const struct drm_dp_aux *aux, u8 rd_interval)
{
if (rd_interval > 4)
diff --git a/drivers/gpu/drm/tegra/dp.c b/drivers/gpu/drm/tegra/dp.c
index 70dfb7d1dec5..bb5bfa93950f 100644
--- a/drivers/gpu/drm/tegra/dp.c
+++ b/drivers/gpu/drm/tegra/dp.c
@@ -559,8 +559,7 @@ static void drm_dp_link_get_adjustments(struct drm_dp_link *link,
drm_dp_get_adjust_request_pre_emphasis(status, i) >>
DP_TRAIN_PRE_EMPHASIS_SHIFT;
- adjust->post_cursor[i] =
- drm_dp_get_adjust_request_post_cursor(status, i);
+ adjust->post_cursor[i] = 0;
}
}
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 30359e434c3f..28378db676c8 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -1528,8 +1528,6 @@ u8 drm_dp_get_adjust_request_pre_emphasis(const u8 link_status[DP_LINK_STATUS_SI
int lane);
u8 drm_dp_get_adjust_tx_ffe_preset(const u8 link_status[DP_LINK_STATUS_SIZE],
int lane);
-u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZE],
- unsigned int lane);
#define DP_BRANCH_OUI_HEADER_SIZE 0xc
#define DP_RECEIVER_CAP_SIZE 0xf
Or maybe do a long link status read in Tegra only?
--
Kees Cook
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] drm/dp: Actually read Adjust Request Post Cursor2 register
2021-12-08 11:19 ` Jani Nikula
@ 2021-12-09 6:23 ` Kees Cook
-1 siblings, 0 replies; 16+ messages in thread
From: Kees Cook @ 2021-12-09 6:23 UTC (permalink / raw)
To: Jani Nikula
Cc: Maarten Lankhorst, David Airlie, linux-kernel, linux-hardening,
dri-devel, Thomas Zimmermann, Thierry Reding
On Wed, Dec 08, 2021 at 01:19:28PM +0200, Jani Nikula wrote:
> On Fri, 03 Dec 2021, Kees Cook <keescook@chromium.org> wrote:
> > The link_status array was not large enough to read the Adjust Request
> > Post Cursor2 register. Adjust the size to include it. Found with a
> > -Warray-bounds build:
> >
> > drivers/gpu/drm/drm_dp_helper.c: In function 'drm_dp_get_adjust_request_post_cursor':
> > drivers/gpu/drm/drm_dp_helper.c:59:27: error: array subscript 10 is outside array bounds of 'const u8[6]' {aka 'const unsigned char[6]'} [-Werror=array-bounds]
> > 59 | return link_status[r - DP_LANE0_1_STATUS];
> > | ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
> > drivers/gpu/drm/drm_dp_helper.c:147:51: note: while referencing 'link_status'
> > 147 | u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZE],
> > | ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > Fixes: 79465e0ffeb9 ("drm/dp: Add helper to get post-cursor adjustments")
> > Signed-off-by: Kees Cook <keescook@chromium.org>
>
> Using DP_ADJUST_REQUEST_POST_CURSOR2 has been deprecated since DP 1.3
> published in 2014, and Tegra is the only user of
> drm_dp_get_adjust_request_post_cursor().
I see POST_CURSOR2 is used here too:
drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
Here's a version of that for tegra (untested):
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 23f9073bc473..c9528aa62c9c 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -144,16 +144,6 @@ u8 drm_dp_get_adjust_tx_ffe_preset(const u8 link_status[DP_LINK_STATUS_SIZE],
}
EXPORT_SYMBOL(drm_dp_get_adjust_tx_ffe_preset);
-u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZE],
- unsigned int lane)
-{
- unsigned int offset = DP_ADJUST_REQUEST_POST_CURSOR2;
- u8 value = dp_link_status(link_status, offset);
-
- return (value >> (lane << 1)) & 0x3;
-}
-EXPORT_SYMBOL(drm_dp_get_adjust_request_post_cursor);
-
static int __8b10b_clock_recovery_delay_us(const struct drm_dp_aux *aux, u8 rd_interval)
{
if (rd_interval > 4)
diff --git a/drivers/gpu/drm/tegra/dp.c b/drivers/gpu/drm/tegra/dp.c
index 70dfb7d1dec5..f5535eb04c6b 100644
--- a/drivers/gpu/drm/tegra/dp.c
+++ b/drivers/gpu/drm/tegra/dp.c
@@ -549,6 +549,15 @@ static void drm_dp_link_get_adjustments(struct drm_dp_link *link,
{
struct drm_dp_link_train_set *adjust = &link->train.adjust;
unsigned int i;
+ u8 post_cursor;
+ int err;
+
+ err = drm_dp_dpcd_read(link->aux, DP_ADJUST_REQUEST_POST_CURSOR2,
+ &post_cursor, sizeof(post_cursor));
+ if (err < 0) {
+ DRM_ERROR("failed to read post_cursor2: %d\n", err);
+ post_cursor = 0;
+ }
for (i = 0; i < link->lanes; i++) {
adjust->voltage_swing[i] =
@@ -560,7 +569,7 @@ static void drm_dp_link_get_adjustments(struct drm_dp_link *link,
DP_TRAIN_PRE_EMPHASIS_SHIFT;
adjust->post_cursor[i] =
- drm_dp_get_adjust_request_post_cursor(status, i);
+ (post_cursor >> (i << 1)) & 0x3;
}
}
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 30359e434c3f..28378db676c8 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -1528,8 +1528,6 @@ u8 drm_dp_get_adjust_request_pre_emphasis(const u8 link_status[DP_LINK_STATUS_SI
int lane);
u8 drm_dp_get_adjust_tx_ffe_preset(const u8 link_status[DP_LINK_STATUS_SIZE],
int lane);
-u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZE],
- unsigned int lane);
#define DP_BRANCH_OUI_HEADER_SIZE 0xc
#define DP_RECEIVER_CAP_SIZE 0xf
Is that the right way to go?
--
Kees Cook
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] drm/dp: Actually read Adjust Request Post Cursor2 register
@ 2021-12-09 6:23 ` Kees Cook
0 siblings, 0 replies; 16+ messages in thread
From: Kees Cook @ 2021-12-09 6:23 UTC (permalink / raw)
To: Jani Nikula
Cc: David Airlie, linux-kernel, dri-devel, linux-hardening,
Thomas Zimmermann, Thierry Reding
On Wed, Dec 08, 2021 at 01:19:28PM +0200, Jani Nikula wrote:
> On Fri, 03 Dec 2021, Kees Cook <keescook@chromium.org> wrote:
> > The link_status array was not large enough to read the Adjust Request
> > Post Cursor2 register. Adjust the size to include it. Found with a
> > -Warray-bounds build:
> >
> > drivers/gpu/drm/drm_dp_helper.c: In function 'drm_dp_get_adjust_request_post_cursor':
> > drivers/gpu/drm/drm_dp_helper.c:59:27: error: array subscript 10 is outside array bounds of 'const u8[6]' {aka 'const unsigned char[6]'} [-Werror=array-bounds]
> > 59 | return link_status[r - DP_LANE0_1_STATUS];
> > | ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
> > drivers/gpu/drm/drm_dp_helper.c:147:51: note: while referencing 'link_status'
> > 147 | u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZE],
> > | ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > Fixes: 79465e0ffeb9 ("drm/dp: Add helper to get post-cursor adjustments")
> > Signed-off-by: Kees Cook <keescook@chromium.org>
>
> Using DP_ADJUST_REQUEST_POST_CURSOR2 has been deprecated since DP 1.3
> published in 2014, and Tegra is the only user of
> drm_dp_get_adjust_request_post_cursor().
I see POST_CURSOR2 is used here too:
drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
Here's a version of that for tegra (untested):
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 23f9073bc473..c9528aa62c9c 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -144,16 +144,6 @@ u8 drm_dp_get_adjust_tx_ffe_preset(const u8 link_status[DP_LINK_STATUS_SIZE],
}
EXPORT_SYMBOL(drm_dp_get_adjust_tx_ffe_preset);
-u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZE],
- unsigned int lane)
-{
- unsigned int offset = DP_ADJUST_REQUEST_POST_CURSOR2;
- u8 value = dp_link_status(link_status, offset);
-
- return (value >> (lane << 1)) & 0x3;
-}
-EXPORT_SYMBOL(drm_dp_get_adjust_request_post_cursor);
-
static int __8b10b_clock_recovery_delay_us(const struct drm_dp_aux *aux, u8 rd_interval)
{
if (rd_interval > 4)
diff --git a/drivers/gpu/drm/tegra/dp.c b/drivers/gpu/drm/tegra/dp.c
index 70dfb7d1dec5..f5535eb04c6b 100644
--- a/drivers/gpu/drm/tegra/dp.c
+++ b/drivers/gpu/drm/tegra/dp.c
@@ -549,6 +549,15 @@ static void drm_dp_link_get_adjustments(struct drm_dp_link *link,
{
struct drm_dp_link_train_set *adjust = &link->train.adjust;
unsigned int i;
+ u8 post_cursor;
+ int err;
+
+ err = drm_dp_dpcd_read(link->aux, DP_ADJUST_REQUEST_POST_CURSOR2,
+ &post_cursor, sizeof(post_cursor));
+ if (err < 0) {
+ DRM_ERROR("failed to read post_cursor2: %d\n", err);
+ post_cursor = 0;
+ }
for (i = 0; i < link->lanes; i++) {
adjust->voltage_swing[i] =
@@ -560,7 +569,7 @@ static void drm_dp_link_get_adjustments(struct drm_dp_link *link,
DP_TRAIN_PRE_EMPHASIS_SHIFT;
adjust->post_cursor[i] =
- drm_dp_get_adjust_request_post_cursor(status, i);
+ (post_cursor >> (i << 1)) & 0x3;
}
}
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 30359e434c3f..28378db676c8 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -1528,8 +1528,6 @@ u8 drm_dp_get_adjust_request_pre_emphasis(const u8 link_status[DP_LINK_STATUS_SI
int lane);
u8 drm_dp_get_adjust_tx_ffe_preset(const u8 link_status[DP_LINK_STATUS_SIZE],
int lane);
-u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZE],
- unsigned int lane);
#define DP_BRANCH_OUI_HEADER_SIZE 0xc
#define DP_RECEIVER_CAP_SIZE 0xf
Is that the right way to go?
--
Kees Cook
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] drm/dp: Actually read Adjust Request Post Cursor2 register
2021-12-09 6:23 ` Kees Cook
(?)
@ 2021-12-09 22:20 ` Harry Wentland
2021-12-09 23:41 ` Kees Cook
-1 siblings, 1 reply; 16+ messages in thread
From: Harry Wentland @ 2021-12-09 22:20 UTC (permalink / raw)
To: Kees Cook, Jani Nikula
Cc: David Airlie, linux-kernel, dri-devel, linux-hardening,
Thomas Zimmermann, Thierry Reding
On 2021-12-09 01:23, Kees Cook wrote:
> On Wed, Dec 08, 2021 at 01:19:28PM +0200, Jani Nikula wrote:
>> On Fri, 03 Dec 2021, Kees Cook <keescook@chromium.org> wrote:
>>> The link_status array was not large enough to read the Adjust Request
>>> Post Cursor2 register. Adjust the size to include it. Found with a
>>> -Warray-bounds build:
>>>
>>> drivers/gpu/drm/drm_dp_helper.c: In function 'drm_dp_get_adjust_request_post_cursor':
>>> drivers/gpu/drm/drm_dp_helper.c:59:27: error: array subscript 10 is outside array bounds of 'const u8[6]' {aka 'const unsigned char[6]'} [-Werror=array-bounds]
>>> 59 | return link_status[r - DP_LANE0_1_STATUS];
>>> | ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
>>> drivers/gpu/drm/drm_dp_helper.c:147:51: note: while referencing 'link_status'
>>> 147 | u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZE],
>>> | ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>
>>> Fixes: 79465e0ffeb9 ("drm/dp: Add helper to get post-cursor adjustments")
>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>
>> Using DP_ADJUST_REQUEST_POST_CURSOR2 has been deprecated since DP 1.3
>> published in 2014, and Tegra is the only user of
>> drm_dp_get_adjust_request_post_cursor().
>
> I see POST_CURSOR2 is used here too:
>
> drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
>
Looks like we read and parse that in the admgpu driver without
using drm_dp_get_adjust_request_post_cursor.
I don't have a strong feeling but I liked your original
patch a bit better. I'm not sure what it means when part
of a spec is deprecated. Once a spec is written display
vendors might implement it. We should make sure that
displays like that are always handled in a sane manner.
Harry
> Here's a version of that for tegra (untested):
>
>
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 23f9073bc473..c9528aa62c9c 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -144,16 +144,6 @@ u8 drm_dp_get_adjust_tx_ffe_preset(const u8 link_status[DP_LINK_STATUS_SIZE],
> }
> EXPORT_SYMBOL(drm_dp_get_adjust_tx_ffe_preset);
>
> -u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZE],
> - unsigned int lane)
> -{
> - unsigned int offset = DP_ADJUST_REQUEST_POST_CURSOR2;
> - u8 value = dp_link_status(link_status, offset);
> -
> - return (value >> (lane << 1)) & 0x3;
> -}
> -EXPORT_SYMBOL(drm_dp_get_adjust_request_post_cursor);
> -
> static int __8b10b_clock_recovery_delay_us(const struct drm_dp_aux *aux, u8 rd_interval)
> {
> if (rd_interval > 4)
> diff --git a/drivers/gpu/drm/tegra/dp.c b/drivers/gpu/drm/tegra/dp.c
> index 70dfb7d1dec5..f5535eb04c6b 100644
> --- a/drivers/gpu/drm/tegra/dp.c
> +++ b/drivers/gpu/drm/tegra/dp.c
> @@ -549,6 +549,15 @@ static void drm_dp_link_get_adjustments(struct drm_dp_link *link,
> {
> struct drm_dp_link_train_set *adjust = &link->train.adjust;
> unsigned int i;
> + u8 post_cursor;
> + int err;
> +
> + err = drm_dp_dpcd_read(link->aux, DP_ADJUST_REQUEST_POST_CURSOR2,
> + &post_cursor, sizeof(post_cursor));
> + if (err < 0) {
> + DRM_ERROR("failed to read post_cursor2: %d\n", err);
> + post_cursor = 0;
> + }
>
> for (i = 0; i < link->lanes; i++) {
> adjust->voltage_swing[i] =
> @@ -560,7 +569,7 @@ static void drm_dp_link_get_adjustments(struct drm_dp_link *link,
> DP_TRAIN_PRE_EMPHASIS_SHIFT;
>
> adjust->post_cursor[i] =
> - drm_dp_get_adjust_request_post_cursor(status, i);
> + (post_cursor >> (i << 1)) & 0x3;
> }
> }
>
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 30359e434c3f..28378db676c8 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -1528,8 +1528,6 @@ u8 drm_dp_get_adjust_request_pre_emphasis(const u8 link_status[DP_LINK_STATUS_SI
> int lane);
> u8 drm_dp_get_adjust_tx_ffe_preset(const u8 link_status[DP_LINK_STATUS_SIZE],
> int lane);
> -u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZE],
> - unsigned int lane);
>
> #define DP_BRANCH_OUI_HEADER_SIZE 0xc
> #define DP_RECEIVER_CAP_SIZE 0xf
>
>
> Is that the right way to go?
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] drm/dp: Actually read Adjust Request Post Cursor2 register
2021-12-09 22:20 ` Harry Wentland
@ 2021-12-09 23:41 ` Kees Cook
0 siblings, 0 replies; 16+ messages in thread
From: Kees Cook @ 2021-12-09 23:41 UTC (permalink / raw)
To: Harry Wentland
Cc: Jani Nikula, David Airlie, linux-kernel, dri-devel,
linux-hardening, Thomas Zimmermann, Thierry Reding
On Thu, Dec 09, 2021 at 05:20:45PM -0500, Harry Wentland wrote:
>
>
> On 2021-12-09 01:23, Kees Cook wrote:
> > On Wed, Dec 08, 2021 at 01:19:28PM +0200, Jani Nikula wrote:
> >> On Fri, 03 Dec 2021, Kees Cook <keescook@chromium.org> wrote:
> >>> The link_status array was not large enough to read the Adjust Request
> >>> Post Cursor2 register. Adjust the size to include it. Found with a
> >>> -Warray-bounds build:
> >>>
> >>> drivers/gpu/drm/drm_dp_helper.c: In function 'drm_dp_get_adjust_request_post_cursor':
> >>> drivers/gpu/drm/drm_dp_helper.c:59:27: error: array subscript 10 is outside array bounds of 'const u8[6]' {aka 'const unsigned char[6]'} [-Werror=array-bounds]
> >>> 59 | return link_status[r - DP_LANE0_1_STATUS];
> >>> | ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
> >>> drivers/gpu/drm/drm_dp_helper.c:147:51: note: while referencing 'link_status'
> >>> 147 | u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZE],
> >>> | ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>>
> >>> Fixes: 79465e0ffeb9 ("drm/dp: Add helper to get post-cursor adjustments")
> >>> Signed-off-by: Kees Cook <keescook@chromium.org>
> >>
> >> Using DP_ADJUST_REQUEST_POST_CURSOR2 has been deprecated since DP 1.3
> >> published in 2014, and Tegra is the only user of
> >> drm_dp_get_adjust_request_post_cursor().
> >
> > I see POST_CURSOR2 is used here too:
> >
> > drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
> >
>
> Looks like we read and parse that in the admgpu driver without
> using drm_dp_get_adjust_request_post_cursor.
Right, and probably that could be switched to use it, but I'm not sure
what the impact of the larger link_status read is.
>
> I don't have a strong feeling but I liked your original
> patch a bit better. I'm not sure what it means when part
> of a spec is deprecated. Once a spec is written display
> vendors might implement it. We should make sure that
> displays like that are always handled in a sane manner.
Jani, Dave, any guidance here? I'm fine with whatever, but the current
code is for sure broken. ;)
-Kees
--
Kees Cook
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] drm/dp: Actually read Adjust Request Post Cursor2 register
@ 2021-12-09 23:41 ` Kees Cook
0 siblings, 0 replies; 16+ messages in thread
From: Kees Cook @ 2021-12-09 23:41 UTC (permalink / raw)
To: Harry Wentland
Cc: David Airlie, linux-kernel, dri-devel, Thomas Zimmermann,
Thierry Reding, linux-hardening
On Thu, Dec 09, 2021 at 05:20:45PM -0500, Harry Wentland wrote:
>
>
> On 2021-12-09 01:23, Kees Cook wrote:
> > On Wed, Dec 08, 2021 at 01:19:28PM +0200, Jani Nikula wrote:
> >> On Fri, 03 Dec 2021, Kees Cook <keescook@chromium.org> wrote:
> >>> The link_status array was not large enough to read the Adjust Request
> >>> Post Cursor2 register. Adjust the size to include it. Found with a
> >>> -Warray-bounds build:
> >>>
> >>> drivers/gpu/drm/drm_dp_helper.c: In function 'drm_dp_get_adjust_request_post_cursor':
> >>> drivers/gpu/drm/drm_dp_helper.c:59:27: error: array subscript 10 is outside array bounds of 'const u8[6]' {aka 'const unsigned char[6]'} [-Werror=array-bounds]
> >>> 59 | return link_status[r - DP_LANE0_1_STATUS];
> >>> | ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
> >>> drivers/gpu/drm/drm_dp_helper.c:147:51: note: while referencing 'link_status'
> >>> 147 | u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZE],
> >>> | ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>>
> >>> Fixes: 79465e0ffeb9 ("drm/dp: Add helper to get post-cursor adjustments")
> >>> Signed-off-by: Kees Cook <keescook@chromium.org>
> >>
> >> Using DP_ADJUST_REQUEST_POST_CURSOR2 has been deprecated since DP 1.3
> >> published in 2014, and Tegra is the only user of
> >> drm_dp_get_adjust_request_post_cursor().
> >
> > I see POST_CURSOR2 is used here too:
> >
> > drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
> >
>
> Looks like we read and parse that in the admgpu driver without
> using drm_dp_get_adjust_request_post_cursor.
Right, and probably that could be switched to use it, but I'm not sure
what the impact of the larger link_status read is.
>
> I don't have a strong feeling but I liked your original
> patch a bit better. I'm not sure what it means when part
> of a spec is deprecated. Once a spec is written display
> vendors might implement it. We should make sure that
> displays like that are always handled in a sane manner.
Jani, Dave, any guidance here? I'm fine with whatever, but the current
code is for sure broken. ;)
-Kees
--
Kees Cook
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] drm/dp: Actually read Adjust Request Post Cursor2 register
2021-12-09 23:41 ` Kees Cook
(?)
@ 2021-12-10 10:06 ` Jani Nikula
2021-12-10 16:53 ` Kees Cook
-1 siblings, 1 reply; 16+ messages in thread
From: Jani Nikula @ 2021-12-10 10:06 UTC (permalink / raw)
To: Kees Cook, Harry Wentland
Cc: David Airlie, linux-kernel, dri-devel, linux-hardening,
Thomas Zimmermann, Thierry Reding
On Thu, 09 Dec 2021, Kees Cook <keescook@chromium.org> wrote:
> On Thu, Dec 09, 2021 at 05:20:45PM -0500, Harry Wentland wrote:
>>
>>
>> On 2021-12-09 01:23, Kees Cook wrote:
>> > On Wed, Dec 08, 2021 at 01:19:28PM +0200, Jani Nikula wrote:
>> >> On Fri, 03 Dec 2021, Kees Cook <keescook@chromium.org> wrote:
>> >>> The link_status array was not large enough to read the Adjust Request
>> >>> Post Cursor2 register. Adjust the size to include it. Found with a
>> >>> -Warray-bounds build:
>> >>>
>> >>> drivers/gpu/drm/drm_dp_helper.c: In function 'drm_dp_get_adjust_request_post_cursor':
>> >>> drivers/gpu/drm/drm_dp_helper.c:59:27: error: array subscript 10 is outside array bounds of 'const u8[6]' {aka 'const unsigned char[6]'} [-Werror=array-bounds]
>> >>> 59 | return link_status[r - DP_LANE0_1_STATUS];
>> >>> | ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
>> >>> drivers/gpu/drm/drm_dp_helper.c:147:51: note: while referencing 'link_status'
>> >>> 147 | u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZE],
>> >>> | ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> >>>
>> >>> Fixes: 79465e0ffeb9 ("drm/dp: Add helper to get post-cursor adjustments")
>> >>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> >>
>> >> Using DP_ADJUST_REQUEST_POST_CURSOR2 has been deprecated since DP 1.3
>> >> published in 2014, and Tegra is the only user of
>> >> drm_dp_get_adjust_request_post_cursor().
>> >
>> > I see POST_CURSOR2 is used here too:
>> >
>> > drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
>> >
>>
>> Looks like we read and parse that in the admgpu driver without
>> using drm_dp_get_adjust_request_post_cursor.
>
> Right, and probably that could be switched to use it, but I'm not sure
> what the impact of the larger link_status read is.
>
>>
>> I don't have a strong feeling but I liked your original
>> patch a bit better. I'm not sure what it means when part
>> of a spec is deprecated. Once a spec is written display
>> vendors might implement it. We should make sure that
>> displays like that are always handled in a sane manner.
>
> Jani, Dave, any guidance here? I'm fine with whatever, but the current
> code is for sure broken. ;)
Post Cursor2 was completely optional for the transmitter even before it
was deprecated.
And now we'd be adding 5 bytes extra to all link status reads. To fix
the only user of drm_dp_get_adjust_request_post_cursor() that apparently
has never worked as intended. I'm just not convinced.
I was trying to look through the implications of DP_LINK_STATUS_SIZE
increase, and at least drm_dp_dpcd_read_phy_link_status() comes across
as something probably needing attention.
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] drm/dp: Actually read Adjust Request Post Cursor2 register
2021-12-10 10:06 ` Jani Nikula
@ 2021-12-10 16:53 ` Kees Cook
0 siblings, 0 replies; 16+ messages in thread
From: Kees Cook @ 2021-12-10 16:53 UTC (permalink / raw)
To: Jani Nikula
Cc: Harry Wentland, David Airlie, linux-kernel, dri-devel,
linux-hardening, Thomas Zimmermann, Thierry Reding
On Fri, Dec 10, 2021 at 12:06:20PM +0200, Jani Nikula wrote:
> On Thu, 09 Dec 2021, Kees Cook <keescook@chromium.org> wrote:
> > On Thu, Dec 09, 2021 at 05:20:45PM -0500, Harry Wentland wrote:
> >>
> >>
> >> On 2021-12-09 01:23, Kees Cook wrote:
> >> > On Wed, Dec 08, 2021 at 01:19:28PM +0200, Jani Nikula wrote:
> >> >> On Fri, 03 Dec 2021, Kees Cook <keescook@chromium.org> wrote:
> >> >>> The link_status array was not large enough to read the Adjust Request
> >> >>> Post Cursor2 register. Adjust the size to include it. Found with a
> >> >>> -Warray-bounds build:
> >> >>>
> >> >>> drivers/gpu/drm/drm_dp_helper.c: In function 'drm_dp_get_adjust_request_post_cursor':
> >> >>> drivers/gpu/drm/drm_dp_helper.c:59:27: error: array subscript 10 is outside array bounds of 'const u8[6]' {aka 'const unsigned char[6]'} [-Werror=array-bounds]
> >> >>> 59 | return link_status[r - DP_LANE0_1_STATUS];
> >> >>> | ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
> >> >>> drivers/gpu/drm/drm_dp_helper.c:147:51: note: while referencing 'link_status'
> >> >>> 147 | u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZE],
> >> >>> | ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >> >>>
> >> >>> Fixes: 79465e0ffeb9 ("drm/dp: Add helper to get post-cursor adjustments")
> >> >>> Signed-off-by: Kees Cook <keescook@chromium.org>
> >> >>
> >> >> Using DP_ADJUST_REQUEST_POST_CURSOR2 has been deprecated since DP 1.3
> >> >> published in 2014, and Tegra is the only user of
> >> >> drm_dp_get_adjust_request_post_cursor().
> >> >
> >> > I see POST_CURSOR2 is used here too:
> >> >
> >> > drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
> >> >
> >>
> >> Looks like we read and parse that in the admgpu driver without
> >> using drm_dp_get_adjust_request_post_cursor.
> >
> > Right, and probably that could be switched to use it, but I'm not sure
> > what the impact of the larger link_status read is.
> >
> >>
> >> I don't have a strong feeling but I liked your original
> >> patch a bit better. I'm not sure what it means when part
> >> of a spec is deprecated. Once a spec is written display
> >> vendors might implement it. We should make sure that
> >> displays like that are always handled in a sane manner.
> >
> > Jani, Dave, any guidance here? I'm fine with whatever, but the current
> > code is for sure broken. ;)
>
> Post Cursor2 was completely optional for the transmitter even before it
> was deprecated.
>
> And now we'd be adding 5 bytes extra to all link status reads. To fix
> the only user of drm_dp_get_adjust_request_post_cursor() that apparently
> has never worked as intended. I'm just not convinced.
>
> I was trying to look through the implications of DP_LINK_STATUS_SIZE
> increase, and at least drm_dp_dpcd_read_phy_link_status() comes across
> as something probably needing attention.
Okay, it sounds like you'd prefer the "make it tegra-specific" patch I
proposed. I will work that up as a proper patch and send it.
Thanks!
--
Kees Cook
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] drm/dp: Actually read Adjust Request Post Cursor2 register
@ 2021-12-10 16:53 ` Kees Cook
0 siblings, 0 replies; 16+ messages in thread
From: Kees Cook @ 2021-12-10 16:53 UTC (permalink / raw)
To: Jani Nikula
Cc: David Airlie, linux-kernel, dri-devel, linux-hardening,
Thomas Zimmermann, Thierry Reding
On Fri, Dec 10, 2021 at 12:06:20PM +0200, Jani Nikula wrote:
> On Thu, 09 Dec 2021, Kees Cook <keescook@chromium.org> wrote:
> > On Thu, Dec 09, 2021 at 05:20:45PM -0500, Harry Wentland wrote:
> >>
> >>
> >> On 2021-12-09 01:23, Kees Cook wrote:
> >> > On Wed, Dec 08, 2021 at 01:19:28PM +0200, Jani Nikula wrote:
> >> >> On Fri, 03 Dec 2021, Kees Cook <keescook@chromium.org> wrote:
> >> >>> The link_status array was not large enough to read the Adjust Request
> >> >>> Post Cursor2 register. Adjust the size to include it. Found with a
> >> >>> -Warray-bounds build:
> >> >>>
> >> >>> drivers/gpu/drm/drm_dp_helper.c: In function 'drm_dp_get_adjust_request_post_cursor':
> >> >>> drivers/gpu/drm/drm_dp_helper.c:59:27: error: array subscript 10 is outside array bounds of 'const u8[6]' {aka 'const unsigned char[6]'} [-Werror=array-bounds]
> >> >>> 59 | return link_status[r - DP_LANE0_1_STATUS];
> >> >>> | ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
> >> >>> drivers/gpu/drm/drm_dp_helper.c:147:51: note: while referencing 'link_status'
> >> >>> 147 | u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZE],
> >> >>> | ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >> >>>
> >> >>> Fixes: 79465e0ffeb9 ("drm/dp: Add helper to get post-cursor adjustments")
> >> >>> Signed-off-by: Kees Cook <keescook@chromium.org>
> >> >>
> >> >> Using DP_ADJUST_REQUEST_POST_CURSOR2 has been deprecated since DP 1.3
> >> >> published in 2014, and Tegra is the only user of
> >> >> drm_dp_get_adjust_request_post_cursor().
> >> >
> >> > I see POST_CURSOR2 is used here too:
> >> >
> >> > drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
> >> >
> >>
> >> Looks like we read and parse that in the admgpu driver without
> >> using drm_dp_get_adjust_request_post_cursor.
> >
> > Right, and probably that could be switched to use it, but I'm not sure
> > what the impact of the larger link_status read is.
> >
> >>
> >> I don't have a strong feeling but I liked your original
> >> patch a bit better. I'm not sure what it means when part
> >> of a spec is deprecated. Once a spec is written display
> >> vendors might implement it. We should make sure that
> >> displays like that are always handled in a sane manner.
> >
> > Jani, Dave, any guidance here? I'm fine with whatever, but the current
> > code is for sure broken. ;)
>
> Post Cursor2 was completely optional for the transmitter even before it
> was deprecated.
>
> And now we'd be adding 5 bytes extra to all link status reads. To fix
> the only user of drm_dp_get_adjust_request_post_cursor() that apparently
> has never worked as intended. I'm just not convinced.
>
> I was trying to look through the implications of DP_LINK_STATUS_SIZE
> increase, and at least drm_dp_dpcd_read_phy_link_status() comes across
> as something probably needing attention.
Okay, it sounds like you'd prefer the "make it tegra-specific" patch I
proposed. I will work that up as a proper patch and send it.
Thanks!
--
Kees Cook
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] drm/dp: Actually read Adjust Request Post Cursor2 register
2021-12-10 16:53 ` Kees Cook
@ 2021-12-13 9:34 ` Jani Nikula
-1 siblings, 0 replies; 16+ messages in thread
From: Jani Nikula @ 2021-12-13 9:34 UTC (permalink / raw)
To: Kees Cook
Cc: David Airlie, linux-kernel, dri-devel, linux-hardening,
Thomas Zimmermann, Thierry Reding
On Fri, 10 Dec 2021, Kees Cook <keescook@chromium.org> wrote:
> On Fri, Dec 10, 2021 at 12:06:20PM +0200, Jani Nikula wrote:
>> Post Cursor2 was completely optional for the transmitter even before it
>> was deprecated.
>>
>> And now we'd be adding 5 bytes extra to all link status reads. To fix
>> the only user of drm_dp_get_adjust_request_post_cursor() that apparently
>> has never worked as intended. I'm just not convinced.
>>
>> I was trying to look through the implications of DP_LINK_STATUS_SIZE
>> increase, and at least drm_dp_dpcd_read_phy_link_status() comes across
>> as something probably needing attention.
>
> Okay, it sounds like you'd prefer the "make it tegra-specific" patch I
> proposed. I will work that up as a proper patch and send it.
Yes. Of course, I'd like to hear Thierry's view as well.
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] drm/dp: Actually read Adjust Request Post Cursor2 register
@ 2021-12-13 9:34 ` Jani Nikula
0 siblings, 0 replies; 16+ messages in thread
From: Jani Nikula @ 2021-12-13 9:34 UTC (permalink / raw)
To: Kees Cook
Cc: Harry Wentland, David Airlie, linux-kernel, dri-devel,
linux-hardening, Thomas Zimmermann, Thierry Reding
On Fri, 10 Dec 2021, Kees Cook <keescook@chromium.org> wrote:
> On Fri, Dec 10, 2021 at 12:06:20PM +0200, Jani Nikula wrote:
>> Post Cursor2 was completely optional for the transmitter even before it
>> was deprecated.
>>
>> And now we'd be adding 5 bytes extra to all link status reads. To fix
>> the only user of drm_dp_get_adjust_request_post_cursor() that apparently
>> has never worked as intended. I'm just not convinced.
>>
>> I was trying to look through the implications of DP_LINK_STATUS_SIZE
>> increase, and at least drm_dp_dpcd_read_phy_link_status() comes across
>> as something probably needing attention.
>
> Okay, it sounds like you'd prefer the "make it tegra-specific" patch I
> proposed. I will work that up as a proper patch and send it.
Yes. Of course, I'd like to hear Thierry's view as well.
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2021-12-13 9:38 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-03 8:43 [PATCH] drm/dp: Actually read Adjust Request Post Cursor2 register Kees Cook
2021-12-03 8:43 ` Kees Cook
2021-12-08 11:19 ` Jani Nikula
2021-12-08 11:19 ` Jani Nikula
2021-12-09 5:54 ` Kees Cook
2021-12-09 5:54 ` Kees Cook
2021-12-09 6:23 ` Kees Cook
2021-12-09 6:23 ` Kees Cook
2021-12-09 22:20 ` Harry Wentland
2021-12-09 23:41 ` Kees Cook
2021-12-09 23:41 ` Kees Cook
2021-12-10 10:06 ` Jani Nikula
2021-12-10 16:53 ` Kees Cook
2021-12-10 16:53 ` Kees Cook
2021-12-13 9:34 ` Jani Nikula
2021-12-13 9:34 ` Jani Nikula
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.