[bug report] [SCSI] iscsi_transport: Add support to display CHAP list and delete CHAP entry

[Dan Carpenter]

Hi SCSI and netlink developers,

Commit 6260a5d22122 ("[SCSI] iscsi_transport: Add support to display
CHAP list and delete CHAP entry") from Feb 27, 2012 (linux-next),
leads to the following Smatch static checker warning:

drivers/scsi/scsi_transport_iscsi.c:3340 iscsi_get_chap()
warn: potential user controlled sizeof overflow 'ev->u.get_chap.num_entries * 524' '0-u32max * 524' type='uint'

drivers/scsi/scsi_transport_iscsi.c
3319 static int
3320 iscsi_get_chap(struct iscsi_transport *transport, struct nlmsghdr *nlh)
3321 {
3322 struct iscsi_uevent *ev = nlmsg_data(nlh);
3323 struct Scsi_Host *shost = NULL;
3324 struct iscsi_chap_rec *chap_rec;
3325 struct iscsi_internal *priv;
3326 struct sk_buff *skbchap;
3327 struct nlmsghdr *nlhchap;
3328 struct iscsi_uevent *evchap;
3329 uint32_t chap_buf_size;
3330 int len, err = 0;
3331 char *buf;
3332
3333 if (!transport->get_chap)
3334 return -EINVAL;
3335
3336 priv = iscsi_if_transport_lookup(transport);
3337 if (!priv)
3338 return -EINVAL;
3339
--> 3340 chap_buf_size = (ev->u.get_chap.num_entries * sizeof(*chap_rec));
3341 len = nlmsg_total_size(sizeof(*ev) + chap_buf_size);

Smatch doesn't trust nlmsg_data(). ev->u.get_chap.num_entries and
chap_buf_size are both u32 types so it looks like this integer overflow
warning is valid. On the other hand, hopefully, you trust your ISCSI
transport.

Then we pass the overflowed value to nlmsg_total_size() and do three
more integer overflows:

1) sizeof(*ev) + chap_buf_size
2) NLMSG_HDRLEN + payload
3) NLMSG_ALIGN() (ALIGN macros wrap to zero)

So my solution was going to be use size_mul(ev->u.get_chap.num_entries,
sizeof(*chap_rec)) for the multiply.

I kind of want it to be a static checker error when code uses
size_add/mul() and saves the result to anything except unsigned long.
Or when code uses the result to do further math. The problem with
this is that people like struct_size() and use it even when they know
the result can't overflow so this generates false positive warnings.

Also maybe we should harden nlmsg_total_size() against integer
overflows?

regards,
dan carpenter

3342
3343 shost = scsi_host_lookup(ev->u.get_chap.host_no);
3344 if (!shost) {
3345 printk(KERN_ERR "%s: failed. Could not find host no %u\n",
3346 __func__, ev->u.get_chap.host_no);
3347 return -ENODEV;
3348 }
3349
3350 do {
3351 int actual_size;
3352
3353 skbchap = alloc_skb(len, GFP_KERNEL);
3354 if (!skbchap) {
3355 printk(KERN_ERR "can not deliver chap: OOM\n");
3356 err = -ENOMEM;
3357 goto exit_get_chap;
3358 }
3359
3360 nlhchap = __nlmsg_put(skbchap, 0, 0, 0,
3361 (len - sizeof(*nlhchap)), 0);