The patch applies to today's, May 5, linux-next just fine but I think
I need to re-write the commit message to make the bug more clear.
On Thu, May 05, 2022 at 05:39:35AM -0700, Luis Chamberlain wrote:
> On Thu, May 05, 2022 at 01:29:15PM +0300, Dan Carpenter wrote:
> > If we iterate through a loop using list_for_each_entry() without
> > hitting a break, then the iterator points to bogus memory. The
> > if (tst->name != test_fw_config->upload_name) { will likely still work
> > but technically it's an out of bounds read.
> >
> > Fixes: a31ad463b72d ("test_firmware: Add test support for firmware upload")
> > Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> > ---
> > lib/test_firmware.c | 11 +++++++----
> > 1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/lib/test_firmware.c b/lib/test_firmware.c
> > index 76115c1a2629..c82b65947ce6 100644
> > --- a/lib/test_firmware.c
> > +++ b/lib/test_firmware.c
> > @@ -1392,7 +1392,8 @@ static ssize_t upload_read_show(struct device *dev,
> > struct device_attribute *attr,
> > char *buf)
> > {
> > - struct test_firmware_upload *tst;
> > + struct test_firmware_upload *tst = NULL;
> > + struct test_firmware_upload *tst_iter;
> > int ret = -EINVAL;
> >
> > if (!test_fw_config->upload_name) {
> > @@ -1401,11 +1402,13 @@ static ssize_t upload_read_show(struct device *dev,
> > }
> >
> > mutex_lock(&test_fw_mutex);
>
> Note the mutex lock.
>
This lock is fine.
> > - list_for_each_entry(tst, &test_upload_list, node)
> > - if (tst->name == test_fw_config->upload_name)
> > + list_for_each_entry(tst_iter, &test_upload_list, node)
>
> If a lock is held I can't see how the premise of this patch is
> correct and we ensure we don't remove entries while holdingg
> the lock.
>
> Generalizing this problem seems like a bigger issue, no?
>
It has nothing to do with the look. The problem is using the list
iterator outside of the loop.
> Additionally this patch doesn't apply at all on linux-next.
>
> Luis
>
> > + if (tst_iter->name == test_fw_config->upload_name) {
> > + tst = tst_iter;
> > break;
> > + }
> >
> > - if (tst->name != test_fw_config->upload_name) {
> > + if (!tst) {
This test is reading out of bounds. Another fix would be to write it
as:
if (list_entry_is_head(tst, &test_upload_list, node)) {
But there is a desire to make it impossible to access the list iterator
outside the loop. Linus was drafting alternative list macros but I
don't know the status of that.
regards,
dan carpenter
