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(_req, 0, sizeof(aux_req));
653 memset(_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,
_req);
667 operation_result =
aux_engine->funcs->get_channel_status(aux_engine, _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
);
677
678 break;
regards,
dan carpenter
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
