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
>
