Potential Bug in drm/amd/display/dc_link

Potential Bug in drm/amd/display/dc_link

[Yizhuo Zhai]

Hi All:
I just found a bug in the cramfs using the static analysis tool, but
not sure if this could happen in reality, could you please advise me
here? Thanks for your attention : ) And please ignore the last one
with HTML format if you did not filter it out.

In function enable_stream_features(), the variable
"old_downspread.raw" could be uninitialized if core_link_read_dpcd
fails(), however, it is used in the later if statement, and further,
core_link_write_dpcd() may write random value, which is potentially
unsafe. But this function does not return the error code to the up
caller and I got stuck in drafting the patch, could you please advise
me here?

The related code:
static void enable_stream_features(struct pipe_ctx *pipe_ctx)
{
     union down_spread_ctrl old_downspread;
    core_link_read_dpcd(link, DP_DOWNSPREAD_CTRL,
                         &old_downspread.raw, sizeof(old_downspread);

        //old_downspread.raw used here
        if (new_downspread.raw != old_downspread.raw) {
               core_link_write_dpcd(link, DP_DOWNSPREAD_CTRL,
                         &new_downspread.raw, sizeof(new_downspread));
        }
}
enum dc_status core_link_read_dpcd(
    struct dc_link *link,
    uint32_t address,
    uint8_t *data,
    uint32_t size)
{
        //data could be uninitialized if the helpers fails and log
some error info
        if (!dm_helpers_dp_read_dpcd(link->ctx,
               link,address, data, size))
                      return DC_ERROR_UNEXPECTED;
        return DC_OK;
}

The same issue in function wait_for_training_aux_rd_interval() in
drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
-- 
Kind Regards,

Yizhuo Zhai

Computer Science, Graduate Student
University of California, Riverside

Re: Potential Bug in drm/amd/display/dc_link

[Harry Wentland]

On 2021-12-09 03:02, Yizhuo Zhai wrote:
> Hi All:
> I just found a bug in the cramfs using the static analysis tool, but
> not sure if this could happen in reality, could you please advise me
> here? Thanks for your attention : ) And please ignore the last one
> with HTML format if you did not filter it out.
> 
> In function enable_stream_features(), the variable
> "old_downspread.raw" could be uninitialized if core_link_read_dpcd
> fails(), however, it is used in the later if statement, and further,
> core_link_write_dpcd() may write random value, which is potentially
> unsafe. But this function does not return the error code to the up
> caller and I got stuck in drafting the patch, could you please advise
> me here?
> 

Thanks for highlighting this.

Unfortunately we frequently ignore DPCD error codes.

In this case I would do a memset as shown below.

> The related code:
> static void enable_stream_features(struct pipe_ctx *pipe_ctx)
> {
>      union down_spread_ctrl old_downspread;

	memset(&old_downspread, 0, sizeof(old_downspread));

>     core_link_read_dpcd(link, DP_DOWNSPREAD_CTRL,
>                          &old_downspread.raw, sizeof(old_downspread);
> 
>         //old_downspread.raw used here
>         if (new_downspread.raw != old_downspread.raw) {
>                core_link_write_dpcd(link, DP_DOWNSPREAD_CTRL,
>                          &new_downspread.raw, sizeof(new_downspread));
>         }
> }
> enum dc_status core_link_read_dpcd(
>     struct dc_link *link,
>     uint32_t address,
>     uint8_t *data,
>     uint32_t size)
> {
>         //data could be uninitialized if the helpers fails and log
> some error info
>         if (!dm_helpers_dp_read_dpcd(link->ctx,
>                link,address, data, size))
>                       return DC_ERROR_UNEXPECTED;
>         return DC_OK;
> }
> 
> The same issue in function wait_for_training_aux_rd_interval() in
> drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c

I don't see this. Do you mean this one?

> void dp_wait_for_training_aux_rd_interval(
> 	struct dc_link *link,
> 	uint32_t wait_in_micro_secs)
> {
> #if defined(CONFIG_DRM_AMD_DC_DCN)
> 	if (wait_in_micro_secs > 16000)
> 		msleep(wait_in_micro_secs/1000);
> 	else
> 		udelay(wait_in_micro_secs);
> #else
> 	udelay(wait_in_micro_secs);
> #endif
> 
> 	DC_LOG_HW_LINK_TRAINING("%s:\n wait = %d\n",
> 		__func__,
> 		wait_in_micro_secs);
> }

Thanks,
Harry

> 

Re: Potential Bug in drm/amd/display/dc_link

[Yizhuo Zhai]

Hi Harry:
Thanks for your feedback, I will submit the patch for variable
"old_downspread" in the function enable_stream_features().
And I double checked the code in the mainline and found that the buggy
function wait_for_training_aux_rd_interval() has been removed, and the
corresponding bug has been fixed in v5.1-rc1 by a memset. Sorry for
the confusion.

On Thu, Dec 9, 2021 at 2:30 PM Harry Wentland <harry.wentland@amd.com> wrote:
>
>
> On 2021-12-09 03:02, Yizhuo Zhai wrote:
> > Hi All:
> > I just found a bug in the cramfs using the static analysis tool, but
> > not sure if this could happen in reality, could you please advise me
> > here? Thanks for your attention : ) And please ignore the last one
> > with HTML format if you did not filter it out.
> >
> > In function enable_stream_features(), the variable
> > "old_downspread.raw" could be uninitialized if core_link_read_dpcd
> > fails(), however, it is used in the later if statement, and further,
> > core_link_write_dpcd() may write random value, which is potentially
> > unsafe. But this function does not return the error code to the up
> > caller and I got stuck in drafting the patch, could you please advise
> > me here?
> >
>
> Thanks for highlighting this.
>
> Unfortunately we frequently ignore DPCD error codes.
>
> In this case I would do a memset as shown below.
>
> > The related code:
> > static void enable_stream_features(struct pipe_ctx *pipe_ctx)
> > {
> >      union down_spread_ctrl old_downspread;
>
>         memset(&old_downspread, 0, sizeof(old_downspread));
>
> >     core_link_read_dpcd(link, DP_DOWNSPREAD_CTRL,
> >                          &old_downspread.raw, sizeof(old_downspread);
> >
> >         //old_downspread.raw used here
> >         if (new_downspread.raw != old_downspread.raw) {
> >                core_link_write_dpcd(link, DP_DOWNSPREAD_CTRL,
> >                          &new_downspread.raw, sizeof(new_downspread));
> >         }
> > }
> > enum dc_status core_link_read_dpcd(
> >     struct dc_link *link,
> >     uint32_t address,
> >     uint8_t *data,
> >     uint32_t size)
> > {
> >         //data could be uninitialized if the helpers fails and log
> > some error info
> >         if (!dm_helpers_dp_read_dpcd(link->ctx,
> >                link,address, data, size))
> >                       return DC_ERROR_UNEXPECTED;
> >         return DC_OK;
> > }
> >
> > The same issue in function wait_for_training_aux_rd_interval() in
> > drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
>
> I don't see this. Do you mean this one?
>
> > void dp_wait_for_training_aux_rd_interval(
> >       struct dc_link *link,
> >       uint32_t wait_in_micro_secs)
> > {
> > #if defined(CONFIG_DRM_AMD_DC_DCN)
> >       if (wait_in_micro_secs > 16000)
> >               msleep(wait_in_micro_secs/1000);
> >       else
> >               udelay(wait_in_micro_secs);
> > #else
> >       udelay(wait_in_micro_secs);
> > #endif
> >
> >       DC_LOG_HW_LINK_TRAINING("%s:\n wait = %d\n",
> >               __func__,
> >               wait_in_micro_secs);
> > }
>
> Thanks,
> Harry
>
> >
>
>
>


-- 
Kind Regards,

Yizhuo Zhai

Computer Science, Graduate Student
University of California, Riverside

Re: Potential Bug in drm/amd/display/dc_link

[Yizhuo Zhai]

Hi Harry:
Thanks for your feedback, I will submit the patch for variable
"old_downspread" in the function enable_stream_features().
And I double checked the code in the mainline and found that the buggy
function wait_for_training_aux_rd_interval() has been removed, and the
corresponding bug has been fixed in v5.1-rc1 by a memset. Sorry for
the confusion.

On Thu, Dec 9, 2021 at 2:30 PM Harry Wentland <harry.wentland@amd.com> wrote:
>
>
> On 2021-12-09 03:02, Yizhuo Zhai wrote:
> > Hi All:
> > I just found a bug in the cramfs using the static analysis tool, but
> > not sure if this could happen in reality, could you please advise me
> > here? Thanks for your attention : ) And please ignore the last one
> > with HTML format if you did not filter it out.
> >
> > In function enable_stream_features(), the variable
> > "old_downspread.raw" could be uninitialized if core_link_read_dpcd
> > fails(), however, it is used in the later if statement, and further,
> > core_link_write_dpcd() may write random value, which is potentially
> > unsafe. But this function does not return the error code to the up
> > caller and I got stuck in drafting the patch, could you please advise
> > me here?
> >
>
> Thanks for highlighting this.
>
> Unfortunately we frequently ignore DPCD error codes.
>
> In this case I would do a memset as shown below.
>
> > The related code:
> > static void enable_stream_features(struct pipe_ctx *pipe_ctx)
> > {
> >      union down_spread_ctrl old_downspread;
>
>         memset(&old_downspread, 0, sizeof(old_downspread));
>
> >     core_link_read_dpcd(link, DP_DOWNSPREAD_CTRL,
> >                          &old_downspread.raw, sizeof(old_downspread);
> >
> >         //old_downspread.raw used here
> >         if (new_downspread.raw != old_downspread.raw) {
> >                core_link_write_dpcd(link, DP_DOWNSPREAD_CTRL,
> >                          &new_downspread.raw, sizeof(new_downspread));
> >         }
> > }
> > enum dc_status core_link_read_dpcd(
> >     struct dc_link *link,
> >     uint32_t address,
> >     uint8_t *data,
> >     uint32_t size)
> > {
> >         //data could be uninitialized if the helpers fails and log
> > some error info
> >         if (!dm_helpers_dp_read_dpcd(link->ctx,
> >                link,address, data, size))
> >                       return DC_ERROR_UNEXPECTED;
> >         return DC_OK;
> > }
> >
> > The same issue in function wait_for_training_aux_rd_interval() in
> > drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
>
> I don't see this. Do you mean this one?
>
> > void dp_wait_for_training_aux_rd_interval(
> >       struct dc_link *link,
> >       uint32_t wait_in_micro_secs)
> > {
> > #if defined(CONFIG_DRM_AMD_DC_DCN)
> >       if (wait_in_micro_secs > 16000)
> >               msleep(wait_in_micro_secs/1000);
> >       else
> >               udelay(wait_in_micro_secs);
> > #else
> >       udelay(wait_in_micro_secs);
> > #endif
> >
> >       DC_LOG_HW_LINK_TRAINING("%s:\n wait = %d\n",
> >               __func__,
> >               wait_in_micro_secs);
> > }
>
> Thanks,
> Harry
>
> >
>
>
>


-- 
Kind Regards,

Yizhuo Zhai

Computer Science, Graduate Student
University of California, Riverside