Re: [POSSIBLE BUG] Dereferencing of NULL pointer

[Jan Beulich <jbeulich@xxxxxxxx>]

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]);
>                                                               
>               ^^^^^^^^^
> ---------------------------------------------------------------------
> 
>