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

[Dan Carpenter <dan.carpenter@xxxxxxxxxx>]

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);