Hello Dan Carpenter,
Thank you for your comments.
W dniu 13.12.2013 12:22, Dan Carpenter pisze:
Hello Andrzej Pietrasiewicz,
The patch 83167f12da05: "usb: gadget: nokia: convert to new interface
of f_phonet" from May 23, 2013, leads to the following Smatch
warnings:
drivers/usb/gadget/nokia.c:209 nokia_bind_config() error: potential NULL dereference 'f_obex2'.
drivers/usb/gadget/nokia.c:211 nokia_bind_config() error: potential NULL dereference 'f_obex1'.
drivers/usb/gadget/nokia.c:213 nokia_bind_config() error: potential NULL dereference 'f_phonet'.
Let me walk you throug the code for the first warning.
drivers/usb/gadget/nokia.c
115 static struct usb_function_instance *fi_acm;
116 static struct usb_function_instance *fi_ecm;
117 static struct usb_function_instance *fi_obex1;
118 static struct usb_function_instance *fi_obex2;
119 static struct usb_function_instance *fi_phonet;
120
121 static int __init nokia_bind_config(struct usb_configuration *c)
122 {
123 struct usb_function *f_acm;
124 struct usb_function *f_phonet = NULL;
125 struct usb_function *f_obex1 = NULL;
126 struct usb_function *f_ecm;
127 struct usb_function *f_obex2 = NULL;
128 int status = 0;
129 int obex1_stat = 0;
130 int obex2_stat = 0;
131 int phonet_stat = 0;
We need to trace three variables to understand the first warning
message: fi_obex2, f_obex2, and obex2_stat.
Lets start with the assumption that "fi_obex2" is an error pointer at
the start of the function.
f_obex2 is NULL.
obex2_stat is zero.
132
133 if (!IS_ERR(fi_phonet)) {
134 f_phonet = usb_get_function(fi_phonet);
135 if (IS_ERR(f_phonet))
136 pr_debug("could not get phonet function\n");
137 }
138
139 if (!IS_ERR(fi_obex1)) {
140 f_obex1 = usb_get_function(fi_obex1);
141 if (IS_ERR(f_obex1))
142 pr_debug("could not get obex function 0\n");
It's funny that the human readable messages start counting from zero as
in "obex function 0" and "obex function 1" but the computer code starts
counting from 1 as in "fi_obex1" and "fi_obex2". Normally you would
expect it to be the other way around. Also this is not consistent with
the rest of the driver.
143 }
144
145 if (!IS_ERR(fi_obex2)) {
146 f_obex2 = usb_get_function(fi_obex2);
147 if (IS_ERR(f_obex2))
148 pr_debug("could not get obex function 1\n");
149 }
fi_obex2 is an error pointer.
f_obex2 is NULL.
obex2_stat is zero.
150
151 f_acm = usb_get_function(fi_acm);
152 if (IS_ERR(f_acm)) {
153 status = PTR_ERR(f_acm);
154 goto err_get_acm;
155 }
156
157 f_ecm = usb_get_function(fi_ecm);
158 if (IS_ERR(f_ecm)) {
159 status = PTR_ERR(f_ecm);
160 goto err_get_ecm;
161 }
162
163 if (!IS_ERR_OR_NULL(f_phonet)) {
164 phonet_stat = usb_add_function(c, f_phonet);
165 if (phonet_stat)
166 pr_debug("could not add phonet function\n");
167 }
168
169 if (!IS_ERR_OR_NULL(f_obex1)) {
170 obex1_stat = usb_add_function(c, f_obex1);
171 if (obex1_stat)
172 pr_debug("could not add obex function 0\n");
173 }
174
175 if (!IS_ERR_OR_NULL(f_obex2)) {
176 obex2_stat = usb_add_function(c, f_obex2);
177 if (obex2_stat)
178 pr_debug("could not add obex function 1\n");
179 }
f_obex2 is NULL so the condition is false.
obex2_stat is zero.
180
181 status = usb_add_function(c, f_acm);
182 if (status)
183 goto err_conf;
Let's assume we hit this goto.
184
185 status = usb_add_function(c, f_ecm);
186 if (status) {
187 pr_debug("could not bind ecm config %d\n", status);
188 goto err_ecm;
189 }
190 if (c == &nokia_config_500ma_driver) {
191 f_acm_cfg1 = f_acm;
192 f_ecm_cfg1 = f_ecm;
193 f_phonet_cfg1 = f_phonet;
194 f_obex1_cfg1 = f_obex1;
195 f_obex2_cfg1 = f_obex2;
196 } else {
197 f_acm_cfg2 = f_acm;
198 f_ecm_cfg2 = f_ecm;
199 f_phonet_cfg2 = f_phonet;
200 f_obex1_cfg2 = f_obex1;
201 f_obex2_cfg2 = f_obex2;
Btw, we are assigning an ERR_PTR to the global here. It means we need
to constantly check it, which the code does, but it's messy. The driver
would be cleaner if we never did this.
202 }
203
204 return status;
Just "return 0" here to make it more obvious that we are returning zero.
205 err_ecm:
206 usb_remove_function(c, f_acm);
207 err_conf:
208 if (!obex2_stat)
209 usb_remove_function(c, f_obex2);
obex2_stat is zero.
f_obex2 is NULL.
This causes a NULL dereference.
We could initialize obex2_stat to -1 instead of 0 which would fix the
NULL deref
I'd choose this approach.
but the real problem is that this code is quite messy.
Please see below.
We should be clearing the ERR_PTRs right away if we aren't going to use
the values.
f_obex1 = usb_get_function(fi_obex1);
if (IS_ERR(f_obex1)) {
pr_debug("could not get obex function 0\n");
f_obex1 = NULL;
}
I'm not very familiar with this hardware, why is it not an error when
the usb_get_function() and usb_add_function() calls fail?
The driver is not hardware-related; it is isolated from the hardware by the
udc-core layer.
The point here is that this is a legacy gadget (not using configfs), and
this behaviour follows the original design of this gadget: some USB functions
it provides are optional. Which means that even if they cannot be successfully
found (usb_get_function() fails) the gadget should still bind with reduced
functionality.
210 if (!obex1_stat)
211 usb_remove_function(c, f_obex1);
212 if (!phonet_stat)
213 usb_remove_function(c, f_phonet);
214 usb_put_function(f_ecm);
215 err_get_ecm:
216 usb_put_function(f_acm);
217 err_get_acm:
218 if (!IS_ERR_OR_NULL(f_obex2))
219 usb_put_function(f_obex2);
220 if (!IS_ERR_OR_NULL(f_obex1))
221 usb_put_function(f_obex1);
222 if (!IS_ERR_OR_NULL(f_phonet))
223 usb_put_function(f_phonet);
224 return status;
225 }
regards,
dan carpenter
Thanks,
Andrzej Pietrasiewicz
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
