Re: [PATCH] mm: Don't warn if memdup_user fails
|[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]|
On Wed, 11 Jan 2012, Andrew Morton wrote:
diff --git a/mm/util.c b/mm/util.c index 136ac4f..88bb4d4 100644 --- a/mm/util.c +++ b/mm/util.c @@ -91,7 +91,7 @@ void *memdup_user(const void __user *src, size_t len) * cause pagefault, which makes it pointless to use GFP_NOFS * or GFP_ATOMIC. */ - p = kmalloc_track_caller(len, GFP_KERNEL); + p = kmalloc_track_caller(len, GFP_KERNEL | __GFP_NOWARN); if (!p) return ERR_PTR(-ENOMEM);There's nothing particularly special about memdup_user(): there are many ways in which userspace can trigger GFP_KERNEL allocations. The problem here (one which your patch carefully covers up) is that ecryptfs_miscdev_write() is passing an unchecked userspace-provided `count' direct into kmalloc(). This is a bit problematic for other reasons: it gives userspace a way to trigger heavy reclaim activity and perhaps even to trigger the oom-killer. A better fix here would be to validate the incoming arg before using it. Preferably by running ecryptfs_parse_packet_length() before taking a copy of the data. That would require adding a small copy_from_user() to peek at the message header.
Yup, right you are. I didn't think about the reclaim and oom issue. We should add a big fat warning on top of memdup_user() to tell users to check 'len' for sanity themselves. I think they're now fooled into thinking memdup_user() automagically does the right thing.
Pekka -- To unsubscribe from this list: send the line "unsubscribe ecryptfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
[LARTC] [Bugtraq] [Yosemite Forum] [Photo]