linux-kernel - Re: [KJ] Re: [PATCH] bugfix GFP_KERNEL -> GFP_ATOMIC in spin

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87k5uimzkh.fsf@wanadoo.fr>
Date:	Tue, 05 Jun 2007 13:05:18 +0200
From:	Yoann Padioleau <padator@...adoo.fr>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	linux-kernel@...r.kernel.org, kernel-janitors@...ts.osdl.org
Subject: Re: [KJ] Re: [PATCH] bugfix GFP_KERNEL -> GFP_ATOMIC in spin_locked region

Andrew Morton <akpm@...ux-foundation.org> writes:

>> 
>>  net/wan/lmc/lmc_main.c        |    2 +-
>>  scsi/megaraid/megaraid_mm.c   |    2 +-
>>  usb/serial/io_ti.c            |    2 +-
>>  usb/serial/ti_usb_3410_5052.c |    2 +-
>>  usb/serial/whiteheat.c        |    6 +++---
>>  5 files changed, 7 insertions(+), 7 deletions(-)
>
> This patch covers three areas of maintainer responsibility, so poor old me
> has to split it up and redo the changelogs.  Oh well.

Sorry. 

For modifications that crosscut the kernel it is not very practical to
look each time for the maintainer. We plan to send a patch that
replaces some calls to kmalloc/memset with kzalloc; do we have to split
it for each maintainer ?

>> diff --git a/drivers/net/wan/lmc/lmc_main.c b/drivers/net/wan/lmc/lmc_main.c
>> index ae132c1..750b3ef 100644
>> --- a/drivers/net/wan/lmc/lmc_main.c
>> +++ b/drivers/net/wan/lmc/lmc_main.c
>> @@ -483,7 +483,7 @@ #endif /* end ifdef _DBG_EVENTLOG */
>>                              break;
>>                      }
>>  
>> -                    data = kmalloc(xc.len, GFP_KERNEL);
>> +                    data = kmalloc(xc.len, GFP_ATOMIC);
>>                      if(data == 0x0){
>>                              printk(KERN_WARNING "%s: Failed to allocate memory for copy\n", dev->name);
>>                              ret = -ENOMEM;
>
> A few lines later we do:
>
> 	if(copy_from_user(data, xc.data, xc.len))
>
> which also is illegal under spinlock.

I have launched my tool to detect such situations and I get this 
(in a diff-like format).

--- /home/pad/kernels/git/linux-2.6/arch/cris/arch-v10/drivers/gpio.c	2007-03-27 15:12:38.000000000 +0200
+++ /tmp/output.c	2007-06-05 11:38:47.000000000 +0200
@@ -776,7 +776,7 @@
 		/* bits set in *arg is set to input,
 		 * *arg updated with current input pins.
 		 */
-		if (copy_from_user(&val, (unsigned long*)arg, sizeof(val)))
 		{
 			ret = -EFAULT;
 			break;
@@ -789,7 +789,7 @@
 		/* bits set in *arg is set to output,
 		 * *arg updated with current output pins.
 		 */
-		if (copy_from_user(&val, (unsigned long*)arg, sizeof(val)))
 		{
 			ret = -EFAULT;
 			break;


--- /home/pad/kernels/git/linux-2.6/arch/mips/au1000/common/power.c	2007-03-27 15:12:41.000000000 +0200
+++ /tmp/output.c	2007-06-05 11:38:57.000000000 +0200
@@ -330,7 +330,7 @@
 			spin_unlock_irqrestore(&pm_lock, flags);
 			return -EFAULT;
 		}
-		if (copy_from_user(buf, buffer, *len)) {
 			spin_unlock_irqrestore(&pm_lock, flags);
 			return -EFAULT;
 		}

and some cases in lmc_main.c


>
>
> Frankly, I think that a better use of this tool would be to just report on
> the errors, let humans fix them up.

Ok. Do you have a preference on the format ?  a <file>:<line> format  ?

Is there a place that gathered all those implicit programming rules 
(that copy_from_user must not be called inside a spinlock, etc) so that
I can translate them in a script for our tool.


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/