lkml.org 
[lkml]   [2020]   [Oct]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 4.19 111/264] nvmem: core: fix possibly memleak when use nvmem_cell_info_to_nvmem_cell()
    Hi!

    > From: Vadym Kochan <vadym.kochan@plvision.eu>
    >
    > [ Upstream commit fc9eec4d643597cf4cb2fef17d48110e677610da ]
    >
    > Fix missing 'kfree_const(cell->name)' when call to
    > nvmem_cell_info_to_nvmem_cell() in several places:
    >
    > * after nvmem_cell_info_to_nvmem_cell() failed during
    > nvmem_add_cells()
    >
    > * during nvmem_device_cell_{read,write} when cell->name is
    > kstrdup'ed() without calling kfree_const() at the end, but
    > really there is no reason to do that 'dup, because the cell
    > instance is allocated on the stack for some short period to be
    > read/write without exposing it to the caller.
    >
    > So the new nvmem_cell_info_to_nvmem_cell_nodup() helper is introduced
    > which is used to convert cell_info -> cell without name duplication as
    > a lighweight version of nvmem_cell_info_to_nvmem_cell().
    >
    > Fixes: e2a5402ec7c6 ("nvmem: Add nvmem_device based consumer apis.")

    There's something very wrong here.

    > index 30c040786fde2..54204d550fc22 100644
    > --- a/drivers/nvmem/core.c
    > +++ b/drivers/nvmem/core.c
    > @@ -326,9 +326,9 @@ static void nvmem_cell_add(struct nvmem_cell *cell)
    > mutex_unlock(&nvmem_cells_mutex);
    > }
    >
    > -static int nvmem_cell_info_to_nvmem_cell(struct nvmem_device *nvmem,
    > - const struct nvmem_cell_info *info,
    > - struct nvmem_cell *cell)
    > +static int nvmem_cell_info_to_nvmem_cell_nodup(struct nvmem_device *nvmem,
    > + const struct nvmem_cell_info *info,
    > + struct nvmem_cell *cell)
    > {
    > cell->nvmem = nvmem;
    > cell->offset = info->offset;
    > @@ -345,13 +345,30 @@ static int nvmem_cell_info_to_nvmem_cell(struct nvmem_device *nvmem,
    > if (!IS_ALIGNED(cell->offset, nvmem->stride)) {
    > dev_err(&nvmem->dev,
    > "cell %s unaligned to nvmem stride %d\n",
    > - cell->name, nvmem->stride);
    > + cell->name ?: "<unknown>", nvmem->stride);
    > return -EINVAL;
    > }
    >
    > return 0;
    > }

    We rename call from .._cell to .._cell_nodup, but it did not have the
    kstrdup_const() in the first place!

    > +static int nvmem_cell_info_to_nvmem_cell(struct nvmem_device *nvmem,
    > + const struct nvmem_cell_info *info,
    > + struct nvmem_cell *cell)
    > +{
    > + int err;
    > +
    > + err = nvmem_cell_info_to_nvmem_cell_nodup(nvmem, info, cell);
    > + if (err)
    > + return err;
    > +
    > + cell->name = kstrdup_const(info->name, GFP_KERNEL);
    > + if (!cell->name)
    > + return -ENOMEM;
    > +
    > + return 0;
    > +}

    So now we introduce an allocation, but we don't have a place to free
    it. In mainline, it is freed in nvmem_cell_drop(), but 4.19 does not
    have a free there.

    Best regards,
    Pavel
    --
    http://www.livejournal.com/~pavelmachek
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2020-10-28 23:42    [W:4.249 / U:0.188 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site