On 20.08.2022 19:30, Rustam Subkhankulov wrote:
> Version: 6.0-rc1
>
> Description:
>
> In function 'privcmd_ioctl_dm_op' (drivers/xen/privcmd.c: 615)return
> value of 'kcalloc' with GFP_KERNEL flag is assigned to "pages"
> variable. GFP_KERNEL flag does not guarantee, that the return value
> will not be NULL. In that case, there is a jump to the "out" label.
The problem is wider than that, because earlier errors would also
lead to "out" (e.g. after copy_from_user() failed). Plus I guess
unlock_pages() shouldn't be called at all (or with its 2nd arg set
to zero) before lock_pages() was actually called. But I agree with
the further analysis below. Would you mind sending a patch?
Jan
> ---------------------------------------------------------------------
> 667 pages = kcalloc(nr_pages, sizeof(*pages), GFP_KERNEL);
> 668 if (!pages) {
> 669 rc = -ENOMEM;
> 670 goto out;
> 671 }
> ---------------------------------------------------------------------
>
> Variable 'pages' is passed to function 'unpin_user_pages_dirty_lock' as
> 1st parameter at [drivers/xen/privcmd.c: 695].
>
> ---------------------------------------------------------------------
> 694 out:
> 695 unlock_pages(pages, nr_pages);
> ---------------------------------------------------------------------
>
> Then, variable 'pages' is passed to function
> 'unpin_user_pages_dirty_lock' as 1st parameter at
> [drivers/xen/privcmd.c: 612].
>
> ---------------------------------------------------------------------
> 610 static void unlock_pages(struct page *pages[], unsigned int
> nr_pages)
> 611 {
> 612 unpin_user_pages_dirty_lock(pages, nr_pages, true);
> 613 }
> ---------------------------------------------------------------------
>
> 'pages' and 'npages' are passed as parameters to function
> 'sanity_check_pinned_pages' at [mm/gup.c: 311].
>
> ---------------------------------------------------------------------
> 299 void unpin_user_pages_dirty_lock(struct page **pages, unsigned
> long npages,
> 300 bool make_dirty)
> 301 {
> 302 unsigned long i;
> 303 struct folio *folio;
> 304 unsigned int nr;
> 305
> 306 if (!make_dirty) {
> 307 unpin_user_pages(pages, npages);
> 308 return;
> 309 }
> 310
> 311 sanity_check_pinned_pages(pages, npages);
> ---------------------------------------------------------------------
>
> In function 'sanity_check_pinned_pages', if
> (IS_ENABLED(CONFIG_DEBUG_VM)) and (npages > 0), NULL pointer 'pages' is
> dereferenced at [mm/gup.c: 51].
>
> ---------------------------------------------------------------------
> 32 static inline void sanity_check_pinned_pages(struct page
> **pages,
> 33 unsigned long
> npages)
> 34 {
> 35 if (!IS_ENABLED(CONFIG_DEBUG_VM))
> 36 return;
> ..
> 50 for (; npages; npages--, pages++) {
> 51 struct page *page = *pages;
> ^^^^^^
> ^
> ---------------------------------------------------------------------
>
> Else if (!IS_ENABLED(CONFIG_DEBUG_VM)) and (npages > 0) function
> 'gup_folio_next' is called with 'pages' and 'npages' as parameters at
> [mm/gup.c: 311].
>
> ---------------------------------------------------------------------
> 312 for (i = 0; i < npages; i += nr) {
> 313 folio = gup_folio_next(pages, npages, i, &nr);
> ---------------------------------------------------------------------
>
> In function 'gup_folio_next' NULL pointer 'list' is dereferenced at
> [mm/gup.c: 263].
>
> ---------------------------------------------------------------------
> 262 static inline struct folio *gup_folio_next(struct page **list,
> 263 unsigned long npages, unsigned long i,
> unsigned int *ntails)
> 264 {
> 265 struct folio *folio = page_folio(list[i]);
>
> ^^^^^^^^^
> ---------------------------------------------------------------------
>
>
