Re: [PATCH 8/9] ib_srpt: Convert srp_max_rsp_size into per port configfs attribute

["Nicholas A. Bellinger" <nab@linux-iscsi.org>]

On Mon, 2011-10-24 at 22:11 +0200, Bart Van Assche wrote:
> On Mon, Oct 24, 2011 at 10:05 PM, Nicholas A. Bellinger
> <nab@linux-iscsi.org> wrote:
> > On Mon, 2011-10-24 at 21:58 +0200, Bart Van Assche wrote:
> >> On Mon, Oct 24, 2011 at 9:49 PM, Nicholas A. Bellinger
> >> <nab@linux-iscsi.org> wrote:
> >> > On Mon, 2011-10-24 at 21:44 +0200, Bart Van Assche wrote:
> >> >> On Mon, Oct 24, 2011 at 7:33 AM, Nicholas A. Bellinger
> >> >> <nab@linux-iscsi.org> wrote:
> >> >> > +static ssize_t srpt_tpg_attrib_store_srp_max_rsp_size(
> >> >> > +       struct se_portal_group *se_tpg,
> >> >> > +       const char *page,
> >> >> > +       size_t count)
> >> >> > +{
> >> >> > +       struct srpt_port *sport = container_of(se_tpg, struct srpt_port, port_tpg_1);
> >> >> > +       unsigned long val;
> >> >> > +       int ret;
> >> >> > +
> >> >> > +       ret = strict_strtoul(page, 0, &val);
> >> >>
> >> >> If the data "page" points at only consists of digits, the above
> >> >> strict_strtoul() call will trigger a past-end-of-buffer read.
> >> >
> >> > I don't understand what you mean here.  Can you provide a test case to
> >> > demonstrate please..?
> >>
> >> echo -n "345" >$configfs_path_of_parameter.
> >
> > Still not sure what your getting at here..?
> 
> Only the data in page[0..count-1] is guaranteed to be initialized.
> strict_strtoul() will read until it either finds whitespace or a
> binary zero, so if the data in page[] does neither contain whitespace
> nor a binary zero then strict_strtoul() will read past the end of the
> data in page[]. There may be any data at page[count], including a
> valid digit.
> 

That is part obvious.  The point your missing is that configfs is
already sanitizing the the incoming buffer in fs/configfs/file.c to work
with strict_strtoul() here:

static int
fill_write_buffer(struct configfs_buffer * buffer, const char __user * buf, size_t count)
{
        int error;

        if (!buffer->page)
                buffer->page = (char *)__get_free_pages(GFP_KERNEL, 0);
        if (!buffer->page)
                return -ENOMEM;

        if (count >= SIMPLE_ATTR_SIZE)
                count = SIMPLE_ATTR_SIZE - 1;
        error = copy_from_user(buffer->page,buf,count);
        buffer->needs_read_fill = 1;
        /* if buf is assumed to contain a string, terminate it by \0,
         * so e.g. sscanf() can scan the string easily */
        buffer->page[count] = 0;
        return error ? -EFAULT : count;
}