Re: [PATCH] fix calls to request_module()

["Nguyen Anh Quynh" <aquynh@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2212 bytes --]

On Thu, Dec 11, 2008 at 3:00 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Thu, Dec 11, 2008 at 02:42:28PM +0900, Nguyen Anh Quynh wrote:
>> > Ah, but that's different.  Take a look at that warning and think _why_
>> > it is given and what is it about.  Getting an untrusted string as
>> > format argument is a real security hole, but it has nothing to do
>> > with a pile of cases in your patch.
>>
>> Yes, clearly the warning is to warn us about potential format string
>> bugs. But I agree that there are a lot of false possitives.
>>
>> My patch is mainly to make gcc happy.
>
> Your patch is mostly obfuscating the places gcc does *not* warn about...
>
> Looking through it, only
> drivers/media/video/cx18/cx18-driver.c
> drivers/media/video/ivtv/ivtv-driver.c
> drivers/media/video/pvrusb2/pvrusb2-hdw.c
> drivers/media/video/zoran/zoran_card.c
> drivers/mtd/chips/gen_probe.c
> drivers/net/wireless/hostap/hostap_ioctl.c
> drivers/of/of_spi.c
> drivers/usb/storage/libusual.c
> fs/dquot.c
> fs/gfs2/locking.c
> net/ieee80211/ieee80211_wx.c
> sound/core/sound.c
>
> are not of <string literal> -> "%s", <string literal> variety.  Everything
> else has never generated a format warning.  At least trim the patch,
> removing the obviously useless parts.

Yes, initially I misunderstood these warnings.

So here is the patch, again, with only the "correct" warning fixed.

Signed-off-by: Nguyen Anh Quynh <aquynh@gmail.com>

Thanks,
Quynh


# diffstat fix-request_module-call.linus_git_tree.patch
 drivers/media/video/cx18/cx18-driver.c     |    2 +-
 drivers/media/video/ivtv/ivtv-driver.c     |    2 +-
 drivers/media/video/pvrusb2/pvrusb2-hdw.c  |    2 +-
 drivers/media/video/zoran/zoran_card.c     |    8 ++++----
 drivers/mtd/chips/gen_probe.c              |    2 +-
 drivers/net/wireless/hostap/hostap_ioctl.c |    2 +-
 drivers/of/of_spi.c                        |    2 +-
 drivers/usb/storage/libusual.c             |    2 +-
 fs/dquot.c                                 |    2 +-
 fs/gfs2/locking.c                          |    2 +-
 net/ieee80211/ieee80211_wx.c               |    2 +-
 sound/core/sound.c                         |    2 +-
 12 files changed, 15 insertions(+), 15 deletions(-)

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: fix-request_module-call.linus_git_tree.patch --]
[-- Type: text/x-patch; name=fix-request_module-call.linus_git_tree.patch, Size: 6675 bytes --]

diff --git a/drivers/media/video/cx18/cx18-driver.c b/drivers/media/video/cx18/cx18-driver.c
index 7874d97..81f0d5e 100644
--- a/drivers/media/video/cx18/cx18-driver.c
+++ b/drivers/media/video/cx18/cx18-driver.c
@@ -572,7 +572,7 @@ static u32 cx18_request_module(struct cx18 *cx, u32 hw,
 {
 	if ((hw & id) == 0)
 		return hw;
-	if (request_module(name) != 0) {
+	if (request_module("%s", name) != 0) {
 		CX18_ERR("Failed to load module %s\n", name);
 		return hw & ~id;
 	}
diff --git a/drivers/media/video/ivtv/ivtv-driver.c b/drivers/media/video/ivtv/ivtv-driver.c
index b69cc1d..a26ad59 100644
--- a/drivers/media/video/ivtv/ivtv-driver.c
+++ b/drivers/media/video/ivtv/ivtv-driver.c
@@ -859,7 +859,7 @@ static u32 ivtv_request_module(struct ivtv *itv, u32 hw,
 {
 	if ((hw & id) == 0)
 		return hw;
-	if (request_module(name) != 0) {
+	if (request_module("%s", name) != 0) {
 		IVTV_ERR("Failed to load module %s\n", name);
 		return hw & ~id;
 	}
diff --git a/drivers/media/video/pvrusb2/pvrusb2-hdw.c b/drivers/media/video/pvrusb2/pvrusb2-hdw.c
index 5b81ba4..6d75d3c 100644
--- a/drivers/media/video/pvrusb2/pvrusb2-hdw.c
+++ b/drivers/media/video/pvrusb2/pvrusb2-hdw.c
@@ -1967,7 +1967,7 @@ static void pvr2_hdw_setup_low(struct pvr2_hdw *hdw)
 	if (!pvr2_hdw_dev_ok(hdw)) return;
 
 	for (idx = 0; idx < hdw->hdw_desc->client_modules.cnt; idx++) {
-		request_module(hdw->hdw_desc->client_modules.lst[idx]);
+		request_module("%s", hdw->hdw_desc->client_modules.lst[idx]);
 	}
 
 	if (!hdw->hdw_desc->flag_no_powerup) {
diff --git a/drivers/media/video/zoran/zoran_card.c b/drivers/media/video/zoran/zoran_card.c
index fa5f2f8..586ee7d 100644
--- a/drivers/media/video/zoran/zoran_card.c
+++ b/drivers/media/video/zoran/zoran_card.c
@@ -1435,7 +1435,7 @@ find_zr36057 (void)
 		}
 
 		if (i2c_dec_name) {
-			if ((result = request_module(i2c_dec_name)) < 0) {
+			if ((result = request_module("%s", i2c_dec_name)) < 0) {
 				dprintk(1,
 					KERN_ERR
 					"%s: failed to load module %s: %d\n",
@@ -1455,7 +1455,7 @@ find_zr36057 (void)
 		}
 
 		if (i2c_enc_name) {
-			if ((result = request_module(i2c_enc_name)) < 0) {
+			if ((result = request_module("%s", i2c_enc_name)) < 0) {
 				dprintk(1,
 					KERN_ERR
 					"%s: failed to load module %s: %d\n",
@@ -1478,7 +1478,7 @@ find_zr36057 (void)
 		if (zr->card.video_codec != 0 &&
 		    (codec_name =
 		     codecid_to_modulename(zr->card.video_codec)) != NULL) {
-			if ((result = request_module(codec_name)) < 0) {
+			if ((result = request_module("%s", codec_name)) < 0) {
 				dprintk(1,
 					KERN_ERR
 					"%s: failed to load modules %s: %d\n",
@@ -1488,7 +1488,7 @@ find_zr36057 (void)
 		if (zr->card.video_vfe != 0 &&
 		    (vfe_name =
 		     codecid_to_modulename(zr->card.video_vfe)) != NULL) {
-			if ((result = request_module(vfe_name)) < 0) {
+			if ((result = request_module("%s", vfe_name)) < 0) {
 				dprintk(1,
 					KERN_ERR
 					"%s: failed to load modules %s: %d\n",
diff --git a/drivers/mtd/chips/gen_probe.c b/drivers/mtd/chips/gen_probe.c
index e2dc964..814d4f3 100644
--- a/drivers/mtd/chips/gen_probe.c
+++ b/drivers/mtd/chips/gen_probe.c
@@ -212,7 +212,7 @@ static inline struct mtd_info *cfi_cmdset_unknown(struct map_info *map,
 
 	probe_function = __symbol_get(probename);
 	if (!probe_function) {
-		request_module(probename + sizeof(MODULE_SYMBOL_PREFIX) - 1);
+		request_module("%s", probename + sizeof(MODULE_SYMBOL_PREFIX) - 1);
 		probe_function = __symbol_get(probename);
 	}
 
diff --git a/drivers/net/wireless/hostap/hostap_ioctl.c b/drivers/net/wireless/hostap/hostap_ioctl.c
index 3f8b1d7..ea03992 100644
--- a/drivers/net/wireless/hostap/hostap_ioctl.c
+++ b/drivers/net/wireless/hostap/hostap_ioctl.c
@@ -3297,7 +3297,7 @@ static int prism2_ioctl_siwencodeext(struct net_device *dev,
 
 	ops = ieee80211_get_crypto_ops(alg);
 	if (ops == NULL) {
-		request_module(module);
+		request_module("%s", module);
 		ops = ieee80211_get_crypto_ops(alg);
 	}
 	if (ops == NULL) {
diff --git a/drivers/of/of_spi.c b/drivers/of/of_spi.c
index bed0ed6..ad96437 100644
--- a/drivers/of/of_spi.c
+++ b/drivers/of/of_spi.c
@@ -82,7 +82,7 @@ void of_register_spi_devices(struct spi_master *master, struct device_node *np)
 		spi->dev.archdata.of_node = nc;
 
 		/* Register the new device */
-		request_module(spi->modalias);
+		request_module("%s", spi->modalias);
 		rc = spi_add_device(spi);
 		if (rc) {
 			dev_err(&master->dev, "spi_device register error %s\n",
diff --git a/drivers/usb/storage/libusual.c b/drivers/usb/storage/libusual.c
index d617e8a..a481e02 100644
--- a/drivers/usb/storage/libusual.c
+++ b/drivers/usb/storage/libusual.c
@@ -180,7 +180,7 @@ static int usu_probe_thread(void *arg)
 	unsigned long flags;
 
 	mutex_lock(&usu_probe_mutex);
-	rc = request_module(bias_names[type]);
+	rc = request_module("%s", bias_names[type]);
 	spin_lock_irqsave(&usu_lock, flags);
 	if (rc == 0 && (st->fls & USU_MOD_FL_PRESENT) == 0) {
 		/*
diff --git a/fs/dquot.c b/fs/dquot.c
index 5e95261..3984307 100644
--- a/fs/dquot.c
+++ b/fs/dquot.c
@@ -167,7 +167,7 @@ static struct quota_format_type *find_quota_format(int id)
 		spin_unlock(&dq_list_lock);
 		
 		for (qm = 0; module_names[qm].qm_fmt_id && module_names[qm].qm_fmt_id != id; qm++);
-		if (!module_names[qm].qm_fmt_id || request_module(module_names[qm].qm_mod_name))
+		if (!module_names[qm].qm_fmt_id || request_module("%s", module_names[qm].qm_mod_name))
 			return NULL;
 
 		spin_lock(&dq_list_lock);
diff --git a/fs/gfs2/locking.c b/fs/gfs2/locking.c
index 523243a..7a9df7f 100644
--- a/fs/gfs2/locking.c
+++ b/fs/gfs2/locking.c
@@ -177,7 +177,7 @@ retry:
 		if (!try && capable(CAP_SYS_MODULE)) {
 			try = 1;
 			mutex_unlock(&lmh_lock);
-			request_module(proto_name);
+			request_module("%s", proto_name);
 			goto retry;
 		}
 		printk(KERN_INFO "GFS2: can't find protocol %s\n", proto_name);
diff --git a/net/ieee80211/ieee80211_wx.c b/net/ieee80211/ieee80211_wx.c
index 973832d..de4be03 100644
--- a/net/ieee80211/ieee80211_wx.c
+++ b/net/ieee80211/ieee80211_wx.c
@@ -608,7 +608,7 @@ int ieee80211_wx_set_encodeext(struct ieee80211_device *ieee,
 
 	ops = ieee80211_get_crypto_ops(alg);
 	if (ops == NULL) {
-		request_module(module);
+		request_module("%s", module);
 		ops = ieee80211_get_crypto_ops(alg);
 	}
 	if (ops == NULL) {
diff --git a/sound/core/sound.c b/sound/core/sound.c
index 44a69bb..c143b9c 100644
--- a/sound/core/sound.c
+++ b/sound/core/sound.c
@@ -88,7 +88,7 @@ static void snd_request_other(int minor)
 	case SNDRV_MINOR_TIMER:		str = "snd-timer";	break;
 	default:			return;
 	}
-	request_module(str);
+	request_module("%s", str);
 }
 
 #endif	/* modular kernel */