On 8/30/2024 3:01 PM, Dmitry Baryshkov wrote:
> On Thu, Aug 22, 2024 at 04:29:32PM GMT, Ekansh Gupta wrote:
>> Fastrpc driver creates maps for user allocated fd buffers. Before
>> creating a new map, the map list is checked for any already existing
>> maps using map fd. Checking with just map fd is not sufficient as the
>> user can pass offsetted buffer with less size when the map is created
>> and then a larger size the next time which could result in memory
>> issues. Check for user passed VA and length also when looking up for
>> the map.
>>
>> Fixes: c68cfb718c8f ("misc: fastrpc: Add support for context Invoke method")
>> Cc: stable <stable@xxxxxxxxxx>
>> Signed-off-by: Ekansh Gupta <quic_ekangupt@xxxxxxxxxxx>
>> ---
>> drivers/misc/fastrpc.c | 24 +++++++++++++++---------
>> 1 file changed, 15 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> index bcfb8ce1a0e3..ebe828770a8d 100644
>> --- a/drivers/misc/fastrpc.c
>> +++ b/drivers/misc/fastrpc.c
>> @@ -362,7 +362,8 @@ static int fastrpc_map_get(struct fastrpc_map *map)
>>
>>
>> static int fastrpc_map_lookup(struct fastrpc_user *fl, int fd,
>> - struct fastrpc_map **ppmap, bool take_ref)
>> + u64 va, u64 len, struct fastrpc_map **ppmap,
>> + bool take_ref)
> Please use consistent alignment between the lines.
Ack.
>> {
>> struct fastrpc_session_ctx *sess = fl->sctx;
>> struct fastrpc_map *map = NULL;
>> @@ -370,7 +371,8 @@ static int fastrpc_map_lookup(struct fastrpc_user *fl, int fd,
>>
>> spin_lock(&fl->lock);
>> list_for_each_entry(map, &fl->maps, node) {
>> - if (map->fd != fd)
>> + if (map->fd != fd || va < (u64)map->va ||
>> + va + len > (u64)map->va + map->size)
>> continue;
>>
>> if (take_ref) {
>> @@ -752,7 +754,8 @@ static const struct dma_buf_ops fastrpc_dma_buf_ops = {
>> };
>>
>> static int fastrpc_map_create(struct fastrpc_user *fl, int fd,
>> - u64 len, u32 attr, struct fastrpc_map **ppmap)
>> + u64 va, u64 len, u32 attr,
>> + struct fastrpc_map **ppmap)
>> {
>> struct fastrpc_session_ctx *sess = fl->sctx;
>> struct fastrpc_map *map = NULL;
>> @@ -760,7 +763,7 @@ static int fastrpc_map_create(struct fastrpc_user *fl, int fd,
>> struct scatterlist *sgl = NULL;
>> int err = 0, sgl_index = 0;
>>
>> - if (!fastrpc_map_lookup(fl, fd, ppmap, true))
>> + if (!fastrpc_map_lookup(fl, fd, va, len, ppmap, true))
>> return 0;
>>
>> map = kzalloc(sizeof(*map), GFP_KERNEL);
>> @@ -807,7 +810,7 @@ static int fastrpc_map_create(struct fastrpc_user *fl, int fd,
>> err = -EINVAL;
>> goto map_err;
>> }
>> - map->va = sg_virt(map->table->sgl);
>> + map->va = (void *)(uintptr_t)va;
> This looks unrelated
Will be dropping this.
>
>> map->len = len;
>>
>> if (attr & FASTRPC_ATTR_SECUREMAP) {
>> @@ -920,7 +923,8 @@ static int fastrpc_create_maps(struct fastrpc_invoke_ctx *ctx)
>> continue;
>>
>> err = fastrpc_map_create(ctx->fl, ctx->args[i].fd,
>> - ctx->args[i].length, ctx->args[i].attr, &ctx->maps[i]);
>> + (u64)ctx->args[i].ptr, ctx->args[i].length,
>> + ctx->args[i].attr, &ctx->maps[i]);
>> if (err) {
>> dev_err(dev, "Error Creating map %d\n", err);
>> return -EINVAL;
>> @@ -1106,7 +1110,7 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
>> for (i = 0; i < FASTRPC_MAX_FDLIST; i++) {
>> if (!fdlist[i])
>> break;
>> - if (!fastrpc_map_lookup(fl, (int)fdlist[i], &mmap, false))
>> + if (!fastrpc_map_lookup(fl, (int)fdlist[i], 0, 0, &mmap, false))
> Isn't this going to always return false?
Yes, I'll correct this.
>
>> fastrpc_map_put(mmap);
>> }
>>
>> @@ -1412,7 +1416,8 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
>> fl->pd = USER_PD;
>>
>> if (init.filelen && init.filefd) {
>> - err = fastrpc_map_create(fl, init.filefd, init.filelen, 0, &map);
>> + err = fastrpc_map_create(fl, init.filefd, init.file,
>> + init.filelen, 0, &map);
>> if (err)
>> goto err;
>> }
>> @@ -2034,7 +2039,8 @@ static int fastrpc_req_mem_map(struct fastrpc_user *fl, char __user *argp)
>> return -EFAULT;
>>
>> /* create SMMU mapping */
>> - err = fastrpc_map_create(fl, req.fd, req.length, 0, &map);
>> + err = fastrpc_map_create(fl, req.fd, req.vaddrin, req.length,
>> + 0, &map);
>> if (err) {
>> dev_err(dev, "failed to map buffer, fd = %d\n", req.fd);
>> return err;
>> --
>> 2.34.1
>>
