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.
---------------------------------------------------------------------
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]);
^^^^^^^^^
---------------------------------------------------------------------
On 24.08.22 15:59, Jan Beulich wrote:
> 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?
Just started writing it. :-)
Juergen
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]);
>
> ^^^^^^^^^
> ---------------------------------------------------------------------
>
>