[bug report] drm/amd/display: Read AUX channel even if only status byte is returned

[Leo Li]

On 2018-07-31 02:24 PM, Dan Carpenter wrote:
> [ Potential security issue, if I'm reading the code correctly.  I don't
>    really know the code and I haven't looked at the larger context. -dan ]
> 
> Hello Leo (Sunpeng) Li,
> 
> The patch edf6ffe4f47e: "drm/amd/display: Read AUX channel even if
> only status byte is returned" from Jun 26, 2018, leads to the
> following static checker warning:
> 
> 	drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc_link_ddc.c:673 dc_link_aux_transfer()
> 	warn: 'returned_bytes' is unsigned
> 
> drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc_link_ddc.c
>     634  int dc_link_aux_transfer(struct ddc_service *ddc,
>     635                               unsigned int address,
>     636                               uint8_t *reply,
>     637                               void *buffer,
>     638                               unsigned int size,
>     639                               enum aux_transaction_type type,
>     640                               enum i2caux_transaction_action action)
>     641  {
>     642          struct ddc *ddc_pin = ddc->ddc_pin;
>     643          struct engine *engine;
>     644          struct aux_engine *aux_engine;
>     645          enum aux_channel_operation_result operation_result;
>     646          struct aux_request_transaction_data aux_req;
>     647          struct aux_reply_transaction_data aux_rep;
>     648          uint8_t returned_bytes = 0;
>                  ^^^^^^^^^^^^^^^^^^^^^^^^^^
> returned_bytes is a u8.
> 
>     649          int res = -1;
>     650          uint32_t status;
>     651
>     652          memset(&aux_req, 0, sizeof(aux_req));
>     653          memset(&aux_rep, 0, sizeof(aux_rep));
>     654
>     655          engine = ddc->ctx->dc->res_pool->engines[ddc_pin->pin_data->en];
>     656          aux_engine = engine->funcs->acquire(engine, ddc_pin);
>     657
>     658          aux_req.type = type;
>     659          aux_req.action = action;
>     660
>     661          aux_req.address = address;
>     662          aux_req.delay = 0;
>     663          aux_req.length = size;
>     664          aux_req.data = buffer;
>     665
>     666          aux_engine->funcs->submit_channel_request(aux_engine, &aux_req);
>     667          operation_result = aux_engine->funcs->get_channel_status(aux_engine, &returned_bytes);
>     668
>     669          switch (operation_result) {
>     670          case AUX_CHANNEL_OPERATION_SUCCEEDED:
>     671                  res = returned_bytes;
>                          ^^^^^^^^^^^^^^^^^^^^
> res = 0-255
> 
>     672
>     673                  if (res <= size && res >= 0)
>                              ^^^^^^^^^^^^^^^^^^^^^^^
> So obviously the res >= 0 check can be removed, but that's harmless.
> The bigger problem is that the other test looks to be reversed.  Instead
> of <= it should be >= size.  Otherwise we are reading beyond the end of
> the returned bytes.

Right, the >= 0 is pointless. And upon closer look at the context, the
<= size check also seems to be pointless.

`size` refers to the size of the buffer we're given to save the reply
in. So although the `res <= size` check seems correct, it is redundant.
Calling read_channel_reply will read from hw the returned_bytes again,
and check for a buffer overflow. (At least all the current hooks do).

Thanks for the catch, will clean it up.
Leo

> 
>     674                          res = aux_engine->funcs->read_channel_reply(aux_engine, size,
>     675                                                                  buffer, reply,
>     676                                                                  &status);
>     677
>     678                  break;
> 
> regards,
> dan carpenter
>