From e2e1c7bda4445363027a4ceed1cc3a8ccc2472a9 Mon Sep 17 00:00:00 2001 From: John Hubbard Date: Thu, 21 May 2020 21:15:05 -0700 Subject: [PATCH] video: fbdev: fix error handling for get_user_pages_fast() Dealing with the return value of get_user_pages*() variants has a few classic pitfalls, and this driver found one of them: the return value might be zero, positive, or -errno. And if positive, it might be fewer pages than were requested. And if fewer pages than requested, then the caller should return (via put_page()) the pages that *were* pinned. This driver was doing that *except* that it had a problem with the -errno case, which was being stored in an unsigned int, and which would case an interesting mess if it ever happened: nr_pages would be interpreted as a spectacularly huge unsigned value, rather than a small negative value. Also, it was unnecessarily overriding a potentially informative -errno, with -EINVAL, in some cases. Instead: clamp the nr_pages to zero or positive, so that the error handling works. And return the -errno value from get_user_pages*(), unchanged, if we get one. And explain this with comments, seeing as how it is error-prone. Cc: Bartlomiej Zolnierkiewicz Cc: Arnd Bergmann Cc: Daniel Vetter Cc: Gustavo A. R. Silva Cc: Jani Nikula Cc: dri-devel@lists.freedesktop.org Cc: linux-fbdev@vger.kernel.org Signed-off-by: John Hubbard Signed-off-by: Sam Ravnborg Link: https://patchwork.freedesktop.org/patch/msgid/20200522041506.39638-2-jhubbard@nvidia.com --- drivers/video/fbdev/pvr2fb.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/drivers/video/fbdev/pvr2fb.c b/drivers/video/fbdev/pvr2fb.c index f18d457175d9..ceb6ef590597 100644 --- a/drivers/video/fbdev/pvr2fb.c +++ b/drivers/video/fbdev/pvr2fb.c @@ -654,8 +654,22 @@ static ssize_t pvr2fb_write(struct fb_info *info, const char *buf, ret = get_user_pages_fast((unsigned long)buf, nr_pages, FOLL_WRITE, pages); if (ret < nr_pages) { - nr_pages = ret; - ret = -EINVAL; + if (ret < 0) { + /* + * Clamp the unsigned nr_pages to zero so that the + * error handling works. And leave ret at whatever + * -errno value was returned from GUP. + */ + nr_pages = 0; + } else { + nr_pages = ret; + /* + * Use -EINVAL to represent a mildly desperate guess at + * why we got fewer pages (maybe even zero pages) than + * requested. + */ + ret = -EINVAL; + } goto out_unmap; }