[PATCH] ipr: Fix regression when loading firmware

[PATCH] ipr: Fix regression when loading firmware

[Gabriel Krisman Bertazi]

Commit d63c7dd5bcb9 ("ipr: Fix out-of-bounds null overwrite") removed the
end of line handling when storing the update_fw sysfs attribute.  This
changed the userpace API because it started refusing writes terminated
by a line feed, which broke the update tools we already have.

This patch re-adds that handling, so both a write terminated by a line
feed or not can make it through with the update.

Fixes: d63c7dd5bcb9 ("ipr: Fix out-of-bounds null overwrite")
Signed-off-by: Gabriel Krisman Bertazi <krisman@linux.vnet.ibm.com>
Cc: Insu Yun <wuninsu@gmail.com>
---
 drivers/scsi/ipr.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
index 3b3e099..d6a691e 100644
--- a/drivers/scsi/ipr.c
+++ b/drivers/scsi/ipr.c
@@ -4002,6 +4002,7 @@ static ssize_t ipr_store_update_fw(struct device *dev,
 	struct ipr_sglist *sglist;
 	char fname[100];
 	char *src;
+	char *endline;
 	int result, dnld_size;
 
 	if (!capable(CAP_SYS_ADMIN))
@@ -4009,6 +4010,10 @@ static ssize_t ipr_store_update_fw(struct device *dev,
 
 	snprintf(fname, sizeof(fname), "%s", buf);
 
+	endline = strchr(fname, '\n');
+	if (endline)
+		*endline = '\0';
+
 	if (request_firmware(&fw_entry, fname, &ioa_cfg->pdev->dev)) {
 		dev_err(&ioa_cfg->pdev->dev, "Firmware file %s not found\n", fname);
 		return -EIO;
-- 
2.1.0

Re: [PATCH] ipr: Fix regression when loading firmware

[Brian King]

On 02/25/2016 10:54 AM, Gabriel Krisman Bertazi wrote:
> Commit d63c7dd5bcb9 ("ipr: Fix out-of-bounds null overwrite") removed the
> end of line handling when storing the update_fw sysfs attribute.  This
> changed the userpace API because it started refusing writes terminated
> by a line feed, which broke the update tools we already have.
> 
> This patch re-adds that handling, so both a write terminated by a line
> feed or not can make it through with the update.
> 
> Fixes: d63c7dd5bcb9 ("ipr: Fix out-of-bounds null overwrite")
> Signed-off-by: Gabriel Krisman Bertazi <krisman@linux.vnet.ibm.com>
> Cc: Insu Yun <wuninsu@gmail.com>
> ---
>  drivers/scsi/ipr.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
> index 3b3e099..d6a691e 100644
> --- a/drivers/scsi/ipr.c
> +++ b/drivers/scsi/ipr.c
> @@ -4002,6 +4002,7 @@ static ssize_t ipr_store_update_fw(struct device *dev,
>  	struct ipr_sglist *sglist;
>  	char fname[100];
>  	char *src;
> +	char *endline;
>  	int result, dnld_size;
> 
>  	if (!capable(CAP_SYS_ADMIN))
> @@ -4009,6 +4010,10 @@ static ssize_t ipr_store_update_fw(struct device *dev,
> 
>  	snprintf(fname, sizeof(fname), "%s", buf);
> 
> +	endline = strchr(fname, '\n');
> +	if (endline)
> +		*endline = '\0';
> +
>  	if (request_firmware(&fw_entry, fname, &ioa_cfg->pdev->dev)) {
>  		dev_err(&ioa_cfg->pdev->dev, "Firmware file %s not found\n", fname);
>  		return -EIO;
> 

Acked-by: Brian King <brking@linux.vnet.ibm.com>

James - since this is a regression, can we get this fix in for 4.5 yet?

Thanks,

Brian

-- 
Brian King
Power Linux I/O
IBM Linux Technology Center

Re: [PATCH] ipr: Fix regression when loading firmware

[James Bottomley]

On Fri, 2016-02-26 at 14:52 -0600, Brian King wrote:
> On 02/25/2016 10:54 AM, Gabriel Krisman Bertazi wrote:
> > Commit d63c7dd5bcb9 ("ipr: Fix out-of-bounds null overwrite")
> > removed the
> > end of line handling when storing the update_fw sysfs attribute. 
> >  This
> > changed the userpace API because it started refusing writes
> > terminated
> > by a line feed, which broke the update tools we already have.
> > 
> > This patch re-adds that handling, so both a write terminated by a
> > line
> > feed or not can make it through with the update.
> > 
> > Fixes: d63c7dd5bcb9 ("ipr: Fix out-of-bounds null overwrite")
> > Signed-off-by: Gabriel Krisman Bertazi <krisman@linux.vnet.ibm.com>
> > Cc: Insu Yun <wuninsu@gmail.com>
> > ---
> >  drivers/scsi/ipr.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
> > index 3b3e099..d6a691e 100644
> > --- a/drivers/scsi/ipr.c
> > +++ b/drivers/scsi/ipr.c
> > @@ -4002,6 +4002,7 @@ static ssize_t ipr_store_update_fw(struct
> > device *dev,
> >  	struct ipr_sglist *sglist;
> >  	char fname[100];
> >  	char *src;
> > +	char *endline;
> >  	int result, dnld_size;
> > 
> >  	if (!capable(CAP_SYS_ADMIN))
> > @@ -4009,6 +4010,10 @@ static ssize_t ipr_store_update_fw(struct
> > device *dev,
> > 
> >  	snprintf(fname, sizeof(fname), "%s", buf);
> > 
> > +	endline = strchr(fname, '\n');
> > +	if (endline)
> > +		*endline = '\0';
> > +
> >  	if (request_firmware(&fw_entry, fname, &ioa_cfg->pdev
> > ->dev)) {
> >  		dev_err(&ioa_cfg->pdev->dev, "Firmware file %s not
> > found\n", fname);
> >  		return -EIO;
> > 
> 
> Acked-by: Brian King <brking@linux.vnet.ibm.com>
> 
> James - since this is a regression, can we get this fix in for 4.5 
> yet?

Yes, but in future, could you actually check patches in your driver
before they get sent to Linus?  This one went via the usual merge
window process, so there was plenty of time to test it. We get a lot of
these apparently innocuous minor bug fixes that actually contain a much
more subtle bug.  They're very difficult for reviewers to spot, but
they do show up on hardware testing.

James
 

Re: [PATCH] ipr: Fix regression when loading firmware

[Martin K. Petersen]

>>>>> "Gabriel" == Gabriel Krisman Bertazi <krisman@linux.vnet.ibm.com> writes:

Gabriel> Commit d63c7dd5bcb9 ("ipr: Fix out-of-bounds null overwrite")
Gabriel> removed the end of line handling when storing the update_fw
Gabriel> sysfs attribute.  This changed the userpace API because it
Gabriel> started refusing writes terminated by a line feed, which broke
Gabriel> the update tools we already have.

Gabriel> This patch re-adds that handling, so both a write terminated by
Gabriel> a line feed or not can make it through with the update.

Applied to 4.5/scsi-fixes.

-- 
Martin K. Petersen	Oracle Linux Engineering