Bug 2

Summary: NUMA-Q hangs during TSC initialization on boot.
Product: Platform Specific/Hardware Reporter: Martin J. Bligh (mbligh)
Component: i386Assignee: Martin J. Bligh (mbligh)
Status: CLOSED CODE_FIX    
Severity: normal    
Priority: P2    
Hardware: IA-32   
OS: Linux   
Kernel Version: Subsystem:
Regression: --- Bisected commit-id:

Description Martin J. Bligh 2002-11-13 15:56:16 UTC
Exact Kernel version: 2.5.46
Distribution: debian woody
Harware Environment: 16-way NUMA-Q
Problem Description: Hangs during TSC initialization on boot.

There's a garbled panic during IO-APIC init, then hang during TSC sync
Comment 1 Martin J. Bligh 2002-11-13 15:58:49 UTC
Here's an ungarbled panic, derived by putting some delays into the IO-APIC init
code:

CPU:    1
EIP:    0060:[<ffffff97>]    Not tainted
EFLAGS: 00010286
EIP is at E ipv4_config+0x3fc828af/0xffe42c88
eax: 00000000   ebx: c3934940   ecx: c031f178   edx: 036147a0
esi: 00000000   edi: f019c000   ebp: 00000001   esp: f019def0
ds: 0068   es: 0068   ss: 0068
Process swapper (pid: 0, threadinfo=f019c000 task=f01c5740)
Stack: f019c000 00000000 00000001 f019df10 c3934940 036147a0 c031f178 00000000
       c011d8b5 00000000 00000011 c02db960 ffffffee 00000020 c031f178 c031f178
       c011d5ba c02db960 f019c000 00000000 c02aff00 f019df64 00000046 c010904d
Call Trace:
 [<c011d8b5>] tasklet_hi_action+0x85/0xe0
 [<c011d5ba>] do_softirq+0x5a/0xac
 [<c010904d>] do_IRQ+0x18d/0x1b0
 [<c01078cc>] common_interrupt+0x18/0x20
 [<c0118bc8>] _call_console_drivers+0x50/0x58
 [<c0118ca9>] call_console_drivers+0xd9/0xe0
 [<c0118ee2>] release_console_sem+0x42/0xa4
Code:  Bad EIP value.
<0>Kernel panic: Aiee, killing interrupt handler!
Comment 2 Martin J. Bligh 2002-11-13 16:02:17 UTC
A non-boot cpu is getting a timer interrupt, and trying to do
softirq processing in irq_exit. However, the per-cpu stuff for
this is not initialised yet.

This patch fixes the problem by disabling interrupts for the
secondary CPU from before we program the IO-APIC until after
we've done the init in __cpu_up.

diff -purN -X /home/mbligh/.diff.exclude virgin/arch/i386/kernel/smpboot.c
noearlyirq/arch/i386/kernel/smpboot.c
--- virgin/arch/i386/kernel/smpboot.c   Mon Nov  4 14:30:27 2002
+++ noearlyirq/arch/i386/kernel/smpboot.c       Wed Nov  6 15:22:12 2002
@@ -419,6 +419,7 @@ void __init smp_callin(void)
        smp_store_cpu_info(cpuid);
 
        disable_APIC_timer();
+       local_irq_disable();
        /*
         * Allow the master to continue.
         */
@@ -1186,6 +1187,7 @@ int __devinit __cpu_up(unsigned int cpu)
        if (!test_bit(cpu, &cpu_callin_map))
                return -EIO;
 
+       local_irq_enable();
        /* Unleash the CPU! */
        set_bit(cpu, &smp_commenced_mask);
        while (!test_bit(cpu, &cpu_online_map))
Comment 3 karl.belakh.82 2017-07-09 12:15:36 UTC
г.Москва • 19 - 21 июля 2017 г. • артикул - 849

Лицензированный центр повышения квалификации специалистов
приглашает Вас принять участие в обучении:

Внешнеторговые сделки в современных условиях:
практические рекомендации по снижению рисков.
Посвящено детальному анализу основных условий внешнеэкономического контракта с 
учетом практики заключения, исполнения и разрешения споров. Большое внимание уделяется 
типичным ошибкам, допускаемым при заключении контрактов, при предъявлении исков и их 
рассмотрении в МКАС при ТПП РФ, в арбитражных судах в России и за рубежом

Ведущий:
-Тренер-консультант, MBA. Специалист в области ВЭД, международного транспорта, логистики и 
управления цепями поставок, проектного финансирования. Практический опыт работы более 25 
лет. Окончил с отличием МГИМО МИД СССР, факультет МЭО, специальность - экономист по 
внешнеэкономическим связям, международные перевозки (1980 г.), заочную аспирантуру 
"Союзморниипроект" (1984 г.), International Business School при Университете г. Дарэм 
(Великобритания) по программе МВА (1995 г.), International Business School "Frans Maas" в г. 
Венло (Нидерланды), специальность - складская логистика (2003 г.).
-Аттестованный аудитор. Руководитель департамента консалтинговых услуг аудиторской 
компании. Автор многочисленных публикаций в специализированных журналах "Главбух", 
"Главбух". Приложение "Учет в строительстве", "Российский налоговый курьер", "Документы и 
комментарии", "Новая бухгалтерия", газете "Учет. Налоги. Право", Бухгалтерском приложении к 
газете "Экономика и жизнь".
 

Полная программа и регистрация участников по тел.:
8 префикс{495} телефон.:637-83-04;
637-82-04;
637-83-14 {многоканальный}

Участники с помощью сети Интернет могут
видеть и слышать преподавателя в
режиме реального времени.

Обучение состоится по адресу: 
город Москва, м. Бауманская, ул. Бауманская д.6, стр.2., бизнес центр "Виктория плаза".
 От метро 5 минут пешком. Участникам высылается подробная схема проезда.
 

Определение наиболее эффективной модели рассмотрения внешнеторговых споров в 
арбитражных судах РФ. Выбор между государственным судом и международным 
коммерческим арбитражем.
- Процессуальные особенности, преимущества и недостатки рассмотрения внешнеторговых 
споров в государственных арбитражных судах. 
- Процессуальные особенности рассмотрения споров в МКАС и ТПП РФ. 
- Составление арбитражной оговорки: практические вопросы и рекомендации. 
- Оспаривание и приведение в исполнение решений международных коммерческих арбитражей: 
рассмотрение последней практики внешнеторговых споров в российских государственных 
судах и международных коммерческих арбитражах. 
- Анализ актуальных практических вопросов.Основные нарушения и ошибки, допускаемые при оформлении международных 
документов.
- Способы защиты от нарушений (по международно-правовым и национальным нормам).
Правовое обеспечение внешнеэкономической и внешнеторговой деятельности. Анализ 
вступивших в силу и ожидаемых изменений.
Внешнеторговый контракт. Структура и содержание внешнеторгового контракта. 
- Стратегия и тактика переговоров о заключении контракта. Проблемные вопросы подготовки 
контракта (выбор надлежащего партнера, выбор способа обеспечения исполнения 
обязательств контрагентом, выбор применимого права и способа разрешения возможных в 
будущем споров). Инструкции по подготовке и проверке документов. Согласование условий 
контракта. 
- Исполнение договорных обязательств. Двуязычные формулировки контрактов.
- Особенности реализации контракта в зависимости от выбора норм национального права.
- Лицензирование, сертификация, декларирование.
Особенности налогообложения и налогового контроля ВЭД в 2017 году.
- Контролируемые внешнеторговые сделки с точки зрения налогового законодательства.
- НДС при экспортно-импортных операциях. Особенности налогообложения при 
взаимоотношениях с ближним зарубежьем. 
- Оформление счетов-фактур по внешнеторговым операциям. 
- Особенности исчисления налога на прибыль при совершении внешнеторговых сделок. 
- Случаи и основания для исчисления и удержания налога с доходов и НДС при перечислениях 
в адрес иностранной организации. Отчетность налогового агента по НДС и доходам.
Таможенное регулирование и ВТО.
- Особенности подготовки основных документов для реализации внешнеторговой сделки и 
таможенного оформления товаров.
- Соотношение обязательств России в рамках ВТО и в рамках Таможенного союза. 
Регулирование движения товаров на границах Таможенного союза и внутри Таможенного 
союза.
- Правовые инструменты и процедуры урегулирование торговых споров в рамках ВТО. Риски и 
нежелательные ситуации по действующим договорам.
Практические вопросы предупреждения потенциальных рисков при заключении и 
исполнении договоров с участием иностранных фирм.
Ответственность во внешнеэкономических сделках.
- Применение мер ответственности сторон обязательства в контексте права, применимого к 
договору.
- Основания и условия применения мер ответственности.
- Убытки и неустойка. 
- Проценты по применимому праву. 
- Сравнительный анализ Венской конвенции и норм ГК РФ.
Правовое положение иностранных лиц в арбитражном процессе.
- Установление правового статуса иностранных лиц. 
- Особенности рассмотрения дел с участием иностранных лиц. 
- Порядок извещения иностранных лиц.
- Признание и приведение в исполнение решений иностранных судов.
- Основания для признания и исполнения решений иностранных судов и арбитражей. 
- Определение арбитражного суда по делу о признании и приведении в исполнение решения 
иностранного суда.
Новое в сфере валютного контроля и регулирования валютных операций при 
осуществлении ВЭД.
- Федеральный закон №173-ФЗ "О валютном регулировании и валютном контроле": основные 
понятия и требования.
- Практика применения Инструкции Банка России от 04.06.2012 № 138-И. 
- Полномочия органов и агентов валютного контроля (ФНС, Росфиннадзора, ФТС, банки), их 
взаимодействие. 
- Паспорт сделки: актуальные вопросы, проблемы, анализ арбитражной практики. 
- Санкции за нарушения валютного законодательства.
- Основные тенденции судебной практики по спорам с Росфиннадзором.
Базисные условия поставки (Инкотермс - 2010). Комментарии к основным положениям.
- Принципы УНИДРУА 2010. Инкотермс и виды транспорта. 
- Ошибки и рекомендации по выбору оптимальных условий поставки.
- Инкотермс и страхование. 
- Переход права собственности.
- Безвозмездные поставки.

 
Участие составляет: 289оо р.
[ двадцать восемь тысяч девятьсот рублей ]
Скидка (от 2 чел и более - 7%)
Для участников предусмотрено:
Методический материал, обеды, кофе-паузы.
Сертификат об участии в семинаре.
Иногородним участникам помогаем в бронировании гостиницы.
Comment 4 mike.kravetz 2018-03-08 21:10:11 UTC
A vma with vm_pgoff large enough to overflow a loff_t type when
converted to a byte offset can be passed via the remap_file_pages
system call.  The hugetlbfs mmap routine uses the byte offset to
calculate reservations and file size.

A sequence such as:
  mmap(0x20a00000, 0x600000, 0, 0x66033, -1, 0);
  remap_file_pages(0x20a00000, 0x600000, 0, 0x20000000000000, 0);
will result in the following when task exits/file closed,
  kernel BUG at mm/hugetlb.c:749!
Call Trace:
  hugetlbfs_evict_inode+0x2f/0x40
  evict+0xcb/0x190
  __dentry_kill+0xcb/0x150
  __fput+0x164/0x1e0
  task_work_run+0x84/0xa0
  exit_to_usermode_loop+0x7d/0x80
  do_syscall_64+0x18b/0x190
  entry_SYSCALL_64_after_hwframe+0x3d/0xa2

The overflowed pgoff value causes hugetlbfs to try to set up a
mapping with a negative range (end < start) that leaves invalid
state which causes the BUG.

The previous overflow fix to this code was incomplete and did not
take the remap_file_pages system call into account.

Fixes: 045c7a3f53d9 ("hugetlbfs: fix offset overflow in hugetlbfs mmap")
Cc: <stable@vger.kernel.org>
Reported-by: Nic Losby <blurbdust@gmail.com>
Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
---
Changes in v2
  * Use bitmask for overflow check as suggested by Yisheng Xie
  * Add explicit (from > to) check when setting up reservations
  * Cc stable

 fs/hugetlbfs/inode.c | 11 ++++++++---
 mm/hugetlb.c         |  6 ++++++
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index 8fe1b0aa2896..dafffa6affae 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -111,6 +111,7 @@ static void huge_pagevec_release(struct pagevec *pvec)
 static int hugetlbfs_file_mmap(struct file *file, struct vm_area_struct *vma)
 {
 	struct inode *inode = file_inode(file);
+	unsigned long ovfl_mask;
 	loff_t len, vma_len;
 	int ret;
 	struct hstate *h = hstate_file(file);
@@ -127,12 +128,16 @@ static int hugetlbfs_file_mmap(struct file *file, struct vm_area_struct *vma)
 	vma->vm_ops = &hugetlb_vm_ops;
 
 	/*
-	 * Offset passed to mmap (before page shift) could have been
-	 * negative when represented as a (l)off_t.
+	 * page based offset in vm_pgoff could be sufficiently large to
+	 * overflow a (l)off_t when converted to byte offset.
 	 */
-	if (((loff_t)vma->vm_pgoff << PAGE_SHIFT) < 0)
+	ovfl_mask = (1UL << (PAGE_SHIFT + 1)) - 1;
+	ovfl_mask <<= ((sizeof(unsigned long) * BITS_PER_BYTE) -
+		       (PAGE_SHIFT + 1));
+	if (vma->vm_pgoff & ovfl_mask)
 		return -EINVAL;
 
+	/* must be huge page aligned */
 	if (vma->vm_pgoff & (~huge_page_mask(h) >> PAGE_SHIFT))
 		return -EINVAL;
 
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 7c204e3d132b..8eeade0a0b7a 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -4374,6 +4374,12 @@ int hugetlb_reserve_pages(struct inode *inode,
 	struct resv_map *resv_map;
 	long gbl_reserve;
 
+	/* This should never happen */
+	if (from > to) {
+		VM_WARN(1, "%s called with a negative range\n", __func__);
+		return -EINVAL;
+	}
+
 	/*
 	 * Only apply hugepage reservation if asked. At fault time, an
 	 * attempt will be made for VM_NORESERVE to allocate a page
Comment 5 Andrew Morton 2018-03-08 22:15:37 UTC
On Thu,  8 Mar 2018 13:05:02 -0800 Mike Kravetz <mike.kravetz@oracle.com> wrote:

> A vma with vm_pgoff large enough to overflow a loff_t type when
> converted to a byte offset can be passed via the remap_file_pages
> system call.  The hugetlbfs mmap routine uses the byte offset to
> calculate reservations and file size.
> 
> A sequence such as:
>   mmap(0x20a00000, 0x600000, 0, 0x66033, -1, 0);
>   remap_file_pages(0x20a00000, 0x600000, 0, 0x20000000000000, 0);
> will result in the following when task exits/file closed,
>   kernel BUG at mm/hugetlb.c:749!
> Call Trace:
>   hugetlbfs_evict_inode+0x2f/0x40
>   evict+0xcb/0x190
>   __dentry_kill+0xcb/0x150
>   __fput+0x164/0x1e0
>   task_work_run+0x84/0xa0
>   exit_to_usermode_loop+0x7d/0x80
>   do_syscall_64+0x18b/0x190
>   entry_SYSCALL_64_after_hwframe+0x3d/0xa2
> 
> The overflowed pgoff value causes hugetlbfs to try to set up a
> mapping with a negative range (end < start) that leaves invalid
> state which causes the BUG.
> 
> The previous overflow fix to this code was incomplete and did not
> take the remap_file_pages system call into account.
> 
> --- a/fs/hugetlbfs/inode.c
> +++ b/fs/hugetlbfs/inode.c
> @@ -111,6 +111,7 @@ static void huge_pagevec_release(struct pagevec *pvec)
>  static int hugetlbfs_file_mmap(struct file *file, struct vm_area_struct
>  *vma)
>  {
>       struct inode *inode = file_inode(file);
> +     unsigned long ovfl_mask;
>       loff_t len, vma_len;
>       int ret;
>       struct hstate *h = hstate_file(file);
> @@ -127,12 +128,16 @@ static int hugetlbfs_file_mmap(struct file *file,
> struct vm_area_struct *vma)
>       vma->vm_ops = &hugetlb_vm_ops;
>  
>       /*
> -      * Offset passed to mmap (before page shift) could have been
> -      * negative when represented as a (l)off_t.
> +      * page based offset in vm_pgoff could be sufficiently large to
> +      * overflow a (l)off_t when converted to byte offset.
>        */
> -     if (((loff_t)vma->vm_pgoff << PAGE_SHIFT) < 0)
> +     ovfl_mask = (1UL << (PAGE_SHIFT + 1)) - 1;
> +     ovfl_mask <<= ((sizeof(unsigned long) * BITS_PER_BYTE) -
> +                    (PAGE_SHIFT + 1));

That's a compile-time constant.  The compiler will indeed generate an
immediate load, but I think it would be better to make the code look
more like we know that it's a constant, if you get what I mean. 
Something like

/*
 * If a pgoff_t is to be converted to a byte index, this is the max value it
 * can have to avoid overflow in that conversion.
 */
#define PGOFF_T_MAX	<long string of crap>

And I bet that this constant could be used elsewhere - surely it's a
very common thing to be checking for.


Also, the expression seems rather complicated.  Why are we adding 1 to
PAGE_SHIFT?  Isn't there a logical way of using PAGE_MASK?

The resulting constant is 0xfff8000000000000 on 64-bit.  We could just
use along the lines of

	1UL << (BITS_PER_LONG - PAGE_SHIFT - 1)

But why the -1?  We should be able to handle a pgoff_t of
0xfff0000000000000 in this code?


Also, we later to

	len = vma_len + ((loff_t)vma->vm_pgoff << PAGE_SHIFT);
	/* check for overflow */
	if (len < vma_len)
		return -EINVAL;

which is ungainly: even if we passed the PGOFF_T_MAX test, there can
still be an overflow which we still must check for.  Is that avoidable?
Probably not...
Comment 6 mike.kravetz 2018-03-08 23:38:04 UTC
On 03/08/2018 02:15 PM, Andrew Morton wrote:
> On Thu,  8 Mar 2018 13:05:02 -0800 Mike Kravetz <mike.kravetz@oracle.com>
> wrote:
> 
>> A vma with vm_pgoff large enough to overflow a loff_t type when
>> converted to a byte offset can be passed via the remap_file_pages
>> system call.  The hugetlbfs mmap routine uses the byte offset to
>> calculate reservations and file size.
>>
>> A sequence such as:
>>   mmap(0x20a00000, 0x600000, 0, 0x66033, -1, 0);
>>   remap_file_pages(0x20a00000, 0x600000, 0, 0x20000000000000, 0);
>> will result in the following when task exits/file closed,
>>   kernel BUG at mm/hugetlb.c:749!
>> Call Trace:
>>   hugetlbfs_evict_inode+0x2f/0x40
>>   evict+0xcb/0x190
>>   __dentry_kill+0xcb/0x150
>>   __fput+0x164/0x1e0
>>   task_work_run+0x84/0xa0
>>   exit_to_usermode_loop+0x7d/0x80
>>   do_syscall_64+0x18b/0x190
>>   entry_SYSCALL_64_after_hwframe+0x3d/0xa2
>>
>> The overflowed pgoff value causes hugetlbfs to try to set up a
>> mapping with a negative range (end < start) that leaves invalid
>> state which causes the BUG.
>>
>> The previous overflow fix to this code was incomplete and did not
>> take the remap_file_pages system call into account.
>>
>> --- a/fs/hugetlbfs/inode.c
>> +++ b/fs/hugetlbfs/inode.c
>> @@ -111,6 +111,7 @@ static void huge_pagevec_release(struct pagevec *pvec)
>>  static int hugetlbfs_file_mmap(struct file *file, struct vm_area_struct
>>  *vma)
>>  {
>>      struct inode *inode = file_inode(file);
>> +    unsigned long ovfl_mask;
>>      loff_t len, vma_len;
>>      int ret;
>>      struct hstate *h = hstate_file(file);
>> @@ -127,12 +128,16 @@ static int hugetlbfs_file_mmap(struct file *file,
>> struct vm_area_struct *vma)
>>      vma->vm_ops = &hugetlb_vm_ops;
>>  
>>      /*
>> -     * Offset passed to mmap (before page shift) could have been
>> -     * negative when represented as a (l)off_t.
>> +     * page based offset in vm_pgoff could be sufficiently large to
>> +     * overflow a (l)off_t when converted to byte offset.
>>       */
>> -    if (((loff_t)vma->vm_pgoff << PAGE_SHIFT) < 0)
>> +    ovfl_mask = (1UL << (PAGE_SHIFT + 1)) - 1;
>> +    ovfl_mask <<= ((sizeof(unsigned long) * BITS_PER_BYTE) -
>> +                   (PAGE_SHIFT + 1));
> 
> That's a compile-time constant.  The compiler will indeed generate an
> immediate load, but I think it would be better to make the code look
> more like we know that it's a constant, if you get what I mean. 
> Something like
> 
> /*
>  * If a pgoff_t is to be converted to a byte index, this is the max value it
>  * can have to avoid overflow in that conversion.
>  */
> #define PGOFF_T_MAX   <long string of crap>

Ok

> And I bet that this constant could be used elsewhere - surely it's a
> very common thing to be checking for.
> 
> 
> Also, the expression seems rather complicated.  Why are we adding 1 to
> PAGE_SHIFT?  Isn't there a logical way of using PAGE_MASK?

The + 1 is there because this value will eventually be converted to
a loff_t which is signed.  So, we need to take that sign bit into
account or we could end up with a negative value.

For PAGE_SHIFT == 12, PAGE_MASK is 0xfffffffffffff000.  Our target
mask is  0xfff8000000000000 (for the sign bit).  So, we could do
PAGE_MASK << (BITS_PER_LONG - (2 * PAGE_SHIFT) - 1)

This legacy hugetlbfs code may be a little different than other areas
in the use of loff_t.  When doing some previous work in this area, I
did not find enough common used to make this more general purpose.  See,
https://lkml.org/lkml/2017/4/12/793

> The resulting constant is 0xfff8000000000000 on 64-bit.  We could just
> use along the lines of
> 
>       1UL << (BITS_PER_LONG - PAGE_SHIFT - 1)

Ah yes, BITS_PER_LONG is better than (sizeof(unsigned long) * BITS_PER_BYTE

> But why the -1?  We should be able to handle a pgoff_t of
> 0xfff0000000000000 in this code?

I'm not exactly sure what you are asking/suggesting here and in the line
above.  It is because of the conversion to a signed value that we have to
go with 0xfff8000000000000 instead of 0xfff0000000000000.

Here are a couple options for computing the mask.  I changed the name
you suggested to make it more obvious that the mask is being used to
check for loff_t overflow.

If we want to explicitly comptue the mask as in code above.
#define PGOFF_LOFFT_MAX \
	(((1UL << (PAGE_SHIFT + 1)) - 1) <<  (BITS_PER_LONG - (PAGE_SHIFT + 1)))

Or, we use PAGE_MASK
#define PGOFF_LOFFT_MAX (PAGE_MASK << (BITS_PER_LONG - (2 * PAGE_SHIFT) - 1))

In either case, we need a big comment explaining the mask and
how we have that extra bit +/- 1 because the offset will be converted
to a signed value.
	
> Also, we later to
> 
>       len = vma_len + ((loff_t)vma->vm_pgoff << PAGE_SHIFT);
>       /* check for overflow */
>       if (len < vma_len)
>               return -EINVAL;
> 
> which is ungainly: even if we passed the PGOFF_T_MAX test, there can
> still be an overflow which we still must check for.  Is that avoidable?
> Probably not...

Yes, it is required.  That check takes into account the length argument
which is added to page offset.  So, yes you can pass the first check and
fail this one.
Comment 7 Andrew Morton 2018-03-08 23:53:43 UTC
On Thu, 8 Mar 2018 15:37:57 -0800 Mike Kravetz <mike.kravetz@oracle.com> wrote:

> Here are a couple options for computing the mask.  I changed the name
> you suggested to make it more obvious that the mask is being used to
> check for loff_t overflow.
> 
> If we want to explicitly comptue the mask as in code above.
> #define PGOFF_LOFFT_MAX \
>       (((1UL << (PAGE_SHIFT + 1)) - 1) <<  (BITS_PER_LONG - (PAGE_SHIFT +
> 1)))
> 
> Or, we use PAGE_MASK
> #define PGOFF_LOFFT_MAX (PAGE_MASK << (BITS_PER_LONG - (2 * PAGE_SHIFT) - 1))

Sounds good.

> In either case, we need a big comment explaining the mask and
> how we have that extra bit +/- 1 because the offset will be converted
> to a signed value.

Yup.

> > Also, we later to
> > 
> >     len = vma_len + ((loff_t)vma->vm_pgoff << PAGE_SHIFT);
> >     /* check for overflow */
> >     if (len < vma_len)
> >             return -EINVAL;
> > 
> > which is ungainly: even if we passed the PGOFF_T_MAX test, there can
> > still be an overflow which we still must check for.  Is that avoidable?
> > Probably not...
> 
> Yes, it is required.  That check takes into account the length argument
> which is added to page offset.  So, yes you can pass the first check and
> fail this one.

Well I was sort of wondering if both checks could be done in a single
operation, but I guess not.
Comment 8 Hans Verkuil 2024-10-24 08:30:55 UTC
The maximum number of buffers that can be requested was increased to
64 for the video capture queue. But video capture used a must_blank
array that was still sized for 32 (VIDEO_MAX_FRAME). This caused an
out-of-bounds write when using buffer indices >= 32.

Create a new define MAX_VID_CAP_BUFFERS that is used to access the
must_blank array and set max_num_buffers for the video capture queue.

This solves a crash reported by:

	https://bugzilla.kernel.org/show_bug.cgi?id=219258

Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
Fixes: cea70ed416b4 ("media: test-drivers: vivid: Increase max supported buffers for capture queues")
Cc: stable@vger.kernel.org
---
 drivers/media/test-drivers/vivid/vivid-core.c    | 2 +-
 drivers/media/test-drivers/vivid/vivid-core.h    | 4 +++-
 drivers/media/test-drivers/vivid/vivid-ctrls.c   | 2 +-
 drivers/media/test-drivers/vivid/vivid-vid-cap.c | 2 +-
 4 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/media/test-drivers/vivid/vivid-core.c b/drivers/media/test-drivers/vivid/vivid-core.c
index 8d8f60e15d1d..7477ac8cb955 100644
--- a/drivers/media/test-drivers/vivid/vivid-core.c
+++ b/drivers/media/test-drivers/vivid/vivid-core.c
@@ -910,7 +910,7 @@ static int vivid_create_queue(struct vivid_dev *dev,
 	 * videobuf2-core.c to MAX_BUFFER_INDEX.
 	 */
 	if (buf_type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
-		q->max_num_buffers = 64;
+		q->max_num_buffers = MAX_VID_CAP_BUFFERS;
 	if (buf_type == V4L2_BUF_TYPE_SDR_CAPTURE)
 		q->max_num_buffers = 1024;
 	if (buf_type == V4L2_BUF_TYPE_VBI_CAPTURE)
diff --git a/drivers/media/test-drivers/vivid/vivid-core.h b/drivers/media/test-drivers/vivid/vivid-core.h
index cc18a3bc6dc0..d2d52763b119 100644
--- a/drivers/media/test-drivers/vivid/vivid-core.h
+++ b/drivers/media/test-drivers/vivid/vivid-core.h
@@ -26,6 +26,8 @@
 #define MAX_INPUTS 16
 /* The maximum number of outputs */
 #define MAX_OUTPUTS 16
+/* The maximum number of video capture buffers */
+#define MAX_VID_CAP_BUFFERS 64
 /* The maximum up or down scaling factor is 4 */
 #define MAX_ZOOM  4
 /* The maximum image width/height are set to 4K DMT */
@@ -481,7 +483,7 @@ struct vivid_dev {
 	/* video capture */
 	struct tpg_data			tpg;
 	unsigned			ms_vid_cap;
-	bool				must_blank[VIDEO_MAX_FRAME];
+	bool				must_blank[MAX_VID_CAP_BUFFERS];

 	const struct vivid_fmt		*fmt_cap;
 	struct v4l2_fract		timeperframe_vid_cap;
diff --git a/drivers/media/test-drivers/vivid/vivid-ctrls.c b/drivers/media/test-drivers/vivid/vivid-ctrls.c
index 8bb38bc7b8cc..2b5c8fbcd0a2 100644
--- a/drivers/media/test-drivers/vivid/vivid-ctrls.c
+++ b/drivers/media/test-drivers/vivid/vivid-ctrls.c
@@ -553,7 +553,7 @@ static int vivid_vid_cap_s_ctrl(struct v4l2_ctrl *ctrl)
 		break;
 	case VIVID_CID_PERCENTAGE_FILL:
 		tpg_s_perc_fill(&dev->tpg, ctrl->val);
-		for (i = 0; i < VIDEO_MAX_FRAME; i++)
+		for (i = 0; i < MAX_VID_CAP_BUFFERS; i++)
 			dev->must_blank[i] = ctrl->val < 100;
 		break;
 	case VIVID_CID_INSERT_SAV:
diff --git a/drivers/media/test-drivers/vivid/vivid-vid-cap.c b/drivers/media/test-drivers/vivid/vivid-vid-cap.c
index 69620e0a35a0..6a790ac8cbe6 100644
--- a/drivers/media/test-drivers/vivid/vivid-vid-cap.c
+++ b/drivers/media/test-drivers/vivid/vivid-vid-cap.c
@@ -213,7 +213,7 @@ static int vid_cap_start_streaming(struct vb2_queue *vq, unsigned count)

 	dev->vid_cap_seq_count = 0;
 	dprintk(dev, 1, "%s\n", __func__);
-	for (i = 0; i < VIDEO_MAX_FRAME; i++)
+	for (i = 0; i < MAX_VID_CAP_BUFFERS; i++)
 		dev->must_blank[i] = tpg_g_perc_fill(&dev->tpg) < 100;
 	if (dev->start_streaming_error) {
 		dev->start_streaming_error = false;