On Fri, Sep 14 2012, Sebastian Andrzej Siewior wrote:
> Hi Michał,
>
> I'm looking at the life time of descriptors in each gadget and now I got to
> f_fs which brings me to this chunk:
>
> |static ssize_t ffs_ep0_write(struct file *file, const char __user *buf,
> | size_t len, loff_t *ptr)
> |{
> …
> | switch (ffs->state) {
> | case FFS_READ_DESCRIPTORS:
> | case FFS_READ_STRINGS:
> | /* Copy data */
> | if (unlikely(len < 16)) {
> | ret = -EINVAL;
> | break;
> | }
> |
> | data = ffs_prepare_buffer(buf, len);
>
> data contians the a new allocated buffer with data from userland
>
> | if (IS_ERR(data)) {
> | ret = PTR_ERR(data);
> | break;
> | }
> …
> | if (ffs->state == FFS_READ_DESCRIPTORS) {
> | pr_info("read descriptors\n");
> | ret = __ffs_data_got_descs(ffs, data, len);
>
> sets up descriptors and sets f->descriptors and f->hs_descriptors for composite
static int __ffs_data_got_descs(struct ffs_data *ffs,
char *const _data, size_t len)
{
…
ffs->raw_fs_descs_length = fs_len;
ffs->raw_descs_length = fs_len + ret;
ffs->raw_descs = _data;
Saved for later.
ffs->fs_descs_count = fs_count;
ffs->hs_descs_count = hs_count;
return 0;
einval:
ret = -EINVAL;
error:
kfree(_data);
Freed on error path.
return ret;
}
>
> | if (unlikely(ret < 0))
> | break;
> |
> | ffs->state = FFS_READ_STRINGS;
> | ret = len;
> | } else {
> | pr_info("read strings\n");
> | ret = __ffs_data_got_strings(ffs, data, len);
>
> the same thing for strings
static int __ffs_data_got_strings(struct ffs_data *ffs,
char *const _data, size_t len)
{
…
/*
* If we don't need any strings just return and free all
* memory.
*/
if (!needed_count) {
kfree(_data);
Freed on quick exit.
return 0;
}
…
/* Done! */
ffs->stringtabs = stringtabs;
ffs->raw_strings = _data;
Saved for later.
return 0;
error_free:
kfree(stringtabs);
error:
kfree(_data);
Freed on error path.
return -EINVAL;
}
>
> | if (unlikely(ret < 0))
> | break;
> …
> | return len;
> | }
> | break;
> …
> | default:
> | ret = -EBADFD;
> | break;
> | }
> |
> | mutex_unlock(&ffs->mutex);
> | return ret;
> AAAAAAAAAAAaaaaand we are gone
> | }
And later:
static void ffs_data_clear(struct ffs_data *ffs)
{
ENTER();
if (test_and_clear_bit(FFS_FL_CALL_CLOSED_CALLBACK, &ffs->flags))
functionfs_closed_callback(ffs);
BUG_ON(ffs->gadget);
if (ffs->epfiles)
ffs_epfiles_destroy(ffs->epfiles, ffs->eps_count);
kfree(ffs->raw_descs);
Free descs saved for later.
kfree(ffs->raw_strings);
Free strings saved for later.
kfree(ffs->stringtabs);
}
> Based on this I'm sure that data is leaked on the error path (on return from
> __ffs_data_got_descs() / __ffs_data_got_strings()) because I don't see any
> kfree() like I do in the FFS_ACTIVE case.
> Let's assume everything goes as planned. When are the descriptors which are
> set to f->descriptors f->hs_descriptors kfree()d?
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michał “mina86” Nazarewicz (o o)
ooo +----<email/xmpp: mpn@xxxxxxxxxx>--------------ooO--(_)--Ooo--Attachment:
pgpFweOA6Ja6x.pgp
Description: PGP signature
