Hi Xiaomeng Thanks for the patch. Maybe it would be better to just make a function to find the ITERM entity with a given id? Regards! On Mon, 21 Mar 2022 at 16:33, Xiaomeng Tong <xiam0nd.tong@xxxxxxxxx> wrote: > > The list iterator will point to a bogus position containing HEAD if > the list is empty or the element is not found in list. This case > should be checked before any use of the iterator, otherwise it will > lead to a invalid memory access. The missing check here is before > "pin = iterm->id;", just add check here to fix the security bug. > > In addition, the list iterator value will *always* be set and non-NULL > by list_for_each_entry(), so it is incorrect to assume that the iterator > value will be NULL if the element is not found in list, considering > the (mis)use here: "if (iterm == NULL". > > Use a new value 'it' as the list iterator, while use the old value > 'iterm' as a dedicated pointer to point to the found element, which > 1. can fix this bug, due to 'iterm' is NULL only if it's not found. > 2. do not need to change all the uses of 'iterm' after the loop. > 3. can also limit the scope of the list iterator 'it' *only inside* > the traversal loop by simply declaring 'it' inside the loop in the > future, as usage of the iterator outside of the list_for_each_entry > is considered harmful. https://lkml.org/lkml/2022/2/17/1032 > > Fixes: d5e90b7a6cd1c ("[media] uvcvideo: Move to video_ioctl2") > Signed-off-by: Xiaomeng Tong <xiam0nd.tong@xxxxxxxxx> > --- > drivers/media/usb/uvc/uvc_v4l2.c | 20 +++++++++++++------- > 1 file changed, 13 insertions(+), 7 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c > index 711556d13d03..e7cdc01ad277 100644 > --- a/drivers/media/usb/uvc/uvc_v4l2.c > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > @@ -871,6 +871,7 @@ static int uvc_ioctl_enum_input(struct file *file, void *fh, > struct uvc_video_chain *chain = handle->chain; > const struct uvc_entity *selector = chain->selector; > struct uvc_entity *iterm = NULL; > + struct uvc_entity *it; > u32 index = input->index; > int pin = 0; > > @@ -878,22 +879,27 @@ static int uvc_ioctl_enum_input(struct file *file, void *fh, > (chain->dev->quirks & UVC_QUIRK_IGNORE_SELECTOR_UNIT)) { > if (index != 0) > return -EINVAL; > - list_for_each_entry(iterm, &chain->entities, chain) { > - if (UVC_ENTITY_IS_ITERM(iterm)) > + list_for_each_entry(it, &chain->entities, chain) { > + if (UVC_ENTITY_IS_ITERM(it)) { > + iterm = it; > break; > + } > } > - pin = iterm->id; > + if (iterm) > + pin = iterm->id; > } else if (index < selector->bNrInPins) { > pin = selector->baSourceID[index]; > - list_for_each_entry(iterm, &chain->entities, chain) { > - if (!UVC_ENTITY_IS_ITERM(iterm)) > + list_for_each_entry(it, &chain->entities, chain) { > + if (!UVC_ENTITY_IS_ITERM(it)) > continue; > - if (iterm->id == pin) > + if (it->id == pin) { > + iterm = it; > break; > + } > } > } > > - if (iterm == NULL || iterm->id != pin) > + if (iterm == NULL) > return -EINVAL; > > memset(input, 0, sizeof(*input)); > -- > 2.17.1 > -- Ricardo Ribalda
