softraid_raid5: possible NULL dereference

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

softraid_raid5: possible NULL dereference

Tobias Heider-2
sr_block_get() returns dma_alloc(length, PR_NOWAIT | PR_ZERO) which may be
NULL if the memory pool is depleted.
The result is used as 'dst' argument to memcpy() in the following call to
sr_raid5_regenerate(), resulting in a possible NULL dereference.

ok?

Index: softraid_raid5.c
===================================================================
RCS file: /mount/openbsd/cvs/src/sys/dev/softraid_raid5.c,v
retrieving revision 1.29
diff -u -p -r1.29 softraid_raid5.c
--- softraid_raid5.c 8 Aug 2019 02:19:55 -0000 1.29
+++ softraid_raid5.c 25 Mar 2020 23:54:25 -0000
@@ -818,7 +818,8 @@ sr_raid5_rebuild(struct sr_discipline *s
  wu_w = sr_scsi_wu_get(sd, 0);
  wu_r = sr_scsi_wu_get(sd, 0);
 
- xorbuf = sr_block_get(sd, strip_size);
+ if ((xorbuf = sr_block_get(sd, strip_size)) == NULL)
+ goto bad;
  if (sr_raid5_regenerate(wu_r, rebuild_chunk, chunk_lba,
     strip_size, xorbuf))
  goto bad;

Reply | Threaded
Open this post in threaded view
|

Re: softraid_raid5: possible NULL dereference

Stefan Sperling-5
On Thu, Mar 26, 2020 at 01:03:26AM +0100, Tobias Heider wrote:

> sr_block_get() returns dma_alloc(length, PR_NOWAIT | PR_ZERO) which may be
> NULL if the memory pool is depleted.
> The result is used as 'dst' argument to memcpy() in the following call to
> sr_raid5_regenerate(), resulting in a possible NULL dereference.
>
> ok?
>
> Index: softraid_raid5.c
> ===================================================================
> RCS file: /mount/openbsd/cvs/src/sys/dev/softraid_raid5.c,v
> retrieving revision 1.29
> diff -u -p -r1.29 softraid_raid5.c
> --- softraid_raid5.c 8 Aug 2019 02:19:55 -0000 1.29
> +++ softraid_raid5.c 25 Mar 2020 23:54:25 -0000
> @@ -818,7 +818,8 @@ sr_raid5_rebuild(struct sr_discipline *s
>   wu_w = sr_scsi_wu_get(sd, 0);
>   wu_r = sr_scsi_wu_get(sd, 0);
>  
> - xorbuf = sr_block_get(sd, strip_size);
> + if ((xorbuf = sr_block_get(sd, strip_size)) == NULL)
> + goto bad;
>   if (sr_raid5_regenerate(wu_r, rebuild_chunk, chunk_lba,
>      strip_size, xorbuf))
>   goto bad;
>

Obvious fix. ok stsp@

Same problem is present in sr_raid5_scrub() which is currently in #if 0.

Reply | Threaded
Open this post in threaded view
|

Re: softraid_raid5: possible NULL dereference

Mark Kettenis
In reply to this post by Tobias Heider-2
> Date: Thu, 26 Mar 2020 01:03:26 +0100
> From: Tobias Heider <[hidden email]>
>
> sr_block_get() returns dma_alloc(length, PR_NOWAIT | PR_ZERO) which may be
> NULL if the memory pool is depleted.
> The result is used as 'dst' argument to memcpy() in the following call to
> sr_raid5_regenerate(), resulting in a possible NULL dereference.
>
> ok?
>
> Index: softraid_raid5.c
> ===================================================================
> RCS file: /mount/openbsd/cvs/src/sys/dev/softraid_raid5.c,v
> retrieving revision 1.29
> diff -u -p -r1.29 softraid_raid5.c
> --- softraid_raid5.c 8 Aug 2019 02:19:55 -0000 1.29
> +++ softraid_raid5.c 25 Mar 2020 23:54:25 -0000
> @@ -818,7 +818,8 @@ sr_raid5_rebuild(struct sr_discipline *s
>   wu_w = sr_scsi_wu_get(sd, 0);
>   wu_r = sr_scsi_wu_get(sd, 0);
>  
> - xorbuf = sr_block_get(sd, strip_size);
> + if ((xorbuf = sr_block_get(sd, strip_size)) == NULL)
> + goto bad;
>   if (sr_raid5_regenerate(wu_r, rebuild_chunk, chunk_lba,
>      strip_size, xorbuf))
>   goto bad;

As a matter of style, it is better to avoid assignments in if-statements.