Re: [POSSIBLE BUG] Dereferencing of NULL pointer

[Jan Beulich]

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